Harden scc_lib.dll load against search-order hijack#148
Open
JustinMDotNet wants to merge 1 commit into
Open
Conversation
The previous LoadLibrary(sccLib.c_str()) call had two problems: first, if the scc command was ever a relative path, replace_filename produced a relative path and LoadLibrary fell back to the standard search order (CWD, %PATH%); second, even with an absolute path, scc_lib.dll's own imports were resolved with the default search order because no LOAD_LIBRARY_SEARCH_* flags were passed. Both paths let an attacker-writable directory win. Refuse to do the in-process load when sccPath is not absolute (fall back to chaining the original Global_ExecuteProcess), and use LoadLibraryExW with LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32 so scc_lib.dll's dependencies resolve from its own directory plus System32 only. Fixes wopss#138 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Do you know what else is writable by the users? ./red4ext/plugins |
|
Average slop PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #138.
What this changes
In
ExecuteProcess.cpp, the load ofscc_lib.dllis hardened in two ways:aCommandis relative,replace_filenameproduces a relative path and the originalLoadLibrarywould fall back to the standard search order (application dir, system dirs, current dir,%PATH%). We now log a warning and chain to the originalGlobal_ExecuteProcesswithout doing the in-process load.LoadLibraryExWwith safe-search flags. Even when the path is absolute, the previous code allowedscc_lib.dll's own dependencies to be resolved with the default search order. The new call passesLOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32, which restricts the search to the directoryscc_lib.dllwas loaded from plusSystem32.Why
bin\x64is writable by the user who runs the game, and the process working directory often is too. Either an attacker-planted DLL namedscc_lib.dllor one of its imports could end up loaded into the game process. Per theLoadLibraryExdocs, theLOAD_LIBRARY_SEARCH_*flag family is the documented way to opt out of the legacy search-order behavior.Design choice
Flag combination is
LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32:LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR—scc_lib.dll's dependencies resolve from the directory it lives in (the redscript install dir), which is what the previous behavior intended for normal users.LOAD_LIBRARY_SEARCH_SYSTEM32— system DLLs (kernel32,vcruntime, etc.) resolve fromSystem32only.Notably not included:
LOAD_LIBRARY_SEARCH_APPLICATION_DIR— would letbin\x64win, which is exactly the surface we want to close.LOAD_LIBRARY_SEARCH_USER_DIRS— would honor anything added viaAddDllDirectory, which we do not want a third-party plugin to influence.If redscript ships
scc_lib.dllnext to its other dependencies (the usual case), this is sufficient. If a future scc bundle relies on a DLL from somewhere else, we will see a cleanLoadLibraryfailure withERROR_MOD_NOT_FOUNDand can add the flag deliberately — much better than silently picking up a planted DLL.Happy to switch to
LOAD_LIBRARY_SEARCH_DEFAULT_DIRSinstead if you would rather match the documented "modern safe default."What I tested
Local rebuild. I did not exercise the redscript path itself; the change is mechanical and the docs match what we want here. If you have an existing scc smoke test, that would be the easiest validation.