Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix vunerablity to DLL Search Order Hijacking by delay loading version.dll #1838

Closed

Conversation

AustinWise
Copy link
Contributor

Hypothetically if the user's current directory contains a malicious DLL
named version.dll, this DLL will be loaded instead of the one in System32.

I checked the list of DLLs referenced by rufus.exe against the KnownDLLs
list visible in Sysinternal's winobj.exe. On both Windows 7 and Windows 11,
version.dll was the only DLL not on the KnownDLLs list and thus vunerable
to this attack.

To confirm that this work, I used dumpbin.exe /IMPORTS to make sure
version.dll is delay loaded. I then put a breakpoint in the delay load hook
and confirmed that the hook is used. This can be triggered by loading a
Windows installation ISO file.

I also removed kernel32_path, since kernel32.dll is on the KnownDLLs
list and is not vulnerable to this attack.

For information about KnownDLLs, see this article.
For information about delay-load DLL hooks, see this article

…n.dll

Hypothetically if the user's current directory contains a malicious DLL
named version.dll, this DLL will be loaded instead of the one in System32.

I checked the list of DLLs referenced by rufus.exe against the KnownDLLs
list visible in Sysinternal's winobj.exe. On both Windows 7 and Windows 11,
version.dll was the only DLL not on the KnownDLLs list and thus vunerable
to this attack.

To confirm that this work, I used dumpbin.exe /IMPORTS to make sure
version.dll is delay loaded. I then put a breakpoint in the delay load hook
and confirmed that the hook is used. This can be triggered by loading a
Windows installation ISO file.
@AustinWise
Copy link
Contributor Author

This implementation was inspire by dotnet/coreclr@c64a137

@pbatard
Copy link
Owner

pbatard commented Dec 12, 2021

Thanks for the PR.

As you've seen, I've been trying to mitigate these sideloading vulnerabilities, but Microsoft makes it quite difficult to plug them all, especially when linking libraries during compilation (a process which you'd think Microsoft would have hardened by default against side-loading but which they don't).

And as you can also see, trying to service both MSVC and MinGW compilation is tricky, because things that apply to MSVC, such as <DelayLoadDLLs> don't port too well for MinGW (and you should bear in mind that the main executable I release is the MinGW built one rather than the MSVC built one). So far, I'm not seeing a working equivalent of <DelayLoadDLLs> for MinGW, so I fear that the solution will be not not link with version.lib at all, but hook into the DLL at runtime... But if you have other suggestions, I'll take them.

@pbatard pbatard self-requested a review December 12, 2021 18:29
@pbatard pbatard self-assigned this Dec 12, 2021
I'm not sure this is actually the correct thing to do. Why is there an
extra const?
@AustinWise
Copy link
Contributor Author

Oh, that's too bad. A 10 year old stack overflow post says there is no GCC equivalent, but I'll spend some time next weekend digging into GCC and ld to see if it has grown support in the intervening years.

@pbatard
Copy link
Owner

pbatard commented Dec 14, 2021

Yeah, I saw the same stackoverflow entry, which is why I fear the only solution that'll apply to both compilers is not to link with version.lib, but hook into the relevant version.dll calls at runtime, after we have set the DLL lookup directories and removed current dir.

@AustinWise
Copy link
Contributor Author

I did figure out a way in MinGW to create a delay-load import library for an existing DLL. See this repo for the complete example:

https://github.com/AustinWise/delay-load-mingw

I'm thinking that the effort involved in maintaining the MSVC compiler flags and generating a custom import library for GCC is more effort than it is worth. I think it is simpler to just LoadLibrary version.dll to call the three functions that Rufus uses from it.

@pbatard
Copy link
Owner

pbatard commented Dec 25, 2021

I'm thinking that the effort involved in maintaining the MSVC compiler flags and generating a custom import library for GCC is more effort than it is worth. I think it is simpler to just LoadLibrary version.dll to call the three functions that Rufus uses from it.

Yes, I agree. I'm planning to take care of that when I have some time. But I'm also still planning to apply your delayed loading proposal for MSVC, as it's definitely something worth to have there.

@pbatard
Copy link
Owner

pbatard commented Jan 5, 2022

Sorry for the delay. I have now altered Rufus so that we manually load the calls we need from version.dll, which should take care of the issue for both MinGW and MSVC. And I have also applied your patch and made sure we set all the libraries we reference as delay loaded in the MSVC project settings. This is most likely and overkill, but since your patch allows us to delay-load, and it shouldn't hurt, we might as well use that feature where we can.

Thanks again for your contribution!

pbatard added a commit that referenced this pull request Jan 5, 2022
* This is part of #1838, where we need to sort the version.dll sideloading problem for MinGW.
* A subsequent patch will be applied to MSVC, to more generally delay the loading of DLLs.
* Also fix a typo with an assert expression.
@pbatard pbatard closed this in ef2ff71 Jan 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2023
@AustinWise AustinWise deleted the austin/DelayLoadVersionDll branch February 17, 2023 03:08
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants