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 #89311

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Jul 21, 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, like #87495 recently fixed.

This covers the main CoreCLR product DLLs and EXEs:

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

Questions

Is there a better way to apply this flag in the CMake files? Initially I tried adding this to all executables by defining the flag in eng/native/, but some tests broke because they rely on loading DLLs from places other than System32.

EDIT: I moved the setting to eng/native/configurecompiler.cmake and disabled the linker flag for IJW tests.

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.

EDIT: We can leave the application of PGO commented out until updated PGO files are available.

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

ghost commented Jul 21, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
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, like #87495 recently fixed.

This covers the main CoreCLR product DLLs and EXEs:

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

Questions

Is there a better way to apply this flag in the CMake files? Initially I tried adding this to all executables by defining the flag in eng/native/, but some tests broke because they rely on loading DLLs from places other than System32.

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.

Author: AustinWise
Assignees: -
Labels:

area-Host

Milestone: -

@vitek-karas
Copy link
Member

@elinor-fung as our inhouse DLL loading expert :-)

@elinor-fung
Copy link
Member

Is there a better way to apply this flag in the CMake files? Initially I tried adding this to all executables by defining the flag in eng/native/, but some tests broke because they rely on loading DLLs from places other than System32.

For the binaries under src/native/corehost, you could put the option under exe.cmake and lib.cmake, which will be used by product executables/libraries but not tests.

Do you recall which tests these were? If it is an isolated set, you could have them explicitly remove the option. We do something like that for IJW tests:

function(remove_ijw_incompatible_options options updatedOptions)

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.

I believe this is because the data files used for PGO optimization need to be collected/applied withbinaries that use identical options. One option would be to do this in two stages:

  • Temporarily add the flag only when instrumenting
  • Once we have new profile data with that flags, switch to adding that flag both when instrumenting and optimizing

@jkoritzinsky / @agocke do you know if there's a better way to do this?

@jkoritzinsky
Copy link
Member

Generally when we change options in an instrumenting-incompatible way, we just disable PGO optimization as part of the change until new data has been generated and flows back into the runtime. Your idea actually sounds nicer than our usual approach.

I think we should at least wait until after the snap (so wait until tomorrow) before taking a PGO-incompatible change just to simplify code flow.

@elinor-fung
Copy link
Member

I think we should at least wait until after the snap (so wait until tomorrow) before taking a PGO-incompatible change just to simplify code flow.

Agreed.

@AustinWise AustinWise marked this pull request as draft August 19, 2023 20:15
@AustinWise
Copy link
Contributor Author

Thanks for the pointer to the IJW file. I will try to do something like that for tests that break when this linker flag is applied globally. I'll un-draft this PR once it get everything building again.

@AustinWise AustinWise marked this pull request as ready for review August 20, 2023 15:07
@AustinWise
Copy link
Contributor Author

It looks like it was just the IJW tests in src\tests and src\installer\tests\ that were failing. By disabling the linker flags in IJW.cmake, now all tests are passing.

@elinor-fung
Copy link
Member

cc @GrabYourPitchforks @blowdart

@elinor-fung
Copy link
Member

This looks good to me.

I just want to make sure we are in a good position to get updated PGO data and re-enable PGO once this goes in.

@DrewScoggins - we haven't gotten new PGO data in over a month. It looks like the internal dotnet-optimization builds have been failing or timing out. Do you know if this is a known issue or something that still needs to be investigated?

@DrewScoggins
Copy link
Member

It is currently being investigated. The current issue I am working through is related a dependency of the repo using an insecure version of a dependency. I will keep things updated as we work through the issues.

@DrewScoggins
Copy link
Member

The scenario issue is fixed, but now we are seeing an internal runtime error when doing collection. I have created the issue below to track it.

#90962

@elinor-fung
Copy link
Member

Thanks @DrewScoggins. I see dotnet-optimization is back up and we are taking updates (#91171) again - yay!

@elinor-fung
Copy link
Member

Thanks, @AustinWise! Merging this.

@elinor-fung elinor-fung merged commit 9c3f8b3 into dotnet:main Aug 29, 2023
172 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Host 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.

5 participants