`siglen` parameter in `crypto_sign_signature` is prone to API misuse

[davidchisnall]

The crypto_sign_signature function takes a pointer that it will set to the length of the output buffer. This is less than ideal for good ergonomics:

• The parameter makes it appear as if the length of the signature is dynamic, but it is not, it is always MLDSA_CRYPTO_BYTES.
• The shape of the parameter pair (buffer followed by length) is sufficiently close to other uses where it is an input parameter that callers may expect it to be set on input.
• It isn't checked internally, so if a caller does set it and it has the wrong value, then this is ignored.
• A check of the size after the call is almost never useful. There are two cases:
  • The caller provided a buffer that is too large. The check may enable a call to realloc or some other approach to trim the buffer (though given that the size is invariant, this is almost always an error).
  • The caller provided a buffer that is too small. In this case, the check of the returned size happens after the library has written past the end of a buffer and therefore the state of the value is undefined.

All of this adds up to a dangerous API pattern, where people write code that appears defensive but will actually corrupt memory.

[mkannwischer]

Thanks @davidchisnall for bringing up this important issue - this also had bothered me before.

I'm torn, however:

One one hand I agree with you that there is no good reason to have this parameter in ML-DSA in the first place. It serves no techincal purpose and as @davidchisnall outlines it causes at least confusion and possibly API misuse.

On the other hand, this API his inherited from the reference implementations which implements it to fit into libraries like libOQS^ and PQClean (which do indeed support signature schemes with variable-length signatures). I consider keeping mldsa-native compatible with these implementations important to make it easy to switch to mldsa-native.

^ More context: The crypto_sign (which has the same problem), and crypto_sign_open APIs are actuall inherited from the SUPERCOP project and were mandated by NIST for reference implementations. This is not the case for the crypto_sign_signature and the crypto_sign_verify APIs - those were primarily required by libOQS if I remember correctly.

That said, even if we want to maintain compatibility, this does not have to be the default API. We could consider switchting to simpler, less confusing/dangerous API (i.e., just dropping the siglen/smlen from signature and sign) and adding a configuration option to enable the SUPERCOP/libOQS-compatible APIs. If we do that, however, we should move away from the crypto_sign-prefixes as that signals compatibility.

@hanno-becker, WDYT? Maybe @dstebila also has some opinion on this.

[hanno-becker]

I think we should expose a simple API by default, and offer wrappers for SUPERCOP if desired. At the moment, those 'wrappers' are just #define's in mldsa_native.h, keeping the signature but changing the naming; they could be changed to shallow inline wrappers bridging the signatures.

[mkannwischer]

While at it, should crypto_sign_verify have a siglen argument or not?

int crypto_sign_verify(const uint8_t *sig, size_t siglen, const uint8_t *m,
                       size_t mlen, const uint8_t *ctx, size_t ctxlen,
                       const uint8_t pk[MLDSA_CRYPTO_PUBLICKEYBYTES])

It's somewhat different because it is not confusing - but it still serves very little to no purpose.

[davidchisnall]

It remains confusing when using another language, even quite a similar one like C++. Reviewing code that uses crypto APIs from C++, I want to check one of two rules at APIs like this:

• If a pointer + length are passed, they come from the .size() and .data() members of the same object.
• If a pointer is passed without a length, it comes from a type that represents that use (e.g. using MLDSASignature = std::array<uint8_t, MLDSA_CRYPTO_BYTES>;)

In the first case, I expect the underlying API to handle incorrect sizes, but it's preferable to make invalid states unrepresentable as part of the API contract. If the first parameter is const uint8_t sig[MLDSA_CRYPTO_BYTES], then checking that the .data() call is on an object of exactly MLDSA_CRYPTO_BYTES is trivial. And also something that static analysers can warn about if I get it wrong in code review.

[hanno-becker]

Yes, if siglen is dropped from crypto_sign_verify, sig should become const uint8_t sig[MLDSA_CRYPTO_BYTES]. I think that's also what @mkannwischer intended, or?