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

Support overriding MsQuic.dll in the application directory on Windows. #103351

Closed

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jun 12, 2024

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.

  <ItemGroup>
    <PackageReference Include="Microsoft.Native.Quic.MsQuic.OpenSSL"
                      Version="2.3.5"
                      PrivateAssets="all"
                      GeneratePathProperty="true" />

    <RuntimeHostConfigurationOption Include="System.Net.Quic.AppLocalMsQuic" Value="true" />
  </ItemGroup>

  <Target Name="CopyCustomContent" AfterTargets="AfterBuild">
    <Copy SourceFiles="$(PkgMicrosoft_Native_Quic_MsQuic_OpenSSL)\build\native\bin\x64\msquic.dll" DestinationFolder="$(OutDir)" />
  </Target>
  <Target Name="CopyCustomContentOnPublish" AfterTargets="Publish">
    <Copy SourceFiles="$(PkgMicrosoft_Native_Quic_MsQuic_OpenSSL)\build\native\bin\x64\msquic.dll" DestinationFolder="$(PublishDir)" />
  </Target>

@rzikm rzikm requested a review from a team June 12, 2024 16:25
@jkotas
Copy link
Member

jkotas commented Jun 12, 2024

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 System.Globalization.AppLocalIcu switch.

EDIT: I see @elinor-fung made the same comment as well.

@ManickaP
Copy link
Member

App local lib ICU requires explicit opt-in via System.Globalization.AppLocalIcu switch.

👍

@rzikm
Copy link
Member Author

rzikm commented Jun 14, 2024

Added opt-in via appctx switch + envvar (appctx switch takes precedence)

private static bool AllowAppLocalMsQuic() => AppContextSwitchHelper.GetBooleanConfig("System.Net.Quic.AppLocalMsQuic", "DOTNET_SYSTEM_NET_QUIC_APPLOCALMSQUIC");

@rzikm rzikm requested review from elinor-fung and jkotas June 14, 2024 08:52
Copy link
Member

@ManickaP ManickaP left a 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.

@wfurt
Copy link
Member

wfurt commented Jun 14, 2024

Do you think it'd be possible to add a test for this? To prevent us from accidentally breaking it in the future.

It would be great to hook it up to our tests and run standard test suite on older windows.

@rzikm
Copy link
Member Author

rzikm commented Jun 17, 2024

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

@rzikm
Copy link
Member Author

rzikm commented Jun 17, 2024

/azp run runtime

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Jun 18, 2024

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

There don't seem to be any Windows 10 runs in the runtime runtime-extra-platforms and runtime-libraries-coreclr outerloop pipelines, from a quick look at the yml definitions, there should be, unless I misunderstand something.

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

/azp run runtime-coreclr libraries-jitstress

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

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.

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

Good news, it works

C:\helix\work\workitem\e>"C:\helix\work\correlation\dotnet.exe" exec --runtimeconfig System.Net.Quic.Functional.Tests.runtimeconfig.json --depsfile System.Net.Quic.Functional.Tests.deps.json xunit.console.dll System.Net.Quic.Functional.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Net.Quic.Functional.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Quic.Functional.Tests (found 133 of 147 test cases)
  Starting:    System.Net.Quic.Functional.Tests (parallel test collections = on [2 threads], stop on fail = off)
MsQuic supported and using 'msquic.dll 2.3.5.465110 (933f7b79949bc588945672396d70b661143bb8f0)' (OpenSSL).
    System.Net.Quic.Tests.MsQuicPlatformDetectionTests.SupportedWindowsPlatforms_IsSupportedIsTrue [SKIP]
      Condition(s) not met: "SupportsTls13"
    System.Net.Quic.Tests.MsQuicPlatformDetectionTests.UnsupportedPlatforms_ThrowsPlatformNotSupportedException [SKIP]
      Condition(s) not met: "IsQuicUnsupported"
    System.Net.Quic.Tests.MsQuicTests.CloseAsync_ByServer_AcceptThrows [PASS]
      Output:

Bad news, tests fail on Win 8.1, but we can just disable them

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Windows 10 for System.Net.Quic
5 participants