ML-DSA driver for multipart operations#770
ML-DSA driver for multipart operations#770gilles-peskine-arm wants to merge 17 commits intoMbed-TLS:developmentfrom
Conversation
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
size-s
github-project-automation
Bot
moved this to In Development
in Roadmap pull requests (new board)
gilles-peskine-arm
force-pushed
the
mldsa-sign-multipart-driver
branch
from
5783913 to
ff13977
Compare
In setup, only validation is implemented, not the storing of the key or the start of the operation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
mldsa-sign-multipart-driver
branch
from
ff13977 to
b0aec83
Compare
gilles-peskine-arm
added
needs-work
needs-review
As with the key type and algorithm macros, they are in the driver header for now, and will move to the public API headers once MLDSA is reachable from the API. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In setup, only validation is implemented, not the key expansion or the start of the operation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is a deviation from the application interface, which is practical given that the core is supposed to have a copy of the key in the key store, and is useful because it saves the driver from needing to make its own copy. ARM-software/psa-api#350 Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Copy the expanded private key on the heap from setup to finish. Temporarily allocate the public key on the heap during setup. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The pattern (c1 ? v1 : c2 ? v2 : v3) in a preprocessor conditional expression triggers a preprocessor bug in MSVC ``` fatal error C1012: unmatched parenthesis: missing ')' ``` even with Visual Studio 2019 and `/Zc:preprocessor`. Avoid this pattern. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
mldsa-sign-multipart-driver
branch
from
b0aec83 to
fcafc94
Compare
gilles-peskine-arm
added
needs-preceding-pr
bjwtaylor
left a comment
bjwtaylor
left a comment
There was a problem hiding this comment.
Need to do a full review, though a quick question I had noticed.
| * been set up for signing and not finished | ||
| * or aborted yet. | ||
| * \param[out] signature On success, the exported key. | ||
| * \param[in] key_buffer The key material. This must be the same seed |
There was a problem hiding this comment.
Key buffer and key_buffer are never used. Presumably they are specified in the API and then not needed by the function? Is there a plan to use these variables in the future and whatever the plan do we need to document the current reasoning behind this?
There was a problem hiding this comment.
For ML-DSA, the signature calculation needs the public key during setup, and the expanded private key during finish. The standard PSA format for the private key is just the seed. Since the same mldsa-native function calculates both the expanded private key and the public key from the seed, we store the expanded private key during sign_setup and use it in sign_finish (where a comment explains this). It's a performance/memory trade-off: we could instead recalculate the private key in sign_finish.
In addition to this memory/performance tradeoff, there are other algorithms or key representation formats where it's more advantageous to not store any key material between setup and finish. So I proposed that the driver interface would pass the key both to setup and finish. (The driver interface isn't officially defined yet.) ARM-software/psa-api#350
We plan to also support a key representation that's fully expanded, in which case there's nothing to calculate and sign_finish would read the part that it needs from key_buffer. #774
There was a problem hiding this comment.
OK is it worth adding a comment here and potentially at the void points to capture this?
There was a problem hiding this comment.
In the header, I don't think this warrants a comment: whether the parameter is used or not is an implementation detail. The API follows the applicable standard, or at least the current draft of that standard.
In the implementation, there's already a comment just above the void point.
| tf_psa_crypto_mldsa_operation_t *operation, | ||
| const uint8_t *input, size_t input_length); | ||
|
|
||
| /** Finish a pure-ML-DSA signature operation. |
There was a problem hiding this comment.
Minor: There maybe a good reason for this, however the doxygen docs list the parameters in a different order to the prototype. If there's no good reason for it, then it may be worth re-ordering them to improve readability.
| const uint8_t *input, size_t input_length) | ||
| { | ||
| mld_shake256_absorb(&operation->shake, input, input_length); | ||
| return PSA_SUCCESS; |
There was a problem hiding this comment.
This doesn't look right, if I've understood correctly the mld_shake256_absorb calls functions that can fail. I understand they are going through the driver wrapper though, so is there no way we can pass the error?
There was a problem hiding this comment.
mld_xxx functions return void.
The only cases where the functions can fail internally are:
- If there's a bug in our code and we end up calling
setupin a configuration where the algorithm is not provided. This shouldn't happen thanks to compile-time guards. - If SHAKE is provided by a third-party PSA driver (i.e. neither our implementation nor the one in mldsa-native) and that one has failure conditions, which is unusual, especially for an algorithm that doesn't use a secret key.
That second case is a bug, filed as #744. Given how unusual it is for it to matter, it's very low priority.
| * been set up for signing and not finished | ||
| * or aborted yet. | ||
| * \param[out] signature On success, the exported key. | ||
| * \param[in] key_buffer The key material. This must be the same seed |
There was a problem hiding this comment.
OK is it worth adding a comment here and potentially at the void points to capture this?
| *signature_length = 0; | ||
|
|
||
| if (operation->parameter_set != 87) { | ||
| return PSA_ERROR_NOT_SUPPORTED; |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
No, because this can't happen except if there's an internal error somewhere. I put this check in partly for robustness, and partly as a signpost to indicate that in the future, when we support multiple sizes, we'll need to have a switch on the size.
| * we could instead re-expand the private key from the seed | ||
| * in \p key_buffer here. */ | ||
| if (operation->key_length != MLDSA87_SECRETKEYBYTES) { | ||
| return PSA_ERROR_CORRUPTION_DETECTED; |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
No. Generally PSA_ERROR_CORRUPTION_DETECTED indicates that something happened that should not have happened, so there's no way to test it.
| if (abort_status != PSA_SUCCESS) { | ||
| return abort_status; | ||
| } | ||
| return pqcp_to_psa_error(ret); |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
As above, there is no failure more for abort.
| const uint8_t *signature, size_t signature_length) | ||
| { | ||
| if (operation->parameter_set != 87) { | ||
| return PSA_ERROR_NOT_SUPPORTED; |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
As above, it's currently impossible to get that far without having a supported size in the operation.
|
|
||
| psa_status_t abort_status = tf_psa_crypto_mldsa_abort(operation); | ||
| if (abort_status != PSA_SUCCESS) { | ||
| return abort_status; |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
As above, there is no failure more for abort.
| } | ||
|
|
||
| if (ret == MLD_ERR_FAIL) { | ||
| return PSA_ERROR_INVALID_SIGNATURE; |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
We do.
grep -c 'verify_multipart.*PSA_ERROR_INVALID_SIGNATURE' tests/suites/test_suite_psa_crypto_mldsa.*.data
| if (ret == MLD_ERR_FAIL) { | ||
| return PSA_ERROR_INVALID_SIGNATURE; | ||
| } else { | ||
| return pqcp_to_psa_error(ret); |
There was a problem hiding this comment.
Do we have/need a test that covers this return?
There was a problem hiding this comment.
Currently there are no possible errors from verify, other than the invalid-signature case. In the near future, there will be an INSUFFICIENT_MEMORY case.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2 participants
Add support for multipart pure ML-DSA signature at the driver layer. Resolves #635
The signature operation allocates heap memory. We will revise this later, likely when we give the signature function access to the expanded key.
Needs preceding PR: test case generation in Mbed-TLS/mbedtls-framework#300
PR checklist