1983320 - RHEL needs support for ml-dsa

Status: REOPENED

(NSS :: Libraries, enhancement, P5)

Description

[Robert Relyea]

RHEL needs support for ml-dsa in

1. freebl (2 patches)
2. softoken
3. cryptohi and certificates
4. ssl
5. cert/ssl tests
6. pk11_gtests
7. ssl_gtests

The changes in cryptohi and certificates should be straight up (always there).

softoken changes and freebl changes should be compile time.
The freebl implementation may be just a place holder (rhel is currently using leancrypto) until the preferred implementation (presumably libcrux) is available in a C interface. What ever library we have we need support for 1) capturing the seed on key gen. 2) using the seed to regenerate keys, 3) a streaming interface, 3) support for ml-dsa signing contexts. Both DETERMINISTIC and HEDGE support (with preference for HEDGE).
We should also support mu signing and extracting a public key from an expanded private key that is missing the seed.
I'll probably supply multiple options for this preferred implemenation. The reviewer can decide which (or both, selected at compile time).

ssl support may be compile time, or they can be just turned off by the lack of the ML-DSA mechanism support (as advertised by softoken).

Comment 1

[Robert Relyea]

I will definately split the upstream patch into those described above. I may split the bug as well.

Comment 2

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Add support for ML-DSA key type and public key structure

• Added mldsaKey to the KeyType enum in keythi.h to represent
  ML-DSA keys.
• Defined a new structure SECKEYMLDSAPublicKey to represent ML-DSA
  public keys, including:
  • paramSet (CK_ML_DSA_PARAMETER_SET_TYPE): the chosen ML-DSA
    parameter set
  • size: placeholder for key size
  • publicValue: the actual public key bytes
• Added a mldsa union field in the SECKEYPublicKey struct to store
  ML-DSA keys.
  • Updated SECKEY_CopyPublicKey() to support deep copying of ML-DSA
    public keys.
  • Implemented SECKEY_ConvertToPublicKey() path for mldsaKey,
    enabling conversion
    from private key to public key using PKCS#11 attributes
    (CKA_VALUE, CKA_PARAMETER_SET).
  • Extended SECKEY_PublicKeyStrengthInBits() to return estimated
    security strength via the new SECKEY_MLDSAParamsToKeySize().
• Implemented SECKEY_GetMLDSATagByParamSet() to map ML-DSA parameter
  sets to their corresponding SECOidTag.
• Added seckey_GetMLDSAParamSetByTag() (internal) to perform the
  reverse mapping.
• Extended seckey_ExtractPublicKey() to recognize ML-DSA OIDs and
  parse SubjectPublicKeyInfo into a valid SECKEYPublicKey object with
  type mldsaKey.
• Updated seckey_CreateSubjectPublicKeyInfo_helper() to support
  encoding ML-DSA public keys into SPKI structures, including correct OID
  selection and bit length handling.

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nicola Tuveri <[e-mail]>
Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 3

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Add support for ML-DSA keys and mechanisms in PKCS#11 interface

• Extend PK11_ImportPublicKey() and PK11_ExtractPublicKey() to
  handle ML-DSA public key attributes (CKA_VALUE, CKA_PARAMETER_SET).
• Add mlDsaPubTemplate for ML-DSA public key attributes.
• Add CKA_SEED and CKA_PARAMETER_SET in the private key template
  when loading a private key object.
• Include CKM_ML_DSA_KEY_PAIR_GEN mechanism handling in
  PK11_GenerateKeyPairWithOpFlags.
• Extend PK11_IsUserCert() to recognize ML-DSA keys.
• Map CKK_ML_DSA to CKM_ML_DSA in PK11_GetKeyMechanism and
  viceversa in PK11_GetKeyType.
