-
Notifications
You must be signed in to change notification settings - Fork 5k
Support overriding MsQuic.dll in the application directory on Windows. #103351
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
Support overriding MsQuic.dll in the application directory on Windows. #103351
Conversation
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
Explicit probing for native .dll in application directory that is not there is going to introduce DLL planting vulnerability. You can read about it in https://msrc.microsoft.com/blog/2018/04/triaging-a-dll-planting-vulnerability/ . This particular case is "Malicious binary planted in an untrusted application directory.". You may want to mirror what we have done to enable loading app-local copies of lib ICU so that you do not have to deal with reports of dll planting vulnerabilities. App local lib ICU requires explicit opt-in via EDIT: I see @elinor-fung made the same comment as well. |
👍 |
Added opt-in via appctx switch + envvar (appctx switch takes precedence)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
Do you think it'd be possible to add a test for this? To prevent us from accidentally breaking it in the future.
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
It would be great to hook it up to our tests and run standard test suite on older windows. |
looks like Microsoft.Native.Quic.MsQuic.OpenSSL needs to be added to dotnet-public mirror first, https://dev.azure.com/dnceng/internal/_build/results?buildId=2475371&view=results |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
There don't seem to be any Windows 10 runs in the |
/azp run runtime-coreclr libraries-jitstress |
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
e2ebc71
to
8113199
Compare
/azp run runtime-extra-platforms |
Seems to work fine |
{ | ||
return true; | ||
} | ||
if (str == "1" || string.Equals(str, bool.FalseString, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be 0? we already test "1" above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LFTM in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just few nits. Thanks for this!
Closes #101200.
This PR allows (via some explicit user action) to use OpenSSL version of MsQuic to enable use of System.Net.Quic on Windows 10 and other OSes where the Schannel build of MsQuic is not supported.
To use OpenSSL build of MsQuic, you can use e.g. the following snippet in csproj.