-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
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/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 |
Azure Pipelines successfully started running 1 pipeline(s). |
Looks like the reason it did not work in CI was that the dll did not make it in the zip file sent to helix. it should work better now. |
Good news, it works
Bad news, tests fail on Win 8.1, but we can just disable them |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
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.