Remove siglen from signing and verification APIs#1240
Conversation
12f326f to
20221a6
Compare
CBMC Results (ML-DSA-44, REDUCE-RAM)Full Results (206 proofs)
|
CBMC Results (ML-DSA-87, REDUCE-RAM)Full Results (206 proofs)
|
CBMC Results (ML-DSA-65, REDUCE-RAM)Full Results (206 proofs)
|
CBMC Results (ML-DSA-65)Full Results (206 proofs)
|
CBMC Results (ML-DSA-44)Full Results (206 proofs)
|
CBMC Results (ML-DSA-87)Full Results (206 proofs)
|
92392f2 to
fe85367
Compare
CBMC Results (ML-DSA-44)
Full Results (203 proofs)
|
|
@jakemas @bhess @rod-chapman @hanno-becker, WDYT about this proposed change? |
0c4d036 to
12c9398
Compare
12c9398 to
09eef61
Compare
d94cfd6 to
f34f91f
Compare
| #include <stddef.h> | ||
| #include <stdint.h> | ||
|
|
||
| #include "mldsa/src/sign.h" |
There was a problem hiding this comment.
Why is this working with sign.h and not with mldsa_native.h?
There was a problem hiding this comment.
OQS doesn't include any headers at all. I used sign.h here because it's a little easier to use and arguably sig_glue.c itself is internal as well.
But we can switch to mldsa_native.h without much complication.
Pushed.
| * | ||
| * @spec{Implements @[FIPS204, Algorithm 7, ML-DSA.Sign_internal].} | ||
| * | ||
| * @param[out] sig Output signature. |
There was a problem hiding this comment.
The expected length should be specified here. While at it, the text should also be aligned with the other parameters, e.g. Pointer to buffer to hold the generated signature.
| @@ -368,7 +365,6 @@ int MLD_API_NAMESPACE(signature_extmu)( | |||
| * @spec{Implements @[FIPS204, Algorithm 8, ML-DSA.Verify_internal].} | |||
| * | |||
| * @param[in] sig Pointer to input signature. | |||
There was a problem hiding this comment.
Similar here, the length should be documented not only in the signature, but also in the documentation.
hanno-becker
left a comment
There was a problem hiding this comment.
Overall I support this change. Two questions/requests:
- Ideally, no consumer should work with
sign.h, because this is internal. They should be working with the public headermldsa_native.honly. Is there a reason why that's not possible? - Now that the siglen parameter is dropped, the buffer length assumption should be documented.
f34f91f to
af48d97
Compare
af48d97 to
6143622
Compare
6143622 to
b032a56
Compare
hanno-becker
left a comment
There was a problem hiding this comment.
This is a welcome simplification, thank you @mkannwischer.
I left some observations for how we can improve documentation at the same time, but they are not blocking.
b032a56 to
4bbd6c1
Compare
ML-DSA signatures have a fixed size, so siglen was redundant: an always-MLDSA_BYTES output for signing and an input used only for a fixed-size check on verification. Drop it from all signing and verification functions (internal, randomized, extmu, and pre-hash variants) and the SUPERCOP-named aliases. Verification now takes the signature as a fixed-size array. Update the CBMC contracts and harnesses, tests, and examples to match. Adjust AWS-LC, Pavona, and OQS integrations and their tests accordingly. Signed-off-by: Matthias J. Kannwischer <matthias@zerorisc.com>
4bbd6c1 to
7a622ca
Compare
While I can’t argue with the simplification of the API, I do want to put down some thoughts. It kind of goes against aws-LC design guidelines. I believe, across awslc we intend to include len (and length checks) to buffers such as this, so I’m not sure the change is positive for aws-LC. Can’t currently check, but IIRC it’s part of the EVP API so we are always going to have those values as call time. We can of course do anything outside of the mldsa API and within EVP itself, but these are consequences of this change. I do recall some parts in FIPS204 that says about length checks are a requirement (I’m not sure that extends as far as an API requirement, but perhaps worth a check over FIPS 204 on exact language here). See perhaps Section 3.6.2:
siglen allows the caller to define this exactly, and when accepting buffers from potentially untrusted sources, not a bad call for API security in depth. Are we making the API more or less misuse resistant here, are realistic FIPS callers going to be making this check anyway, do we like the control the caller has to enforce len at call time. Would we have to add a caveat that to have FIPS204 one would need to validate the sig length before calling our API? Does this make usage actually more difficult and confusing? |
|
Many thanks @jakemas for chiming in! I think you raise very good points. For higher level APIs such as provided by AWS-LC, it might make sense to always pair buffers with their lengths, and include lengths-checks as required. If we look at the lower-level API of mlkem/mldsa-native, however, this is not the case: Where buffers are statically sized by the spec, we pass them as statically sized buffers. Good examples are I believe there is no right or wrong here: Each library has an API style to choose and a suitable error checking regime to implement. If you pass buffers + lengths, and the length is subject to constraints, you may want to check them. If you merely pass buffers, you may or may not want to include at least rudimentary Generally, the higher-level your API gets, the less you want to assume and the more checks you want to include. So, at the level of the EVP API it makes sense to check everything, IMO. But equally I believe that at the lowest level we are looking at here, not passing redundant information, and limiting parameter validation checks, is a defensible choice, and it remains my preference. Note that there is no need to demand that in stack of APIs, the APIs all follow the same choice -- much the contrary. As you move from one layer to another, you'll discharge the conditions that the higher layer does not assume, but the lower layer does. We see this in @mkannwischer's patch to the LC integration layer: The siglen checks are still present in LC, they have just been hoisted out. There has not been a change in behavior for LC, overall. Overall, I believe that this is a good change which removes an outlier to what is otherwise a consistent low-level API-style pursued in mlx-native. But Jake is right that lengths checks definitely have a place and the higher levels, and we need to make sure there's no change in behavior for our integrations -- which is the case, as far as I see. Thanks again @jakemas for joining in and let us know further thoughts 🙏 |
I would like initiate the discussion on the following breaking change to our API. Please share your thoughts if this change should be made prior to releasing v1.0.0. I have updated the AWS-LC, Pavona, and liboqs integrations (see the patches).
ML-DSA signatures have a fixed size, so siglen was redundant: an always-MLDSA_BYTES output for signing and an input used only for a fixed-size check on verification. Drop it from all signing and verification functions (internal, randomized, extmu, and pre-hash variants). Verification now takes the signature as a fixed-size array.
siglenparameter incrypto_sign_signatureis prone to API misuse #789