ML-DSA Pure Cert Support#251
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
7b1ecb7 to
915636c
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
No new issues found in the changed files. ✅
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
No new issues found in the changed files. ✅
| int pemBufSz = 0; | ||
| int outBufSz = 0; | ||
|
|
||
| #ifdef WOLFSSL_SMALL_STACK |
There was a problem hiding this comment.
Is it ok to remove the WOLFSSL_SMALL_STACK code here?
There was a problem hiding this comment.
That was intentional, I opted to always heap-allocate the MLDSA key material due to size. Let me know if there's a better route to go for those.
| @@ -0,0 +1,65 @@ | |||
| /* clu_mldsa.h | |||
| * | |||
| * Copyright (C) 2006-2025 wolfSSL Inc. | |||
| iky6IRTUzkBy1fssv3Gr/jOsyN8J565NST3RpQ== | ||
| -----END CERTIFICATE----- | ||
| ``` | ||
| A self-signed certificate can also be generated with post-quantum ML-DSA (FIPS 204) key. `ml-dsa` and `dilithium` are interchangeable command aliases, and the level (`2`/`3`/`5`) selects ML-DSA-44/65/87. This needs wolfSSL built with `--enable-dilithium --enable-experimental`. |
There was a problem hiding this comment.
We will need to update enable-dilithium to enable-mldsa, but that can be in a separate PR.
| @@ -0,0 +1,1890 @@ | |||
| /* clu_mldsa.c | |||
| * | |||
| * Copyright (C) 2006-2025 wolfSSL Inc. | |||
| if (ret == NULL) { | ||
| /* every error path above sets conf=NULL after | ||
| * calling wolfCLU_CertSignFree */ | ||
| wolfSSL_NCONF_free(conf); |
There was a problem hiding this comment.
It looks like wolfCLU_CertSignFree(ret) is called on every error, and it does
if (csign->config != NULL) {
wolfSSL_NCONF_free(csign->config);
csign->config = NULL;
}
So this code should not be calling wolfSSL_NCONF_free(conf); at all here? Or it could risk a double free.
There was a problem hiding this comment.
Yes, that call was redundant. I removed it at the bottom.
| } | ||
|
|
||
| #ifdef WOLFCLU_MLDSA_CERTGEN | ||
| if (ret == WOLFCLU_SUCCESS && !isMLDSA && |
There was a problem hiding this comment.
Why is this guarded by !isMLDSA if this codeblock is for ML-DSA?
There was a problem hiding this comment.
The isMLDSA flag indicates that an existing ML-DSA key was passed in via the -key argument. The block is guarded with !isMLDSA so that if the user provides an existing key, a new key doesn't get generated.
| rngInited = 1; | ||
| } | ||
|
|
||
| /* Pick a random temp file path for throwaway key. */ |
There was a problem hiding this comment.
Why would there be temp files and temp keys for certificate generation?
There was a problem hiding this comment.
-newkey ml-dsa was writing the freshly generated key to a randomly-named temp file on disk just so it could immediately be read back into an MlDsaKey, then deleted, but it was easier to do in in memory, so I updated that
There was a problem hiding this comment.
preferably we shouldn't write key material to disk other than what the user requested. Even tmp files can have nasty surprises for sensitive material. E.g. the OS could unlink the file, but not actually zero the sensitive data.
There was a problem hiding this comment.
Good point, I'm working on updating that and will get it in on Monday. There were also a few other gaps in this pr.
| #include <wolfclu/pkey/clu_pkey.h> | ||
| #include <wolfclu/certgen/clu_certgen.h> | ||
|
|
||
| #ifndef WOLFCLU_NO_FILESYSTEM |
There was a problem hiding this comment.
Was this filesystem block necessary because you need to do file operations here and can't rely on wolfSSL wolfSSL_PEM_read_bio_PrivateKey (which doesn't support ML-DSA)?
There are probably other wolfssl or wolfcrypt functions we can use to avoid the need for file ops here.
There was a problem hiding this comment.
No, it was needed to support the above file operations.
|
Improved file I/O logic and key handoff to be in-memory, along with other changes as per reviewer's comments. These other changes were made to the ML-DSA implementation:
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 3
2 finding(s) posted as inline comments (see file-level comments below)
High (1)
Missing derBuf allocation before wc_MlDsaKey_PrivateKeyToDer in ML-DSA key generation
File: src/genkey/clu_genkey.c:1428
Function: wolfCLU_genKey_ML_DSA
Category: Copy-paste errors
derBuf is NULL (initialized at line 1343, never allocated before the switch) when wc_MlDsaKey_PrivateKeyToDer(key, derBuf, (word32)keySz) is called in PRIV_ONLY_FILE/PRIV_AND_PUB_FILES, so private-key DER export writes to a NULL pointer or returns an error and no key file is produced. The equivalent Dilithium path (lines 1153–1158) correctly calls XMALLOC(keySz, ...) before the export.
Recommendation: Add buffer allocation immediately before the wc_MlDsaKey_PrivateKeyToDer call, matching the Dilithium pattern:
derBuf = (byte*)XMALLOC(keySz, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
if (derBuf == NULL) {
ret = MEMORY_E;
break;
}Referenced code: src/genkey/clu_genkey.c:1428-1430 (3 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #251
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
Slimmed down this PR by creating three other PRs from content out of direct scope of this, this PR needs PR #276 merged in before this one. |
Notes: