Reject invalid API inputs before unsafe work#1231
Conversation
Signed-off-by: Fredrik Dahlgren <fredrik.dahlgren@trailofbits.com>
|
|
||
| if (ctxlen > 255 || (ctx == NULL && ctxlen != 0)) | ||
| { | ||
| *siglen = 0; | ||
| mld_memset(sig, 0, MLDSA_CRYPTO_BYTES); | ||
| return MLD_ERR_FAIL; | ||
| } | ||
|
|
||
| MLD_ALLOC(pre, uint8_t, MLD_DOMAIN_SEPARATION_MAX_BYTES, context); | ||
| MLD_ALLOC(rnd, uint8_t, MLDSA_RNDBYTES, context); |
There was a problem hiding this comment.
The check must happen after the allocations for strict C90 compliance. Which has the benefit that we can reuse the cleanup section.
| if (mlen > SIZE_MAX - MLDSA_CRYPTO_BYTES) | ||
| { | ||
| *smlen = 0; | ||
| return MLD_ERR_FAIL; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure about this change.
If a user wants to call us with bad parameters, they can. They can just pass a nonsense (non-NULL) address, and we'll crash.
So, if we do want to conduct input validation, I think it should be input validation for the sake of preventing accidental misuse, not deliberate/adversarial one. The above check doesn't fall under that category, IMO, so I don't think we should add it.
| if (siglen != MLDSA_CRYPTO_BYTES || (externalmu && mlen != MLDSA_CRHBYTES)) | ||
| { | ||
| return MLD_ERR_FAIL; | ||
| } | ||
|
|
||
| MLD_ALLOC(buf, uint8_t, (MLDSA_K * MLDSA_POLYW1_PACKEDBYTES), context); | ||
| MLD_ALLOC(mu, uint8_t, MLDSA_CRHBYTES, context); | ||
| MLD_ALLOC(c, uint8_t, MLDSA_CTILDEBYTES, context); | ||
| MLD_ALLOC(c2, uint8_t, MLDSA_CTILDEBYTES, context); | ||
| MLD_ALLOC(z, mld_polyvecl, 1, context); | ||
| MLD_ALLOC(cp, mld_poly, 1, context); | ||
| MLD_ALLOC(mat, mld_polymat, 1, context); | ||
| MLD_ALLOC(w1, mld_poly, 1, context); | ||
| MLD_ALLOC(tmp, mld_poly, 1, context); |
There was a problem hiding this comment.
Same as above: It's for C90 compliance that we need to conduct the declarations/allocations upfront.
| size_t phlen, const uint8_t *ctx, size_t ctxlen, int hashalg) | ||
| { | ||
| if (ctxlen > 255) | ||
| if (ctxlen > 255 || (ctx == NULL && ctxlen != 0)) |
There was a problem hiding this comment.
I'd prefer input validation to happen at the top-level API, even if it means a bit of code-duplication.
| requires(memory_no_alias(siglen, sizeof(size_t))) | ||
| requires(memory_no_alias(m, mlen)) | ||
| requires(ctxlen <= MLD_MAX_BUFFER_SIZE) | ||
| requires(ctxlen <= 255) |
There was a problem hiding this comment.
It's unclear if this should be changed. There are two valid purposes for the CBMC specs:
- MUST: Document the conditions under which the function is safe to be called and has well-defined behavior.
ctxlen < = 255is not part of that. - MAY: Document the success conditions for the function.
ctx <= 255is (now) part of that.
The CBMC spec differs in this regard from the natural language documentation, which should be focused on 2. -- the success case.
I'm thus leaning towards changing Should be <= 255 to Must be <= 255 in the dcoumentation, but keep the CBMC contract as-is.
| * @param mlen Length of message. Must be <= SIZE_MAX - | ||
| * MLDSA_CRYPTO_BYTES. |
There was a problem hiding this comment.
I'd suggest to drop Must be <= SIZE_MAX - MLDSA_CRYPTO_BYTES -- this is already implied by the validity assumption and the fact that sm has size MLDSA_CRYPTO_BYTES + mlen.
| } \ | ||
| } while (0) | ||
|
|
||
| #define TEST_NO_ALLOC_FAILURE(test_name, call, expected_rc) \ |
There was a problem hiding this comment.
This is no longer needed once we reorder the checks past allocation.
hanno-becker
left a comment
hanno-becker
left a comment
There was a problem hiding this comment.
Thank you for initiating a discussion on input validation, @fegge.
So far, we have deliberately not conducted input validation, but instead simply assumed that the user passes correct arguments. As mentioned in the comments, we ultimately have to do that, because true pre-conditions such as pointer-validity cannot be checked. For that reason, I think the only input validation one should consider is that aiming to catch accidental misuse.
I'd be OK with consistently adding len == 0 || ptr != NULL checks as an safe-guard against accidental passage of bad length or NULL pointers. If we decide to do so, this should be applied to all top-level APIs and all pointers consistently, perhaps even through some shared macro.
@mkannwischer wdyt
2 participants
Summary
Validation
clang-format -i mldsa/src/sign.c mldsa/src/sign.h mldsa/mldsa_native.h test/src/test_alloc.c test/src/test_mldsa.c: passedgit diff --check -- mldsa/src/sign.c mldsa/src/sign.h mldsa/mldsa_native.h test/src/test_alloc.c test/src/test_mldsa.c: passedmake run_alloc run_func -j4: passed./scripts/formatbecausenixpkgs-fmtis not installed in this local environment./scripts/lintbecauseshfmtis not installed in this local environmentFixes #1212
This work was completed by Trail of Bits as part of the Patch The Planet project in collaboration with OpenAI. The issue was identified primarily by the Codex coding agent, and manually reviewed before submission.