• Map CKM_ML_DSA to CKM_ML_DSA_KEYGENinPK11_GetKeyGenWithSize`.
• Assign proper CK_ATTRIBUTE_TYPE usage flags (CKA_SIGN) for ML-DSA
  private keys
• Add "ML-DSA" to PK11_DefaultArray[] and define
  pk11_mldsaSlotList.
• Register and clean up pk11_mldsaSlotList during slot list
  init/destroy.
• Prevent ML-DSA from being used in unsupported derivation functions
  (PK11_PubDerive).
• Ensure assertion and error handling paths are ML-DSA aware.

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 4

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Enable ML-DSA signature verification for certificate chain validation

1. nss/mozpkix:

• Added MLDSA to the PublicKeyAlgorithm enum in pkixder.h.
• Added DigestAlgorithm::no_digest to support signature schemes
  without pre-hashing.
• Implemented VerifyMLDSASignedDataNSS for verifying raw ML-DSA
  signatures.
• Extended the TrustDomain interface with a virtual method
  VerifyMLDSASignedData().
• Integrated ML-DSA support into:
  • pkixverify.cpp (verification dispatch)
  • pkixcheck.cpp (OID recognition and public key checks)
  • pkixder.cpp (signature algorithm decoding)
  • pkixnss.cpp (signature mechanism handling)
  • pkixutil.h (digest size mapping)
• Defined and matched the ML-DSA OIDs.
• Enforced the DER encoding rule that ML-DSA AlgorithmIdentifier must
  omit parameters.
• Ensured DigestBufNSS and existing verifying methods reject
  no_digest where unsupported.
• Updated test utilities to gracefully handle the new no_digest
  case.

2. certverifier:

• Integrated ML-DSA support into CertVerifier by extending
  VerifySignedDataWithCache to handle the MLDSA public key
  algorithm.
• Implemented VerifyMLDSASignedData in NSSCertDBTrustDomain to
  verify ML-DSA signatures.
• Updated NSSCertDBTrustDomain.h to declare the new
  VerifyMLDSASignedData method.
• Handled DigestAlgorithm::no_digest in signature algorithm checks.

3. ct/gtest:

• Added a stub implementation of VerifyMLDSASignedData in
  CTLogVerifier.cpp’s SignatureParamsTrustDomain that currently
  returns
  a fatal error, indicating ML-DSA is not supported yet in this
  context.
• Extended the test trust domain in CTTestUtils.cpp to support
  ML-DSA
  signature verification by implementing VerifyMLDSASignedData using
  the
  existing NSS verification function.
• Thiese changes prepare the CT codebase for future ML-DSA integration
  by adding
  necessary interfaces and enabling test coverage for ML-DSA signature
  verification.

4. netwerk/http:

• Added VerifyMLDSASignedData method to
  ServerCertHashesTrustDomain
  as a stub.
• The method currently asserts unreachable and returns a fatal error,
  indicating ML-DSA verification is not supported yet in this
  verifier.
• These changes prepare groundwork for future ML-DSA signature support
  in
  WebTransport HTTP certificate verification.

5. manager/ssl:

• Added VerifyMLDSASignedData method to AppTrustDomain for ML-DSA
  signature verification.
• Updated AppTrustDomain.h to declare the new
  VerifyMLDSASignedData
  override.
• Implemented a no-op VerifyMLDSASignedData in
  ClientAuthCertNonverifyingTrustDomain.
• Handle DigestAlgorithm::no_digest in
  CheckSignatureDigestAlgorithm.

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 5

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Add support for ML-DSA signature scheme in the SSL stack

1. nss/ssl:

• Added ssl_sig_mldsa* to the SSLSignatureScheme enum.
• Added a new ssl_auth_mldsa entry to the SSLAuthType enum.
• Registered ssl_sig_mldsa* in the defaultSignatureSchemes array
  to
  allow use during TLS negotiation.
• Extended ssl_SignatureSchemeToAuthType() and
  ssl_IsSupportedSignatureScheme() to handle ML-DSA correctly.
• Mapped ssl_auth_mldsa to CKM_ML_DSA in the auth_alg_defs[]
  array.
• Updated ssl_SignatureSchemeMatchesSpkiOid() to always return true
  for ML-DSA (SPKI OID matching is skipped).
• Updated ssl_VerifySignedHashesWithPubKey() to support ML-DSA raw
  hash handling.
• Modified tls13_AddContextToHashes() to handle the case where
  ssl_hash_none is specified.
• Extended ssl_SetAuthKeyBits() to support mldsaKey as a valid key
  type.

2. manager/ssl:

• Added "ML-DSA-*" to getSignatureName for user-readable signature
  scheme names.
• Included ssl_sig_mldsa* in enabled TLS signature schemes in
  nsNSSIOLayer.cpp.
• Added placeholder metrics code for ssl_auth_mldsa key size in
  handshake callbacks (marked as a TODO).

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nicola Tuveri <[e-mail]>
Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 6

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Add ML-DSA certificate support to certviewer

• Introduced MLDSAPublicKey class in pkijs.js for ML-DSA keys with
  schema, JSON (de)serialization, and length validation based on IETF
  draft spec.
• Added MLDSAPublicKey parsing and JWK export to PublicKeyInfo.
• Extended certDecoder.mjs to recognize and extract ML-DSA public key
  info.
• Updated certviewer.mjs to display ML-DSA public value and security
  level.
• Added ML-DSA OIDs and label to signature algorithms mapping.

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 7

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Add ML-DSA public key printing support in NSS command-line utilities

• Added SECU_PrintMLDSAPublicKey to output ML-DSA key size and public
  value.
• Integrated ML-DSA key printing case into
  secu_PrintSubjectPublicKeyInfo dispatcher.

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 8

[Francesco Rollo]

Attachment: Bug 1983320 - Enable ML-DSA integration via OIDs support and SECMOD flag r=rrelyea

Add SECMOD_MLDSA_FLAG for ML-DSA module support
Define pure ML-DSA public key and signature OIDs.
Extended SECOID tag enum with ML-DSA identifiers.

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nicola Tuveri <[e-mail]>
Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 9

[Francesco Rollo]

Attachment: Bug 1983320 - Add support for ML-DSA key type and public key structure

• Added mldsaKey to the KeyType enum in keythi.h to represent
  ML-DSA keys.
• Defined a new structure SECKEYMLDSAPublicKey to represent ML-DSA
  public keys, including:
  • paramSet (CK_ML_DSA_PARAMETER_SET_TYPE): the chosen ML-DSA
    parameter set
  • size: placeholder for key size
  • publicValue: the actual public key bytes
• Added a mldsa union field in the SECKEYPublicKey struct to store
  ML-DSA keys.
  • Updated SECKEY_CopyPublicKey() to support deep copying of ML-DSA
    public keys.
  • Implemented SECKEY_ConvertToPublicKey() path for mldsaKey,
    enabling conversion
    from private key to public key using PKCS#11 attributes
    (CKA_VALUE, CKA_PARAMETER_SET).
  • Extended SECKEY_PublicKeyStrengthInBits() to return estimated
    security strength via the new SECKEY_MLDSAParamsToKeySize().
• Implemented SECKEY_GetMLDSATagByParamSet() to map ML-DSA parameter
  sets to their corresponding SECOidTag.
• Added seckey_GetMLDSAParamSetByTag() (internal) to perform the
  reverse mapping.
• Extended seckey_ExtractPublicKey() to recognize ML-DSA OIDs and
  parse SubjectPublicKeyInfo into a valid SECKEYPublicKey object with
  type mldsaKey.
• Updated seckey_CreateSubjectPublicKeyInfo_helper() to support
  encoding ML-DSA public keys into SPKI structures, including correct OID
  selection and bit length handling.

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nicola Tuveri <[e-mail]>
Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 10

[Francesco Rollo]

Attachment: Bug 1983320 - Add support for ML-DSA keys and mechanisms in PKCS#11 interface

• Extend PK11_ImportPublicKey() and PK11_ExtractPublicKey() to
  handle ML-DSA public key attributes (CKA_VALUE, CKA_PARAMETER_SET).
• Add mlDsaPubTemplate for ML-DSA public key attributes.
• Add CKA_SEED and CKA_PARAMETER_SET in the private key template
  when loading a private key object.
• Include CKM_ML_DSA_KEY_PAIR_GEN mechanism handling in
  PK11_GenerateKeyPairWithOpFlags.
• Extend PK11_IsUserCert() to recognize ML-DSA keys.
• Map CKK_ML_DSA to CKM_ML_DSA in PK11_GetKeyMechanism and
  viceversa in PK11_GetKeyType.
• Map CKM_ML_DSA to CKM_ML_DSA_KEYGENinPK11_GetKeyGenWithSize`.
• Assign proper CK_ATTRIBUTE_TYPE usage flags (CKA_SIGN) for ML-DSA
  private keys
• Add "ML-DSA" to PK11_DefaultArray[] and define
  pk11_mldsaSlotList.
• Register and clean up pk11_mldsaSlotList during slot list
  init/destroy.
• Prevent ML-DSA from being used in unsupported derivation functions
  (PK11_PubDerive).
• Ensure assertion and error handling paths are ML-DSA aware.

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 11

[Francesco Rollo]

Attachment: Bug 1983320 - Integrate ML-DSA signature verification for pkix certificate chain validation

1. nss/mozpkix:

• Added MLDSA to the PublicKeyAlgorithm enum in pkixder.h.
• Added DigestAlgorithm::no_digest to support signature schemes
  without pre-hashing.
• Implemented VerifyMLDSASignedDataNSS for verifying raw ML-DSA
  signatures.
• Extended the TrustDomain interface with a virtual method
  VerifyMLDSASignedData().
• Integrated ML-DSA support into:
  • pkixverify.cpp (verification dispatch)
  • pkixcheck.cpp (OID recognition and public key checks)
  • pkixder.cpp (signature algorithm decoding)
  • pkixnss.cpp (signature mechanism handling)
  • pkixutil.h (digest size mapping)
• Defined and matched the ML-DSA OIDs.
• Enforced the DER encoding rule that ML-DSA AlgorithmIdentifier must
  omit parameters.
• Ensured DigestBufNSS and existing verifying methods reject
  no_digest where unsupported.
• Updated test utilities to gracefully handle the new no_digest
  case.

2. certverifier:

• Integrated ML-DSA support into CertVerifier by extending
  VerifySignedDataWithCache to handle the MLDSA public key
  algorithm.
• Implemented VerifyMLDSASignedData in NSSCertDBTrustDomain to
  verify ML-DSA signatures.
• Updated NSSCertDBTrustDomain.h to declare the new
  VerifyMLDSASignedData method.
• Handled DigestAlgorithm::no_digest in signature algorithm checks.

3. ct/gtest:

• Added a stub implementation of VerifyMLDSASignedData in
  CTLogVerifier.cpp’s SignatureParamsTrustDomain that currently
  returns
  a fatal error, indicating ML-DSA is not supported yet in this
  context.
• Extended the test trust domain in CTTestUtils.cpp to support
  ML-DSA
  signature verification by implementing VerifyMLDSASignedData using
  the
  existing NSS verification function.
• Thiese changes prepare the CT codebase for future ML-DSA integration
  by adding
  necessary interfaces and enabling test coverage for ML-DSA signature
  verification.

4. netwerk/http:

• Added VerifyMLDSASignedData method to
  ServerCertHashesTrustDomain
  as a stub.
• The method currently asserts unreachable and returns a fatal error,
  indicating ML-DSA verification is not supported yet in this
  verifier.
• These changes prepare groundwork for future ML-DSA signature support
  in
  WebTransport HTTP certificate verification.

5. manager/ssl:

• Added VerifyMLDSASignedData method to AppTrustDomain for ML-DSA
  signature verification.
• Updated AppTrustDomain.h to declare the new
  VerifyMLDSASignedData
  override.
• Implemented a no-op VerifyMLDSASignedData in
  ClientAuthCertNonverifyingTrustDomain.
• Handle DigestAlgorithm::no_digest in
  CheckSignatureDigestAlgorithm.

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 12

[Francesco Rollo]

Attachment: Bug 1983320 - Add support for ML-DSA signature scheme in the SSL stack

1. nss/ssl:

• Added ssl_sig_mldsa* to the SSLSignatureScheme enum.
• Added a new ssl_auth_mldsa entry to the SSLAuthType enum.
• Registered ssl_sig_mldsa* in the defaultSignatureSchemes array
  to
  allow use during TLS negotiation.
• Extended ssl_SignatureSchemeToAuthType() and
  ssl_IsSupportedSignatureScheme() to handle ML-DSA correctly.
• Mapped ssl_auth_mldsa to CKM_ML_DSA in the auth_alg_defs[]
  array.
• Updated ssl_SignatureSchemeMatchesSpkiOid() to always return true
  for ML-DSA (SPKI OID matching is skipped).
• Updated ssl_VerifySignedHashesWithPubKey() to support ML-DSA raw
  hash handling.
• Modified tls13_AddContextToHashes() to handle the case where
  ssl_hash_none is specified.
• Extended ssl_SetAuthKeyBits() to support mldsaKey as a valid key
  type.

2. manager/ssl:

• Added "ML-DSA-*" to getSignatureName for user-readable signature
  scheme names.
• Included ssl_sig_mldsa* in enabled TLS signature schemes in
  nsNSSIOLayer.cpp.
• Added placeholder metrics code for ssl_auth_mldsa key size in
  handshake callbacks (marked as a TODO).

Signed-off-by: Francesco Rollo <[e-mail]>

Co-authored-by: Nicola Tuveri <[e-mail]>
Co-authored-by: Nouman Ali Khan <[e-mail]>

