Add ML-DSA-87. by juergw · Pull Request #1342 · google/conscrypt

juergw · 2025-06-02T16:28:15Z

Currently, the ML-DSA implementation only supports the default ML-DSA-65. This pull request now adds support for ML-DSA-87.

Note that the functions in native_crypto.cc for ML-DSA-87 are a copy of the functions for ML-DSA-65, but using different functions.

We add an enum class that is used to indicate which algorithm is used.

We also add validation of the key size to the private and public keys.

Also, add MlDsaAlgorithm enum, which we will need later.

miguelaranda0 · 2025-06-05T14:39:48Z

This is missing the license

Thanks. Done.

tholenst

I'm not too familiar with Conscrypt, so I still might miss important things.

tholenst · 2025-06-06T10:18:12Z

+    ScopedLocalRef<jbyteArray> publicKeyRef(
+            env, env->NewByteArray(static_cast<jsize>(MLDSA87_PUBLIC_KEY_BYTES)));
+    if (publicKeyRef.get() == nullptr) {
+        return nullptr;


Is there a reason that we don't have JNI_TRACE here? I'd expect to have that macro invoked whenever you return nullptr.

I added a JNI_TRACE.

Also, I investigated if we need to manually throw an exception here. It seems that this is not needed: when I set the number of bytes to the max value, i already get an out-of-memory exception.

tholenst · 2025-06-06T10:24:35Z

+    }
+
+    if (ARRAY_OFFSET_LENGTH_INVALID(dataArray, 0, dataLen)) {
+        conscrypt::jniutil::throwException(env, "java/lang/ArrayIndexOutOfBoundsException",


Hm, this I actually also fail to understand. Here we throw an exception, which makes sense. But then why don't we throw in other parts where the input is invalid (e.g. "MLSDSA87_parse_public_key failed" could be an IllegalArgumentException.

I agree that MLSDSA87_parse_public_key should probably also throw instead of just returing -1. I've changed it, and also changed the tests in NativeCryptoTest.

If I read the boringSSL code correctly, it seems that parsing only fails when the size is incorrect. And we already check that in Java, when we create the key class. So I think we don't end up in this case, and throwing will now change the behavior.

I've also did the same change for MLSDSA65_parse_public_key.

But note that when ScopedByteArray returns null, then it already calls jniThrowNullPointerException, so we don't have to do this ourselves.

tholenst · 2025-06-06T10:30:41Z

        }
        OpenSslMlDsaPrivateKey that = (OpenSslMlDsaPrivateKey) o;
-        return Arrays.equals(seed, that.seed);
+        return algorithm.equals(that.algorithm) && Arrays.equals(seed, that.seed);


Would it make sense to call BoringSSL for this here? Calling "Arrays.equals" is so obvious non-constant time it feels scary.

I agree, but I'd prefer to do this in a different pull request. Because this should also be changed in other places.

And also do this for MLDSA65.

tholenst · 2025-06-16T15:39:02Z

+    if (publicKeyArray.get() == nullptr) {
+        return nullptr;
+    }
+    memcpy(publicKeyArray.get(), public_key_bytes, MLDSA87_PUBLIC_KEY_BYTES);


Let's instead simply write directly into publicKeyArray.get(): move the construction of publicKeyArray before the call to MLDSA87_marshal_public_key, and initialize the CBB with publicKeyArray.get().

(Or is there a reason not to do that?)

You are right, that is both simpler and faster.

tholenst · 2025-06-16T15:39:25Z

+    if (resultArray.get() == nullptr) {
+        return nullptr;
+    }
+    memcpy(resultArray.get(), result, MLDSA87_SIGNATURE_BYTES);


Also here, we can avoid this memcpy

Done. Thanks.

tholenst · 2025-06-16T15:52:43Z

    private static final long serialVersionUID = 453861992373478445L;

    private final byte[] raw;
+    private final MlDsaAlgorithm algorithm;


Do we need to make this transient? I don't know if Conscrypt offers anything in terms of stability of the serialization.

If we change it, let's at least change the serialVersionUID to ensure that things properly fail.

This is a good question. I think it would be cumbersome to keep the same serialization as before. So I think we should just choose a new version ID.

I found a way to keep the serialization as it is. I'll do that, I think it's better to keep serialization working even if it's unlikely that anybody is using it.

tholenst · 2025-06-16T15:58:17Z

        putKeySize("XDH", 255);
        putKeySize("EdDSA", 255);
        putKeySize("ML-DSA", -1);
+        putKeySize("ML-DSA-44", -1);


I don't understand why we have this here; just for future cases?

It's needed if you run the test with a OpenJDK (version 24 and newer) that has ML-DSA-44 implemented.

tholenst · 2025-06-16T15:58:53Z

            byte[] signature = vector.getBytes("signature");

-            assertEquals(errMsg + ", algorithm:", "ML-DSA-65", algorithm);
+            if (!algorithm.equals("ML-DSA-65") && !algorithm.equals("ML-DSA-87")) {


Should we also have a test vector where this is just ML-DSA?

Because we need to add a new field to these keys, it would be cumbersome to keep the serialization exactly the same. And because the serialization changes, we need to change the serialVersionUID.

tholenst · 2025-06-23T14:55:02Z

+            oos.writeObject(privateKey);
+        }
+
+        String expectedHexEncoding = "aced000573720024"


Do we want to decode this more?

This would be "STREAM_MAGIC" (https://docs.oracle.com/en/java/javase/11/docs/specs/serialization/protocol.html#grammar-for-the-stream-format)

Actually I think this isn't necessary, but I do think we should add this test before rewriting the class. If it's part of the public API and we want to keep it, let's do it properly.

juergw added 11 commits June 2, 2025 15:58

Add ML-DSA-87 to NativeCrypto.

a78f02b

Also, add MlDsaAlgorithm enum, which we will need later.

Add more tests for ML-DSA-87.

8d7d145

Add ML-DSA-87.

a4daa54

Fix formatting.

2fa3714

Fix formating.

6ec1445

Fix ML-DSA-65 calls.

a023122

Use ExposedByteArrayOutputStream.

aef6a2a

Fix copy-paste error.

34c12db

Fix KeyPairGeneratorTest.

3c9f077

Fix SignatureTest.

ca7402b

Fix formatting.

e48bf07

juergw assigned tholenst Jun 3, 2025

miguelaranda0 reviewed Jun 5, 2025

View reviewed changes

Add missing license.

91e3e4c

tholenst reviewed Jun 6, 2025

View reviewed changes

juergw and others added 5 commits June 10, 2025 14:44

Throw an exception when MLDSA87_parse_public_key fails.

cfaee19

And also do this for MLDSA65.

Merge branch 'google:master' into master

43f6a79

Add JNI_TRACE call when MLDSA_sign couldn't allocate output.

a2cecfa

Merge branch 'master' of https://github.com/juergw/conscrypt

07bf66c

Fix whitespace.

53d44ba

juergw requested a review from tholenst June 13, 2025 09:47

tholenst reviewed Jun 16, 2025

View reviewed changes

juergw added 5 commits June 17, 2025 13:45

Avoid some memcopy.

7312418

Fix formatting, and one error message.

a292bdf

Change the serialVersionUID of the private and the public key.

2015f8d

Because we need to add a new field to these keys, it would be cumbersome to keep the serialization exactly the same. And because the serialization changes, we need to change the serialVersionUID.

Fix formatting.

d3567a3

Remove unused variable.

0b36486

tholenst approved these changes Jun 17, 2025

View reviewed changes

Add test vector for ML-DSA.

596895a

juergw requested a review from miguelaranda0 June 18, 2025 06:40

juergw marked this pull request as draft June 19, 2025 12:09

tholenst self-requested a review June 19, 2025 12:15

juergw added 2 commits June 23, 2025 11:49

Keep serialization of ML-DSA private and public key compatible.

865981c

Add missing imports.

1dab318

juergw marked this pull request as ready for review June 23, 2025 12:52

juergw and others added 3 commits June 25, 2025 12:44

Merge branch 'google:master' into master

645d1b0

Merge remote-tracking branch 'upstream/master'

1c4909c

Remove remaining conflict marks.

6d34ac4

tholenst approved these changes Jun 26, 2025

View reviewed changes

miguelaranda0 approved these changes Jun 26, 2025

View reviewed changes

Merge branch 'google:master' into master

2e9317a

miguelaranda0 merged commit 40b0b67 into google:master Jun 30, 2025
38 checks passed

Uh oh!

Conversation

juergw commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tholenst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juergw commented Jun 2, 2025 •

edited

Loading