Prevent DLL hijacking by setting default DLL directories on initialization #4207
Conversation
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
## Walkthrough
This pull request introduces a new `DLLSearchPaths` option for Windows systems, allowing users to specify DLL search paths. It adds a changelog entry, updates the Windows frontend to conditionally call a Windows API function based on the new option, and extends the Windows options package with additional constants and a new field. The changes enhance configurability regarding DLL file locations without impacting existing functionality.
## Changes
| File(s) | Change Summary |
|---------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------|
| website/src/.../changelog.mdx | Added a new changelog entry for the DLLSearchPaths option to control DLL search paths on Windows. |
| v2/internal/frontend/desktop/windows/frontend.go, v2/pkg/options/windows/windows.go | Introduced DLLSearchPaths support by adding a new import from the Windows package, a conditional call to SetDefaultDllDirectories in NewFrontend, and new constants and a field in the Options struct to manage DLL search configurations. |
| v2/internal/frontend/desktop/windows/winc/w32/uxtheme.go, v2/internal/platform/win32/consts.go | Changed function names from `init` to `Init` in both files to make them exported, allowing access from other packages while keeping internal logic unchanged. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant App as Application
participant Options as AppOptions.Windows
participant WinPkg as Windows Package (w)
App->>Options: Check if Windows options exist and DLLSearchPaths > 0
alt Valid DLLSearchPaths
App->>WinPkg: Call SetDefaultDllDirectories(DLLSearchPaths)
else Missing/Zero Value
App-->>App: Skip DLL configuration
endSuggested reviewers
Poem
|
|
Thanks for taking the time to open this 🙏 I absolutely see the need for this but the solution will need to be more flexible to accommodate everyone's use cases. Some people ship DLLs with their binaries using NSIS and this will break their apps in a way they can't fix. Proposal: create a windows specific application setting that lists the set of search paths. Let's call it DLLSearchPaths. If this is blank, we do nothing and default to Go's defaults. If given, then we set them at application startup. This will mean updating most of the win32 package as you've noted. Thoughts? |
|
Do you think we need to change |
Thanks for highlighting this. I didn’t get a chance to test the new changes as my Windows laptop is having some issues. I would prefer changing func init() to func Init() in both uxtheme.go and consts.go for a minimal impact. |
|
Sounds good. Can you please make sure that the changes work correctly and they don't flicker windows when they start up or anything? That is my only concern with converting the init function. |
Sure i will do proper testing and update you here. |
|
Tested on Windows 10, I did not observe any issues. |
leaanthony
reviewed
Apr 18, 2025
| @@ -80,7 +80,7 @@ ShouldSystemUseDarkMode = bool () // ordinal 138 | |||
| SetPreferredAppMode = PreferredAppMode (PreferredAppMode appMode) // ordinal 135, since 18334 | |||
| IsDarkModeAllowedForApp = bool () // ordinal 139 | |||
| */ | |||
| func init() { | |||
| func Init() { | |||
There was a problem hiding this comment.
Did you mean to call these Init() methods?
There was a problem hiding this comment.
Sorry for the confusion. I misunderstood Go's initialization behavior, thinking capitalization affected execution order of Init() but relized it will behave like very other exported func. Now calling it explicitly after setting the dll path.
|
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.
Description
This PR fixes a DLL hijacking vulnerability identified during a VAPT assessment. When the application tries to load certain DLLs from the application directory instead of the system directories, it creates a potential security risk where an attacker could place malicious DLLs in the application directory.
The issue affects several DLLs including:
While most DLLs can be fixed by adding
windows.SetDefaultDllDirectories(windows.LOAD_LIBRARY_SEARCH_SYSTEM32)in application code, theuxtheme.dllis loaded by the Wails before application initialization. This PR addresses this issue by adding the secure DLL loading directive at the framework level.Additional Background Information
Our security team recently discovered a DLL hijacking vulnerability in our application. After digging into it, I found that this is actually a common issue in Go applications - there's even a similar bug report in the Go repository (golang/go#64411).
Here's what's happening: When Windows looks for DLLs, it first checks the application directory before checking system directories. This creates a security risk where someone could drop malicious versions of these DLLs into our application folder.
I initially tried fixing this in our code by adding
windows.SetDefaultDllDirectories(windows.LOAD_LIBRARY_SEARCH_SYSTEM32), which tells Windows to prioritize loading DLLs from the System32 directory. This worked for most DLLs, but not foruxtheme.dll, it loaded by Wails before application initialization, so our fix never gets a chance to take effect.It's an important security improvement that protects applications from a potentially serious attack vector where bad actors could execute arbitrary code through fake DLLs.Type of change
Please select the option that is relevant.
Steps to Reproduce
<Application Directory>\uxtheme.dlland observeHow Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit