-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
CMS: Add support for hashless signing schemes (ML/SLH-DSA & EdDSA) for no-attributes case #28923
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
Conversation
|
@DDvO @danvangeest FYI |
|
would you please sign a CLA? https://openssl-library.org/policies/cla/ |
|
Very nice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I likely won't have the time for a full review in the next two weeks,
but here are some name/style-wise improvement suggestions at API level.
I saw that some CMS-related tests were already working for the key types I am extending it for in this PR, but the non-attributes case was not covered. I am not sure what to do about PKCS#7... One concern I have with the implementation is the BIO of the input data. I am wondering whether to make a copy of the data since the data may have to be read multiple times (for multiple keys in pure mode), even though this may cause issues with very large files but then on the other hand would improve performance. |
80e45d0 to
3b0423e
Compare
3b0423e to
771b240
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for very swiftly picking up my API suggestions and for adding doc entries.
For now, just spotted some doc nits.
Since this PR also documents the pre-existing functions CMS_dataFinal() and CMS_SignerInfo_verify_content(), you can (well, even should) remove them from util/missingcrypto.txt.
This extension is clearly worth an entry in NEWS.md.
There was a problem hiding this 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 adds support for hashless signing schemes (ML-DSA, SLH-DSA, and EdDSA) in CMS when no signed attributes are present. The implementation enables these schemes to directly sign raw data instead of pre-computed hashes by passing the original data BIO through to signing and verification functions.
Key changes:
- Added new public APIs
CMS_dataFinal_hashless()andCMS_SignerInfo_verify_hashless()to support hashless signing schemes - Modified internal signing/verification functions to handle both hash-based and hashless signing schemes
- Added comprehensive test coverage for EdDSA (Ed25519, Ed448), ML-DSA-44, and SLH-DSA-SHAKE-256s with the
-noattrflag
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| util/libcrypto.num | Exports new public API functions for hashless signing support |
| test/recipes/80-test_cms.t | Adds test cases for hashless signing schemes (EdDSA, ML-DSA, SLH-DSA) with -noattr |
| include/openssl/cms.h.in | Declares new public APIs for hashless signing and verification |
| doc/man3/CMS_verify.pod | Documents the new verification functions for hashless schemes |
| doc/man3/CMS_final.pod | Documents the new finalization functions for hashless schemes |
| crypto/cms/cms_smime.c | Updates high-level CMS functions to use hashless variants |
| crypto/cms/cms_sd.c | Implements core hashless signing/verification logic with algorithm-specific handling |
| crypto/cms/cms_local.h | Updates internal function signatures to support data BIO parameter |
| crypto/cms/cms_lib.c | Implements new public APIs and updates internal routing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
There was no direct button for triggering the Copilot/AI review bot for some reason |
|
Please add to the message of the respective commit this line: |
|
CI failures are relevant; please check and fix issues. |
771b240 to
59f1706
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two hints how to get rid of the doc-nits CI failure.
Again, since this PR also documents the pre-existing functions CMS_dataFinal() and CMS_SignerInfo_verify_content(), should remove them from util/missingcrypto.txt
And comments regarding minor improvements on the coding style.
| CMS_SignerInfo *CMS_add1_signer(CMS_ContentInfo *cms, | ||
| X509 *signer, EVP_PKEY *pk, const EVP_MD *md, | ||
| unsigned int flags) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the only remaining coding style issue reported by the CI is:
crypto/cms/cms_sd.c:421:function body length = 206 > 200 lines:CMS_SignerInfo *CMS_add1_signer(CMS_ContentInfo *cms,
While this PR already downsizes the function body a little bit,
there would be good chances by tweaking some other parts in here to get below the given "limit" ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is removing empty lines allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can ignore this issue.
59f1706 to
746619f
Compare
|
The check docs and pre-commit failures are relevant. |
746619f to
a961417
Compare
950d6b2 to
f9fe645
Compare
crypto/cms/cms_sd.c
Outdated
| const char *name; | ||
| int md_nid; | ||
| int noattr_md_nid; /* in case of 'without signed attributes' */ | ||
| int has_msg_update; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested in the OpenSSL maintainer's opinion here, but I'm not a fan of hardcoding these values. It doesn't cover algorithms which are implemented in a provider but not in OpenSSL. It also doesn't cover 3rd party provider implementations of ML-DSA which support
EVP_PKEY_signbut notEVP_PKEY_sign_message_update.
I think we can hardcode some defaults (like this table) and then add a CAPABILITIES entry (as the TLS capabilities are) for the additional entries that could override this default table.
The first issue could be solved by having the provider supply this information, similar to what
OSSL_PKEY_PARAM_CMS_KEMRI_KDF_ALGORITHMdoes for KEMs.
IMO capabilities are better way.
The second could be solved by checking whether
sign_message_updateis set inEVP_SIGNATURE, although I don't know if people would be keen about this check being done in CMS code.
That would be IMO OK, but it requires adding a public function for that such as EVP_SIGNATURE_has_message_update() or something like that.
b4088b8 to
6231ff4
Compare
t8m
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to run make update to update the libcrypto.num file.
crypto/evp/signature.c
Outdated
| return signature->verify_message_update | ||
| && signature->sign_message_update; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: signature->verify_message_update != NULL && signature->sign_message_update != NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Get a default hash for hash-less signing schemes such as ML-DSA, SLH-DSA, and EdDSA in the case when signed attributes are present as well as for the no signed attributes case. For the latter case, EdDSA is the only signing scheme that has a required hash (sha512 for ED25519 and shake256 for ED448), all other ones have a suggested hash. Only use the suggested hash if the hash provided by the caller of CMS_add1_signer passed a NULL pointer for md. Use the required hash in any case, overriding any choice of the caller. Fixes: openssl#13523 Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Enable the ability to sign with a hashless signing schemes, such as ML-DSA in pure mode, in case no attributes are used in CMS. To support this, pass the BIO with the plain data through to the signing function so that key's pure mode signing scheme can hash the data itself. The current implementation relies on a seek'able BIO so that the data stream can be read multiple times for support of multiple keys. Some signing schemes, such as ML-DSA, support the message_update function when signing data, others, such as EdDSA keys do not support it. The former allows for reading data in smaller chunks and calling EVP_PKEY_sign_message_update with the data, while the latter requires that all data are all read into memory and then passed for signing. This latter method could run into out-of-memory issue when signing very large files. Fixes: openssl#28279 Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
…gning) Enable signature verification for hashless signing schemes, such as ML-DSA and EdDSA, for the non-attribute case of CMS. Also in this case the BIO with the plain input data needs to be passed through to the signature verification function so that the pure-mode signature verification method can hash the plain data itself. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Add CMS test cases for no-attribute signing for ML-DSA, SLH-DSA amd EdDSA (Ed448 and Ed25519 keys). Fixes: openssl#11915 Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Implement EVP_SIGNATURE_hash_message_update() to check for support of EVP_PKEY_sign_message_update() and EVP_PKEY_verify_message_update() and use this function to replace the has_msg_update column in CMS. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
73c47f1 to
b82fa0b
Compare
beldmit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
This pull request is ready to merge |
|
Merged to the master branch. Thank you for your contribution. |
Get a default hash for hash-less signing schemes such as ML-DSA, SLH-DSA, and EdDSA in the case when signed attributes are present as well as for the no signed attributes case. For the latter case, EdDSA is the only signing scheme that has a required hash (sha512 for ED25519 and shake256 for ED448), all other ones have a suggested hash. Only use the suggested hash if the hash provided by the caller of CMS_add1_signer passed a NULL pointer for md. Use the required hash in any case, overriding any choice of the caller. Fixes: #13523 Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28923)
Enable the ability to sign with a hashless signing schemes, such as ML-DSA in pure mode, in case no attributes are used in CMS. To support this, pass the BIO with the plain data through to the signing function so that key's pure mode signing scheme can hash the data itself. The current implementation relies on a seek'able BIO so that the data stream can be read multiple times for support of multiple keys. Some signing schemes, such as ML-DSA, support the message_update function when signing data, others, such as EdDSA keys do not support it. The former allows for reading data in smaller chunks and calling EVP_PKEY_sign_message_update with the data, while the latter requires that all data are all read into memory and then passed for signing. This latter method could run into out-of-memory issue when signing very large files. Fixes: #28279 Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28923)
…gning) Enable signature verification for hashless signing schemes, such as ML-DSA and EdDSA, for the non-attribute case of CMS. Also in this case the BIO with the plain input data needs to be passed through to the signature verification function so that the pure-mode signature verification method can hash the plain data itself. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28923)
Implement EVP_SIGNATURE_hash_message_update() to check for support of EVP_PKEY_sign_message_update() and EVP_PKEY_verify_message_update() and use this function to replace the has_msg_update column in CMS. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28923)
Add support for hashless signing schemes, such as ML-DSA, SLH-DSA, and EdDSA, for the case that signed attributes are absent. Add test cases for all newly supported signing schemes.
Enable the ability to sign (verify) with a hashless signing schemes, such as ML-DSA in pure mode, in case no attributes are used in CMS. To support this, pass the BIO with the plain data through to the signing (verification) function so that key's pure mode signing scheme can hash the data itself.
The current implementation relies on a seek'able BIO so that the data stream can be read multiple times for support of multiple keys.
Some signing schemes, such as ML-DSA, support the message_update function when signing data, others, such as EdDSA keys do not support it. The former allows for reading data in smaller chunks and calling
EVP_PKEY_sign_message_update()with the data, while the latter requires that all data are all read into memory and then passed for signing. This latter method could run into out-of-memory issue when signing very large files.Fixes #11915
Fixes #13523 (?)
Fixes #28279
Checklist