Skip to content

Fix shimmed implementation of TryGetHashAndReset to handle HMAC. #111929

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

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

vcsjones
Copy link
Member

The TryGetHashAndReset in switches on the algorithm name of IncrementalHash. IncrementalHash prepends "HMAC" in front of the algorithm name, so the shim did not correctly handle the HMAC-prepended algorithm names.

Fixes #111926

The TryGetHashAndReset in switches on the algorithm name of IncrementalHash. IncrementalHash prepends "HMAC" in front of the algorithm name, so the shim did not correctly handle the HMAC-prepended algorithm names.
@vcsjones vcsjones added this to the 10.0.0 milestone Jan 28, 2025
@vcsjones vcsjones self-assigned this Jan 28, 2025
@vcsjones
Copy link
Member Author

We can't test this because we don't have any test platforms that test the netstandard2.0 implementation. The unit tests target $(NetCoreAppCurrent) and $(NetFrameworkMinimum). Both of those platforms don't fall in to using this shim.

I manually tested as such:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <Reference Include="Microsoft.Bcl.Cryptography" Aliases="MBC">
      <HintPath>/Users/vcsjones/Projects/runtime/artifacts/bin/Microsoft.Bcl.Cryptography/Debug/netstandard2.0/Microsoft.Bcl.Cryptography.dll</HintPath>
    </Reference>
    <Reference Include="System.Formats.Asn1">
      <HintPath>/Users/vcsjones/Projects/runtime/artifacts/bin/System.Formats.Asn1/Debug/netstandard2.0/System.Formats.Asn1.dll</HintPath>
    </Reference>
  </ItemGroup>
</Project>
extern alias MBC;

using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using X509CertificateLoader = MBC::System.Security.Cryptography.X509Certificates.X509CertificateLoader;

using ECDsa key1 = ECDsa.Create(ECCurve.NamedCurves.nistP256);
CertificateRequest req1 = new("CN=test1", key1, HashAlgorithmName.SHA256);
using X509Certificate2 cert1 = req1.CreateSelfSigned(DateTimeOffset.UtcNow, DateTimeOffset.UtcNow.AddYears(1));
byte[] content = cert1.Export(X509ContentType.Pkcs12, "test")!;
using X509Certificate2 cert = X509CertificateLoader.LoadPkcs12(content, "test");

After the fix, the error no longer reproduces and the PKCS12 doc opens fine.

@vcsjones vcsjones requested a review from bartonjs January 28, 2025 19:57
@vcsjones
Copy link
Member Author

@bartonjs do you want to consider back porting this one?

@bartonjs
Copy link
Member

On the one hand... we got this from a customer report. On the other, it's only reproducible on out-of-support versions... and that makes it hard to justify a backport.

@bartonjs
Copy link
Member

(Would Unity hit it? Some version of Xamarin that's still in support but old?)

@vcsjones
Copy link
Member Author

On the one hand... we got this from a customer report. On the other, it's only reproducible on out-of-support versions... and that makes it hard to justify a backport.

The one that always comes to mind for me is UWP. UWP is apparently supported and will pick up the netstandard2.0 implementation.

I guess the question I have comes down to: do we support the contract, or the things implementing the contract?

If it is the former, then yeah - we should consider backporting, because now or in the future anyone could pick it up. If the latter, then there is still a decent case for pack porting, as I (believe) UWP is one such supported runtime that will pick up the netstandard2.0 configuration.

@vcsjones vcsjones merged commit 02fd751 into dotnet:main Jan 28, 2025
82 of 85 checks passed
@vcsjones vcsjones deleted the fix-111926 branch January 28, 2025 22:59
@vcsjones
Copy link
Member Author

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/13060704161

Copy link
Contributor

@vcsjones backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix shimmed implementation of TryGetHashAndReset to handle HMAC.
Using index info to reconstruct a base tree...
M	src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/NetStandardShims.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/NetStandardShims.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/NetStandardShims.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix shimmed implementation of TryGetHashAndReset to handle HMAC.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@vcsjones
Copy link
Member Author

vcsjones commented Jan 30, 2025

Don't know why I did 8.0. Next thing you are going to tell me it's not 2023 anymore.

@vcsjones
Copy link
Member Author

/backport to release/9.0-staging

Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13061562762

@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X509CertificateLoader does not shim HMAC in .NET Standard 2.0 correctly
2 participants