Comment 13

[Francesco Rollo]

Attachment: WIP: Bug 1983320 - Add ML-DSA certificate support to certviewer

• Introduced MLDSAPublicKey class in pkijs.js for ML-DSA keys with
  schema, JSON (de)serialization, and length validation based on IETF
  draft spec.
• Added MLDSAPublicKey parsing and JWK export to PublicKeyInfo.
• Extended certDecoder.mjs to recognize and extract ML-DSA public key
  info.
• Updated certviewer.mjs to display ML-DSA public value and security
  level.
• Added ML-DSA OIDs and label to signature algorithms mapping.

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 14

[Francesco Rollo]

Attachment: Bug 1983320 - Add ML-DSA public key printing support in NSS command-line utilities

• Added SECU_PrintMLDSAPublicKey to output ML-DSA key size and public
  value.
• Integrated ML-DSA key printing case into
  secu_PrintSubjectPublicKeyInfo dispatcher.

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 15

[Nicola Tuveri]

@rrelyea we had a few issues with the initial submission, but Francesco should have fixed everything now.
Please forgive the noise on the issue for the first set of commits.

We eagerly await your feedback, as well as that of @jschanck (I manually added him on the CC list as requested) and @beurdouche

Comment 16

[John Schanck [:jschanck]]

Thanks Nicola! I'm eager to take a look at this. Some of these patches (e.g. https://phabricator.services.mozilla.com/D262396) directly modify the security/nss directory of Firefox. We'll need you to split those parts out into patches on the NSS repo (https://hg.mozilla.org/projects/nss) instead. We could technically start reviewing as-is. But I think it's best to get this sorted out now so that we don't end up losing review history when we split the patches later.

Comment 17

[Robert Relyea]

Sigh, I will probably want to reject the main NSS patches. I already have patches for those that we have downstream, but I want to rework things to be more 'clean', There are lots of places where we have duplicate code that needs new functions, which is why I assigned the bug to myself.

I'll leave the mozpkix, psm and firefox patches, though. I think they are necessary and I don't have any equivalant for them. Hopefully they don't have to do deep into the datatstructures so they should work with my set of patches.

I definately would prefer my ssl patches. I looked at yours when I created my upstream patch and it's a pretty tight hack.

I'll go look at the nss specific patches now. I might be they are close enough I can clean things up later, but my guess is they are missing things like pkcs12 support, etc. Since the standards for that was only finished last month and they weren't necessary for your project.

bob

Comment 18

[Robert Relyea]

Ok, I've reviewed all the patches. The mozpkix D262395 one looks fine to me, but should get another reviewer. The psm (certviewer) patch also D262397 looks fine, but again should get another reviewer.
The closest to being ready is the oid patch: D262392 We need to have a single generic oid per parameter set here rather than one for signature and one for public key because there is only one oid value define. We can use #defines we can specify the oid differently depending on usage, but things won't be quite right if we try to have multiple entries in the table.

-The main issue with the ml-dsa bulk is that I'd like to make the paramset an oid at the NSS level with that change, I think we can finagle the rest. There are some bugs I've identified, which should be fixed. A lot of my comments are general cleanup I plan to do in any case.

-The main issue with the ssl code is they way signatures are handled. We should fix tls13con.c correctly. I think the big question is how to get the generic. My upstream patch uses secsign/secvfy code. The main thing here is to do sign/hash combo in tls13 since that's what all the new signing algorithms are doing, so we need something more general then the hack supplied in this code.

The downstream patch is https://gitlab.com/rrelyea/nss/-/blob/c10s/nss-3.112-add-ml-dsa-base.patch if you want to reference it. The patch is big, and will be broken down (and there are weirdness that I put in because oids and functions weren't public that you can ignore), but you can see sort of where I'm going.

The SSL downstream patch is https://gitlab.com/rrelyea/nss/-/blob/c10s/nss-3.112-add-ml-dsa-ssl-support.patch . The main differences between it and yours is 1) I need to support server side, so I had to make cert selection work, and 2) I make the tls13 signature generic so now PSS and ML-DSA are going through the same code. Again, you can see where I'm going from those.

Comment 19

[Francesco Rollo]

Hi Bob, sorry for the delayed response. I’ve reviewed your comments on the patches, but I’m still unclear about your intentions with some of them.
I’ve acknowledged your interest in mozpkix (D262395), psm (D262397), and oid (D262392) patches, which require some adjustments. Regarding the ssl and pk11-related changes, I see there are a few fixes needed, especially with the private key object. Looking at your downstream patches, several of these changes are already implemented (sometimes similarly, sometimes differently). I understand the direction you’re taking, and it seems that many of my changes either overlap with yours or are already handled better downstream.

You left comments on those as well, but I’d like to know more about which of these changes you are considering introducing upstream. This will help me refine my patches and drop anything that’s unnecessary.
Thanks.

Comment 20

[Robert Relyea]

So to be clear, the upstream changes have already been completed. The patches I pointed you to are the full upstream changes. They are not completely appropriate downstream because I wanted to limit API and ABI changes to make sure things are correct. I'm about to drop some ml-dsa "prep" patches with add APIs that will make adding ml-dsa signatures easier, though I don't think these will colide as you only added signatures to moz-pkix code, not to the generic NSS sign/verify interface (where most cert signature processing is done). Also my patches include softoken and freebl changes which you don't have, so they won't collide (since you are using an external PKCS #11 module to get your ML-DSA code). Finally I have a ton of ml-dsa tests, which also doesn't collide.

Do the areas where we collide is some mid-level key support, and there I think the only change I would ask for is

1. to keep the paramset in a variable called 'paramSet' which is an OID.
2. make sure you utility function to map OID->PKCS#11 Paramset and PKCS#11 Paramset->OID private. I think long term I want to put those in util so I can call them from softoken. (I think this is already true).
3. Define single OIDs with generic name (preferably "SEC_OID_ML_DSA_xx"). I'm fine with creating #defines for the KEY and SIG values you have. It's good documentation for how they are used in the relevant code. I'm ok with other names, I purposefully did not make those name public in my upstream patch because I didn't want to get locked into a name and a value because my oids weren't downstream yet. I would prefer the string for these be "ML-DSA-xx" as that does appear in some code (and it's a good generic description when outputted by various tools).

I we can focus on these three changes, we can start to get some traction. The can also go in before others. I'd really like to see the oid patch fixed and checked in, since a lot of my changes (like changes to policy code) requires the oids.

SSL is a bit of a mess. The issue is ml-dsa isn't a 'sign a hash' function, but sign data. The old SSL (SSL3/TLS1.0-2) protocol assumed hash and signature algorithms were orthogonal. Thank and given some protocal oddities (like signing a concatenation of SHA1 and MD5) lead to ssl3_SignHashesWithPrivKey() interface for in ssl3. The new algorithms don't sign hashes, so this function is not relevant to them. TLS 1.3 is now defined to do a full signature on the handshake hashes, but the current implentation generates those signatures by hashing and calling ssl3_SignHashesWithPrivKey(). Instead it should call a common hashAndSign function. Your hack sort of does it, by creating a fake hash that just buffers and signs the whole thing. I prefer actually changing the tls1.3 code to instead of hash then sign, do the hashAndSign in it's own step. You can see how it's implemented in the upstream patch.

What you did was fine for the demonstration, but I don't think we want to maintain that going forward (ssl3_SignHashesWithPrivKey already has enough hacks in it). ml-dsa is far from the last time we need to deal with this issue (and in fact we have reasons to go to hashAndSign in the future anyway).

Probably the best way forward is for me to supply the hashAndSign patch as a 'prep' patch without ml-dsa support, and then add ml-dsa to that.

So Let's go with first getting the OID's ready first and then look at the other non-SSL patches with the paramSet parameter == OID. I'll continue to get my prep patches and I'll try to get the ssl-prep patch ready as well.

I'll also get my softoken and PKCS #12 patches ready. Hopefully Franzeska will have the libcrux code ready as well and then I can add my test code.

Schedule wise, I'm leaving for vacation Sept 17 until the 1st week of October, so the more we get done while I'm here, the better.

Comment 21

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: Prep Create a FindOidTagByString function

Prep - create a FindOidTagBy description function and replace all the one off in other libraries and tools in NSS.

Most uses are tooking for crypto algorithms, so in that case we'll filter by the existance of a PKCS #11 mechanism. The function can also take an optional len, so we do a strncmp. This allows comparing a substring .

Comment 22

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: Prep Create a CreateSignatureAlgorithmID function

Create two new public functions:

SEC_CreateSignatureAlgorithmID

Does the full SignatureAlgorithmID creation using the the key and some optional parameters. It replaces code that called SEC_GetSignatureAlgorithmOidTag() and
SEC_SetSignatureAlgorithmID(). The advantage of this function is it can automatically format any parameters needed in the SignatureID. While it's mostly for Signing (and thus using the private key), it can be used with the public key to create a SignAlg for verification.

SEC_GetSignatureAlgorithmOidTagByKey

This replaces SEC_GetSignatureAlgorithmOidTag(). The previous function only took the key type. For ml-dsa and future signing algorithms, we'll need the full key to determine the OidTag.

Also update our tests and internal uses to use these new functions. The existing tests/cert/cert.sh and tests/tools/tools.sh should be sufficient to test these new function.

Comment 23

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: Prep colapse the overuse of keyType outside of pk11wrap and cryptohi

overall: colapse several repeated use of keyType uses.

certutil.c, sign.c, keyhi.h, seckey.c: move the the string function to seckey (and make it safe for future versions).

crmfcont.c, pk11akey.c, p12d.c, pk11pub.h, make the public function PK11_GetPublicValueFromPublicKey to handle all the cases that needs to get the public value from the key.

several, propagate const for public_key caused by the above fix (all these cases were const all along, but not declared as such).

crmfcont.c, pk11akey.c pk11obj.c, pk11pub.h Add PK11_UnwrapPrivKeyByKeyType which find the PKCS #11 attributes by keyType and KeyUsage.

pk11mech.c, pk11skey.c secmodi.h The above uses existing pk11_mapXXXKeyType functions, regularize their names (private ones with lower case and public ones with upper case) and add the missing one (pk11_mapDeriveKeyType).

pk11skey.c Default the non derive keys to fail in PK11_PubDerive and PK11_PubDeriveWithKDF.

Comment 24

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: move tls 1.3 to use streaming signatures

move tls 1.3 to use streaming signatures rather than hash and sign.

Motivations

1. All the new signing algorithms use signatures that don't separate the signature portion from the hash portion. There are also cases where we want to use the combined hash-signature interfaces for our traditional signatures (some tokens only support the hash-signature interface).

2. the ssl_SignHashesWithPrivKey carry a lot of ssl3 and tls1.x x<=2 specific semantics that are no longer applies to tls1.3. TLS 1.3 redefined their signing algortithms so we we a full signature over the handshake hashes specifically to allow signing of the full tls hashes with streaming signatures.

3. secsign and secvfy already have the ability to determine if we need to do combined hash & sign or separate hash and sign.

This patch uses the SecAlgorithmID of the secsign and secvfy interfaces to do hash and sign. The use of SecAlgorithmID is useful in may other interfaces because the SecAlgorithmID is usually then sent directly to ASN.1 encoder, but that step is not necessary for SSL. I would be fine creatine new special SGN_NewContext and VFY_NewContext which just takes AlgTags and a PKCS #11 mechanism params. The latter is necessary for RSA PSS (there is already and interface that takes encoded params, but that's just one step closer to using full SECAlgorithmIDs).

The primary new tls functions are the tlsSignOrVerify interface, which can either sign or verify depending on the context. Using a combined interace allows us to keep the tls context encoding in a single common function and calling tls_SignOrVerifyUpdate() instead of the hashUpdate function. So tls13_AddContextToHashes becomes tls13_SignOrVerifyHashWithContext which either creates or verifies the signature (depending if we are signing or verifying).

This patch depends on https://phabricator.services.mozilla.com/D263657, but if we add new interfaces for SGN_NewContext and VFY_CreateContext that tke PKCS #11 mechanism parameters, that dependency can be dropped

Comment 25

[Robert Relyea]

Francesco, if you can get the OID patch completed and update by Monday end of that, that will help. I only have one more day that this patch is sort of key to getting everything else going.
Thanks, bob

Comment 26

[Francesco Rollo]

Hi Bob, can you please have a look at the latest comment I left in D262393? I'm almost done, I just need clarifications on the helper function that returns the key sizes. Thanks.

Comment 27

[Robert Relyea]

Answered. Thanks for pushing these forward!

Comment 28

[Francesco Rollo]

Hi Bob, it seems there aren’t any notifications for the revisions discussion. I added a comment two days ago and I’m having trouble pushing the work I’ve done. Could you please take a look at D262392? I'm having troubles moving from firefox to nss repo. Thank you.

Comment 29

[Francesco Rollo]

Moz-phab is still running and it's stuck on D262394. I removed the "size" param from the mldsa pubkey struct. You accepted D262398 but it's not ready yet. I also made changes there to drop the "size" print.

Comment 30

[Francesco Rollo]

Submitted all the changes now. I'm not sure what happened with moz-phab earlier.

Comment 31

[Francesco Rollo]

Attachment: Bug 1983320 - Enable ML-DSA signature verification for certificate chain validation

• Added MLDSA to the PublicKeyAlgorithm enum in pkixder.h.
• Added DigestAlgorithm::no_digest to support signature schemes without pre-hashing.
• Implemented VerifyMLDSASignedDataNSS for verifying raw ML-DSA signatures.
• Extended the TrustDomain interface with a virtual method VerifyMLDSASignedData().
• Integrated ML-DSA support into:
  • pkixverify.cpp (verification dispatch)
  • pkixcheck.cpp (OID recognition and public key checks)
  • pkixder.cpp (signature algorithm decoding)
  • pkixnss.cpp (signature mechanism handling)
  • pkixutil.h (digest size mapping)
• Defined and matched the ML-DSA OIDs.
• Enforced the DER encoding rule that ML-DSA AlgorithmIdentifier must omit parameters.
• Ensured DigestBufNSS and existing verifying methods reject no_digest where unsupported.
• Updated test utilities to gracefully handle the new no_digest

Signed-off-by: Francesco Rollo <[e-mail]>

Comment 32

[Robert Relyea]

I found it before I checked in. I do test builds and submit the patches to the try server before the get checked in. (we have clang formatting requirements, so almost every patch needs white space changes).

The check-in CI builds (automatically ran when we check in are here https://treeherder.mozilla.org/jobs?repo=nss&revision=e7f0b93a783fc2700ee1f73f112c50ab47f10caa and here https://treeherder.mozilla.org/jobs?repo=nss&revision=3be2525fb657d7e2d784248495ddf12bb9d34b3f

When I checked in, hg automatically picks up the version that was checked in an applies it to the phabricator link (the check-ins include a link to phabricator), it also automatically closed the phabricator patch and marked it as published.

Comment 33

[Francesco Rollo]

I’ve split D262395 into D264144. The mozpkix changes are now in a separate patch for NSS. Should I wait for it to land and for NSS to be refreshed in mozilla before removing them from D262395?

Comment 34

[Robert Relyea]

Looks good Francesco. If you are happy with the mozpkix patch, please switch the patch to 'needs review' (that Action .. button). I've added nss-reviews to the patch if it doesn't get picked up before I leave I'll randomly pick one of the nss mozilla people.

You'll need to update the original to remove the mozpkix code. I don't know if the firefox people will review it before the nss prereqs are uploaded, but I know you can't check in until it happens.

Comment 35

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: softoken changes

• add mldsa support to softoken.
• includes freebl api, but no freebl implementation. It provides a place for libcrux (or whatever implementation we need) to be grafted in.
• supports CKA_SEED and pkcs#8 key unwrap.
• code is "turned off" (not advertized in mechanisms list) until freebl implementation is available.

Comment 36

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: der key decode

This is needed to run our pk11 unittests in pk11_gtests.

Comment 37

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa: unittests

These are tests for testing basic ml-dsa signature functionality. it tests keyen and verify using NIST acvp vectors. These vectors are pulled using genNISTTestVectory.py, similar to the wycheproof script to pull their test vectors.

To run these tests you'll need the softoken patch and the encode/decode patch as well as an implementation for ml-dsa in freebl.

The hope is that can speed up getting an implementation in freebl since that implementation can use these tests vectors.

This patch can't be checked in until we have and implementation there because all these tests will fail without them.

Comment 38

[Robert Relyea]

Attachment: Bug 1983320 ML-DSA SGN and VFY interfaces.

1. update SGN_ and VFY_ interfaces to handle mldsa.
2. create a SECKEY_GetParameterSet to get the parameter set oid from the private key. This function should work for ml-kem, shl-dsa, and fn-dsa keys in the future.
3. update built-in cert processing code for ml-dsa.
4. add policy oids for ml-dsa.

Comment 39

[Robert Relyea]

Attachment: Bug 1983320 ML-DSA freebl sample implementation - not for checkin

This patch provides and implementation of ml-dsa so that our ml-dsa tests can run. While this is the current implementation used in RHEL, I don't intend to check this in. The real implementation is for libcrux. This patch is only for those experimenting with ml-dsa, to make sure all our ml-dsa test cases work.

I've now convinced myself that leancrypto is not a long term solution (if I hadn't already bought into libcrux). Leancrypto is written to to high a compiler standard and requires lots of tweaks to get it to compile on gcc4 and windows. Also it's not actually working on windows.

If you have linux or mac, you can use this patch to experiment with ML-DSA. With this patch (and the rest that I expect will land next week hopefully), all my ml-dsa tests will pass. The critical thing is that it verified that the unittests can pass with this patch so libcrux and use the unittests when it drops in the true ml-dsa implemenation.

Comment 40

[Robert Relyea]

Attachment: Bug 1983320 ML-DSA SGN and VFY interfaces.

1. update SGN_ and VFY_ interfaces to handle mldsa.
2. create a SECKEY_GetParameterSet to get the parameter set oid from the private key. This function should work for ml-kem, shl-dsa, and fn-dsa keys in the future.
3. update built-in cert processing code for ml-dsa.
4. add policy oids for ml-dsa.

Comment 41

[Robert Relyea]

Attachment: Bug 1983320 ML-DSA tests for Sign/Verify, certificates and pkcs12

These tests use certutil and pk12util to verify that we can generate and verify certificates using the cryptohi signing/verify interfaces. You'll need the main set of ml-dsa patches as well as a freebl interface for these to run, so the can't be checked in until all the rest has landed.

Comment 42

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa tls patch

General differences between this patch and downstream:

• I adopted the QuBIP code of using hash_none for the mldsa hash function rather than creating several different new hashes downstream.
• Upstream, I added code for getting the SID cache wrapping key when using multi-process SSL. It looks like we don't have a working cache on the server side for TLS 1.3, so I was not able to test the downstream code properly. I can add it once we have working TLS 1.3 cache (the code will work for anything that has a deterministic signature).
• The generic tls signature stream code is already a separate patch, so I only needed to add the mldsa algorithms.

General difference between this patch and the QuBIP patch:

• I have support for the server side cert selection. This requires 3 auth values for ml-dsa rather than one (one for each parameter set).
• The auth values were added after the tls3_any auth entry. This is because these values are visible to applications and shouldn't change (though tls_any may not actually be sent int the ssl_info structure).
• the ml-dsa scheme is subject to policy checks.
• I added a mechanism check so ml-dsa sig is not offered if a supporting token does not exist (we can remove this check when ml-dsa softokn and freebl are available).
• I added a check to prevent ml-dsa sig from being offered if TLS 1.3 isn't available. This check required sending a version max argument which filtered up to several other ssl3 functions.
• of course, I used tls13 stream signing rather than the 'buffer on hash_null hack in the QuBIP patch.

Comment 43

[Robert Relyea]

Attachment: Bug 1983320 ml-dsa tls tests

Add support for ml-dsa in the test tools and define both ssl and ssl_gtests for them.

Comment 44

[Robert Relyea]

Ok, we are down to two core patches, a bunch of tests and the QuBIP changes to firefox. Thanks John and Frantisek for timely reviews. I'll be away until the first full week of October, so feel free to review the remaining patches, but no urgency (though I'm fine if someone else does the final checkin while I'm away).

The main holdup is the libcrux freebl code. The downstream freebl placeholder won't work for us even if we wanted it as a stop gap. It doesn't support the Windows compiler, so the code now compiles on window, but does not work. Since we prefer code with formal proofs, we'll have to wait until we get libcrux. Once It's in we can check in the tests as well. The mk-dsa unittests test patch can verify the ml-dsa code so we just need the low level libcrux code an it's ml_dsa.c bindings to the existing freebl interface.

Maybe if I'm lucky it will be in before I get back:).

Comment 45

[Jan Grulich]

Hi, as Robert mentioned in the bug description, we need this in RHEL, in fact we need it sooner than later. I have tested the patches like ~3 weeks ago and it worked, but now I see things have changed quite a bit. While some patches are already merged, there are few remaining ones. Are these in a backportable state already with just minor things to be figured out or are there major changes expected?

Comment 46

[Francesco Rollo]

(In reply to Jan Grulich from comment #45)

Hi, as Robert mentioned in the bug description, we need this in RHEL, in fact we need it sooner than later. I have tested the patches like ~3 weeks ago and it worked, but now I see things have changed quite a bit. While some patches are already merged, there are few remaining ones. Are these in a backportable state already with just minor things to be figured out or are there major changes expected?

Hi Jan, I assume you’re referring to the mozpkix patch. There aren’t any major changes from the previous diff. I just refactored the signature verification function to remove the digest algorithm parameter, since it isn’t needed. This way, we also avoid having to define a no_digest value in the digest enum.

About the Firefox patch D262395, which depends on the mozpkix patch, I’m still waiting for review, but I assume your interest is on the NSS side of the work. Anyway, regarding certverifier patch (firefox) D262397, I still haven't been assigned a reviewer so I guess it's not the priority right now.

Comment 47

[Jan Grulich]

Hi Francesco, I have backported mozpkix patch, D262395 and D262397, but that is still not enough apparently. In the past when testing this I also had D262396, which I think is missing. What I'm trying to test is to run OpenSSL server with ML-DSA certificate/key and trying to connect to this server with Firefox. With those 3 patches it doesn't work and says it doesn't find suitable cypher. Is D262396 needed? From the review it looks this is not going to be accepted and proposed solution is D263840. Should I try to backport D263840 instead? Thank you for your help.

Comment 48

[Francesco Rollo]

(In reply to Jan Grulich from comment #47)

Hi Francesco, I have backported mozpkix patch, D262395 and D262397, but that is still not enough apparently. In the past when testing this I also had D262396, which I think is missing. What I'm trying to test is to run OpenSSL server with ML-DSA certificate/key and trying to connect to this server with Firefox. With those 3 patches it doesn't work and says it doesn't find suitable cypher. Is D262396 needed? From the review it looks this is not going to be accepted and proposed solution is D263840. Should I try to backport D263840 instead? Thank you for your help.

Exactly, D262396 is not going to be accepted, D263840 should be the right replacement.

Comment 49

[Jan Grulich]

(In reply to Francesco Rollo from comment #48)

(In reply to Jan Grulich from comment #47)

Hi Francesco, I have backported mozpkix patch, D262395 and D262397, but that is still not enough apparently. In the past when testing this I also had D262396, which I think is missing. What I'm trying to test is to run OpenSSL server with ML-DSA certificate/key and trying to connect to this server with Firefox. With those 3 patches it doesn't work and says it doesn't find suitable cypher. Is D262396 needed? From the review it looks this is not going to be accepted and proposed solution is D263840. Should I try to backport D263840 instead? Thank you for your help.

Exactly, D262396 is not going to be accepted, D263840 should be the right replacement.

Hmm, problem is that D263840 is for NSS, but Firefox in RHEL 10 is built against NSS we have in RHEL. Do we need this patch in NSS package instead? Also the ML-DSA support in RHEL NSS package is slightly different from the one proposed here so I wonder if it's even going to work out.

Comment 50

[Jan Grulich]

Hi Francesco, it looks we already have some version of this patch in RHEL 10, but it still doesn't make Firefox to work. It always ends up with SSL_ERROR_NO_CYPHER_OVERLAP error when trying to connect to OpenSSL server that I started with openssl s_server -cert localhost-mldsa.crt -key localhost-mldsa.key.

I have backported D262395, D262397 and D264144. The only change I have on top of these is https://gitlab.com/jgrulich/centos_rpms_firefox/-/blob/RHEL-90603/firefox-adapt-ml-dsa-support-to-rhel-nss.patch to make it build with RHEL NSS package. Do you have any suggestion what might be missing or do I have to wait for Robert to be back? Thank you for any help.

Comment 51

[Francesco Rollo]

(In reply to Jan Grulich from comment #50)

Hi Francesco, it looks we already have some version of this patch in RHEL 10, but it still doesn't make Firefox to work. It always ends up with SSL_ERROR_NO_CYPHER_OVERLAP error when trying to connect to OpenSSL server that I started with openssl s_server -cert localhost-mldsa.crt -key localhost-mldsa.key.

I have backported D262395, D262397 and D264144. The only change I have on top of these is https://gitlab.com/jgrulich/centos_rpms_firefox/-/blob/RHEL-90603/firefox-adapt-ml-dsa-support-to-rhel-nss.patch to make it build with RHEL NSS package. Do you have any suggestion what might be missing or do I have to wait for Robert to be back? Thank you for any help.

Sorry Jan for the delayed response. I don't know exactly which is your RHEL NSS package. Does it contain the two ml-dsa patches Robert pointed out in the very first comments? The SSL_ERROR_NO_CYPHER_OVERLAP error can occur for a variety of reasons during the handshake. Assuming there are no server-side issues, my first thought is that the ml-dsa ssl_sig scheme might not be enabled. But I assume your NSS package already includes the ml-dsa SSL patches, correct? If so, it has to be something else. It's hard to tell what is missing or where is the issue. If you can share how I can reproduce the same environment you’re running, I can have a closer look and probably spot the issue. To keep the thread clear, feel free to reach out to me via email.

Comment 52

[fkrenzel]

Attachment: Bug 1983320 - fix sourceTemplate.ulValueLen in pk11_map_trust_entry, r=rrelyea

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/1c91df164c80
ML-DSA SGN and VFY interfaces.

Comment 54

[Robert Relyea]

There are still open patches, reopenning (Pulsebot should check that before closing the bug).

Comment 55

[Phabricator Automation]

Comment on attachment 9518637 [details]
Bug 1983320 - fix sourceTemplate.ulValueLen in pk11_map_trust_entry, r=rrelyea

Revision D267765 was moved to bug 1965329. Setting attachment 9518637 [details] to obsolete.

Comment 56

[Robert Relyea]

John, Is there a bug for libcrux ml-dsa yet, is there an ETA for that libcrux ml-dsa? Thanks bob

Comment 57

[John Schanck [:jschanck]]

I haven't opened a bug for libcrux ml-dsa, but they are looking into generating C code for us now.

Comment 58

[Maurice Dauer [:mdauer]]

I just noticed that the scan-build job in CI has been failing since a815bf0c0ee69cee953b2675f361c8debc53f5da. The bitSize value in SECKEY_PrivateKeyStrengthInBits is never used (or returned) when privk->keyType == mldsaKey. Could this be unintended?

Comment 59

[Robert Relyea]

No that's a bug.. and the getsignature length as well.

Comment 60

[Robert Relyea]

Looks like GetSignatureLength was fixed already, so it's just SECKEY_PrivateKeyStrengthInBits. That makes sense since GetSignatureLength failures would cause certain signaure processing code to fail, but SECKEY_PrivateKeyStrengthInBits only affects the certain connection strength displays.

Comment 61

[Robert Relyea]

Attachment: Bug 1983320 Fix ml-dsa return value for SECKEY_PrivateKeyStrengthInBits.

bitSize is not being returned. For ml-dsa it's unnessary.
Also fixes a clang issue in the tip (I'll drop this out if someone else fixes the clang issue first.

Comment 62

[Robert Relyea]

Sigh, my comment wasn't that clear. bitSize variable is unnecessary, not the return value.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/a95bec408763
Fix ml-dsa return value for SECKEY_PrivateKeyStrengthInBits.

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/b2461f1d3814
Fix ml-dsa return value for SECKEY_PrivateKeyStrengthInBits.

Comment 65

[Maurice Dauer [:mdauer]]

Just FYI, you can add the leave-open keyword to prevent the bots from closing the bug.