Skip to content

Add MLDsaCng into Microsoft.Bcl.Cryptography #116678

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

PranavSenthilnathan
Copy link
Member

This PR adds MLDsaCng into Microsoft.Bcl.Cryptography but does not include certificate support. The general overview of this this PR is to split needed instance members on CngKey into a static part that takes a SafeNKeyCryptHandle and an instance part that calls into the static with the handle. This handle was generally the raw non-duplicated handle previously. However, since M.B.C. does not have access to this, it will call the static methods with a duplicated handle.

The static methods are in CngHelpers.cs - chosen because there were already some there prior to this PR. I tried to only move the necessary parts of CngHelpers.cs from S.S.C to Common, but the other option is just to move the whole file over. This is also simple (no additional dependencies need to be pulled in) and would preserve git history (by tracking it as a rename).

Contributes to #113502

@PranavSenthilnathan PranavSenthilnathan added this to the 10.0.0 milestone Jun 14, 2025
@PranavSenthilnathan PranavSenthilnathan self-assigned this Jun 14, 2025
@Copilot Copilot AI review requested due to automatic review settings June 14, 2025 21:53
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR integrates the ML-DSA CNG implementation (MLDsaCng) into Microsoft.Bcl.Cryptography, refactors CNG key APIs into shared static helpers, and updates tests to exercise the new code paths on Windows.

  • Split per-instance CngKey calls into static helpers under CngHelpers.cs and instance wrappers in CngExtensions.cs
  • Added secure buffer helper (PinAndClear.cs) and new algorithm/format extensions (CngIdentifierExtensions.cs)
  • Updated MLDsaImplementation and MLDsaCng code, plus corresponding Windows‐only tests

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/PinAndClear.cs New helper to pin and zero sensitive byte arrays
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/CngIdentifierExtensions.cs Extensions for new CNG algorithms and blob formats
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/CngHelpers.cs Added GetRandomBytes to core helpers
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/CngExtensions.cs Instance methods forwarding to static CngHelpers
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.CreateCng.cs Refactored ephemeral key creation to use callback‐based export
src/libraries/Common/src/System/Security/Cryptography/MLDsaCng.Windows.cs Windows‐specific MLDsaCng adjustments and import logic
src/libraries/Common/src/System/Security/Cryptography/CngHelpers.cs Consolidated static CNG interop helpers (Duplicate, import, sign/verify, etc.)
src/libraries/Common/src/System/Security/Cryptography/DSACng.SignVerify.cs Replaced CngCommon calls with CngHelpers equivalents
Comments suppressed due to low confidence (4)

src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/CngIdentifierExtensions.cs:6

  • [nitpick] File name CngIdentifierExtensions.cs does not match the classes defined within (CngAlgorithmExtensions, CngAlgorithmGroupExtensions, CngKeyBlobFormatExtensions). Rename the file to match one of the contained classes or adjust the class names to align with the file name.
internal static class CngAlgorithmExtensions

src/libraries/Common/src/System/Security/Cryptography/MLDsaCng.Windows.cs:378

  • [nitpick] Calling ToString on parameterSet (already a string) is redundant. You can interpolate the string variable directly for clarity.
${nameof(parameterSet)}: ${parameterSet.ToString()},

src/libraries/Common/src/System/Security/Cryptography/MLDsaCng.Windows.cs:276

  • Switching the ASN.1 decoder from BER to DER may break compatibility with BER-encoded inputs. Consider restoring AsnEncodingRules.BER or ensuring all inputs are strictly DER.
AsnDecoder.ReadEncodedValue(source, AsnEncodingRules.DER, out _, out _, out len);

src/libraries/Common/src/System/Security/Cryptography/MLDsaCng.Windows.cs:53

  • The property HandleNoDuplicate may not exist on CngKey, leading to a compilation error. Verify that the correct handle property is used or add HandleNoDuplicate in the API.
duplicateKey = CngHelpers.Duplicate(key.HandleNoDuplicate, key.IsEphemeral);

Comment on lines +341 to +356
ReadOnlySpan<byte> bytesToCopy = keyMaterial.AsSpan(0, length);
byte[] trimmedKeyMaterial = new byte[length];
PinAndClear ret = PinAndClear.Track(trimmedKeyMaterial);

try
{
bytesToCopy.CopyTo(trimmedKeyMaterial);
trimmed = trimmedKeyMaterial;
return ret;
}
catch
{
// This should never happen, but let's be safe and clean up the GC Handle if it does
ret.Dispose();
Debug.Fail("Copy failed.");
throw;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using PinAndClear to avoid the GC making a copy... but it's itself making a copy and not clearing the original.

There should be a call to some sort of Clear method after bytesToCopy.CopyTo(trimmedKeyMaterial), either in exceptionless code before the reassignment, or in a finally with a captured reference.

I don't think the finally is all that important, but do agree that it's goodness to free the PinAndClear in the event of a bizarro exception. I only mention a finally since you already have a try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array that's being copied from is owned by the caller of ImportPkcs8PrivateKey, so they are responsible for pinning and/or freeing it (CopyWithPrivateKey is the only caller right now and it does PinAndClear.Track before calling). I thought I added a comment about this somewhere but looks like I didn't.

MLDsaTestHelpers.ImportPublicKey(algorithm, source);

protected override void AssertExportPrivateDataFromPublicKey(Action export)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it "private data from public key", or "missing key element"? That is, does this helper apply when trying to export the seed from a secret-key import, or only for public-key imports?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It´s correct as is, but could be more specific. The private seed and secret key export will fail with a normal CryptographicException. However, for netfx, the PKCS8 export is implemented in Microsoft.Bcl.Cryptography, and since netfx doesn't have a settable HRESULT property on Exception, the custom exception is thrown instead. I've added comments and moved the assert to only the pkcs8 export case.

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.

3 participants