Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows executables: only load imported DLLs from System32 #85227

Closed
wants to merge 3 commits into from

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Apr 23, 2023

By using the /DEPENDENTLOADFLAG link.exe flag, we can tell the Windows loader to only look for referenced DLLs in the System32 directory. This prevents DLL injection.

While this would not have prevented this month's CVE (see #84637), it could prevent DLL injection bugs from being introduced in native code.

Questions

Is there a better way to apply this flag in the CMake files? Ideally it would be the default and tests that don't work with it would opt out.

When I added this flag, I got this error message from LINK.exe:

LINK : fatal error LNK1268: inconsistent option 'DEPENDENTLOADFLAG:0x800' specified with /USEPROFILE but not with /GENPROFILE

How can this be worked around? Currently I have commented out applying PGO to make the build pass.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 23, 2023
@ghost
Copy link

ghost commented Apr 23, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

By using the /DEPENDENTLOADFLAG link.exe flag, we can tell the Windows loader to only look for referenced DLLs in the System32 directory. This prevents DLL injection.

While this would not have prevented this month's CVE (see #84637), it could prevent DLL injection bugs from being introduced in native code.

To demonstrate the effectiveness of this approach, this PR is split into 3 commits. The first adds a test for DLL injection. The second reverts the delay loading for version.dll introduced in dotnet/coreclr#24449. The this causes the test to fail. Lastly the /DEPENDENTLOADFLAG flag is added and the test starts passing again.

Author: AustinWise
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: -

@AustinWise AustinWise marked this pull request as draft April 23, 2023 20:22
@AustinWise
Copy link
Contributor Author

Leaving this as draft for now to see if it breaks anything.

@jkotas
Copy link
Member

jkotas commented Apr 23, 2023

/DEPENDENTLOADFLAG

The docs say that this flag is effective on newer Windows 10 RS1+ only. It does not hurt to add it, but I do not think we can depend on it as long as we support Windows Server 2012: https://github.com/dotnet/core/blob/main/release-notes/8.0/supported-os.md#windows. I would expect the tests to fail on Windows Server 2012.

@AustinWise
Copy link
Contributor Author

I finally got a chance to look at the failures. It appears that some tests (like for IJW) are impacted by this change. I think I will rework this to only target the main product DLLs and EXEs, like coreclr.dll and dotnet.exe.

/DEPENDENTLOADFLAG

The docs say that this flag is effective on newer Windows 10 RS1+ only. It does not hurt to add it, but I do not think we can depend on it as long as we support Windows Server 2012: https://github.com/dotnet/core/blob/main/release-notes/8.0/supported-os.md#windows. I would expect the tests to fail on Windows Server 2012.

Opps, I did not check the minimum server version of Windows. In that case I will revert the changes to delay loading version.dll and let this be a defense-in-depth change before I mark this ready for review.

@AustinWise
Copy link
Contributor Author

AustinWise commented Apr 30, 2023

I removed the test because it was pretty hacky. I think the ideal test would enumerate all loaded modules and make sure this flag is set in non-operating-systems module. I'm not 100% sure on how to write such a test.

In the absence of tests, I manually verified that the follow executable have the dependent load flag using LINK.EXE /DUMP /HEADERS /LOADCONFIG file.dll:

  • coreclr.dll
  • clrjit.dll
  • createdump.exe
  • apphost.exe
  • dotnet.exe
  • singlefilehost.exe
  • comhost.dll
  • hostfxr.dll
  • hostpolicy.dll
  • ijwhost.dll
  • nethost.dll

Which I think covers the main product executables.

@AustinWise
Copy link
Contributor Author

I wrote a program to check for files missing this flag:

https://github.com/AustinWise/CheckDependentLoadFlags

I'm satisfied that I've covered the main product DLLs, so I will open this for review.

@AustinWise AustinWise marked this pull request as ready for review April 30, 2023 23:04
@jkotas jkotas requested a review from elinor-fung May 5, 2023 05:53
@elinor-fung
Copy link
Member

cc @blowdart @GrabYourPitchforks

@AustinWise AustinWise marked this pull request as draft May 7, 2023 01:46
@AustinWise
Copy link
Contributor Author

I identified some further revision I'd like to do, so I'm marking this as draft again. Sorry for the churn.

@ghost ghost closed this Jun 6, 2023
@ghost
Copy link

ghost commented Jun 6, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 6, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants