Skip to content

Reject invalid API inputs before unsafe work #1231

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.

Sign up for GitHub

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

Open
fegge wants to merge 1 commit into
base: main
Choose a base branch
from
+181 −24
Open

Reject invalid API inputs before unsafe work #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions mldsa/mldsa_native.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ int MLD_API_NAMESPACE(signature_internal)(
* @param[in] m Pointer to message to be signed.
* @param mlen Length of message.
* @param[in] ctx Pointer to context string. May be NULL if ctxlen == 0.
* @param ctxlen Length of context string. Should be <= 255.
* @param ctxlen Length of context string. Must be <= 255.
* @param[in] sk Bit-packed secret key.
* @param context Application context. Only present when
* MLD_CONFIG_CONTEXT_PARAMETER is defined; type set by
Expand Down Expand Up @@ -436,9 +436,10 @@ int MLD_API_NAMESPACE(signature_extmu)(
* MLDSA{44,65,87}_BYTES + mlen bytes); can be equal to m.
* @param[out] smlen Pointer to output length of signed message.
* @param[in] m Pointer to message to be signed.
* @param mlen Length of message.
* @param[in] ctx Pointer to context string.
* @param ctxlen Length of context string.
* @param mlen Length of message. Must be <= SIZE_MAX -
* MLDSA{44,65,87}_BYTES.
* @param[in] ctx Pointer to context string. May be NULL if ctxlen == 0.
* @param ctxlen Length of context string. Must be <= 255.
* @param[in] sk Bit-packed secret key.
* @param context Application context. Only present when
* MLD_CONFIG_CONTEXT_PARAMETER is defined; type set by
Expand Down Expand Up @@ -835,8 +836,8 @@ int MLD_API_NAMESPACE(verify_pre_hash_shake256)(
* @param[in] ph Pointer to pre-hashed message (ignored for pure
* ML-DSA).
* @param phlen Length of pre-hashed message (ignored for pure ML-DSA).
* @param[in] ctx Pointer to context string (may be NULL).
* @param ctxlen Length of context string.
* @param[in] ctx Pointer to context string. May be NULL if ctxlen == 0.
* @param ctxlen Length of context string. Must be <= 255.
* @param hashalg Hash algorithm constant (MLD_PREHASH_NONE for pure
* ML-DSA, or MLD_PREHASH_* for HashML-DSA).
*
Expand Down
35 changes: 28 additions & 7 deletions mldsa/src/sign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,14 @@ int mld_sign_signature_internal(uint8_t sig[MLDSA_CRYPTO_BYTES], size_t *siglen,
uint8_t *rho, *tr, *key, *mu, *rhoprime;
uint16_t nonce = 0;
const uint16_t nonce_limit = mld_get_max_signing_attempts();

if (externalmu && mlen != MLDSA_CRHBYTES)
{
*siglen = 0;
mld_memset(sig, 0, MLDSA_CRYPTO_BYTES);
return MLD_ERR_FAIL;
}

MLD_ALLOC(seedbuf, uint8_t,
2 * MLDSA_SEEDBYTES + MLDSA_TRBYTES + 2 * MLDSA_CRHBYTES, context);
MLD_ALLOC(mat, mld_polymat, 1, context);
Expand Down Expand Up @@ -1052,6 +1060,14 @@ int mld_sign_signature(uint8_t sig[MLDSA_CRYPTO_BYTES], size_t *siglen,
{
size_t pre_len;
int ret;

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);
Comment on lines +1063 to 1072

@hanno-becker hanno-becker Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check must happen after the allocations for strict C90 compliance. Which has the benefit that we can reuse the cleanup section.


Expand Down Expand Up @@ -1153,6 +1169,12 @@ int mld_sign(uint8_t *sm, size_t *smlen, const uint8_t *m, size_t mlen,
int ret;
size_t i;

if (mlen > SIZE_MAX - MLDSA_CRYPTO_BYTES)
{
*smlen = 0;
return MLD_ERR_FAIL;
}

Comment on lines +1172 to +1177

@hanno-becker hanno-becker Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (i = 0; i < mlen; ++i)
__loop__(
assigns(i, object_whole(sm))
Expand Down Expand Up @@ -1187,6 +1209,11 @@ int mld_sign_verify_internal(const uint8_t *sig, size_t siglen,
int ret, cmp;
unsigned int i;

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);
Expand All @@ -1204,12 +1231,6 @@ int mld_sign_verify_internal(const uint8_t *sig, size_t siglen,
goto cleanup;
}

if (siglen != MLDSA_CRYPTO_BYTES)
{
ret = MLD_ERR_FAIL;
goto cleanup;
}

mld_memcpy(c, sig, MLDSA_CTILDEBYTES);
mld_polyvecl_unpack_z(z, sig + MLDSA_SIG_Z_OFFSET);

Expand Down Expand Up @@ -1607,7 +1628,7 @@ size_t mld_prepare_domain_separation_prefix(
uint8_t prefix[MLD_DOMAIN_SEPARATION_MAX_BYTES], const uint8_t *ph,
size_t phlen, const uint8_t *ctx, size_t ctxlen, int hashalg)
{
if (ctxlen > 255)
if (ctxlen > 255 || (ctx == NULL && ctxlen != 0))

@hanno-becker hanno-becker Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer input validation to happen at the top-level API, even if it means a bit of code-duplication.

{
return 0;
}
Expand Down
19 changes: 10 additions & 9 deletions mldsa/src/sign.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ __contract__(
* @param[in] m Pointer to message to be signed.
* @param mlen Length of message.
* @param[in] ctx Pointer to context string. May be NULL if ctxlen == 0.
* @param ctxlen Length of context string. Should be <= 255.
* @param ctxlen Length of context string. Must be <= 255.
* @param[in] sk Bit-packed secret key.
* @param context Application context. Only present when
* MLD_CONFIG_CONTEXT_PARAMETER is defined; type set by
Expand Down Expand Up @@ -281,7 +281,7 @@ __contract__(
requires(memory_no_alias(sig, MLDSA_CRYPTO_BYTES))
requires(memory_no_alias(siglen, sizeof(size_t)))
requires(memory_no_alias(m, mlen))
requires(ctxlen <= MLD_MAX_BUFFER_SIZE)
requires(ctxlen <= 255)

@hanno-becker hanno-becker Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear if this should be changed. There are two valid purposes for the CBMC specs:

  1. MUST: Document the conditions under which the function is safe to be called and has well-defined behavior. ctxlen < = 255 is not part of that.
  2. MAY: Document the success conditions for the function. ctx <= 255 is (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.

requires(ctxlen == 0 || memory_no_alias(ctx, ctxlen))
requires(memory_no_alias(sk, MLDSA_CRYPTO_SECRETKEYBYTES))
assigns(memory_slice(sig, MLDSA_CRYPTO_BYTES))
Expand Down Expand Up @@ -342,9 +342,10 @@ __contract__(
* MLDSA_CRYPTO_BYTES + mlen bytes); can be equal to m.
* @param[out] smlen Pointer to output length of signed message.
* @param[in] m Pointer to message to be signed.
* @param mlen Length of message.
* @param[in] ctx Pointer to context string.
* @param ctxlen Length of context string.
* @param mlen Length of message. Must be <= SIZE_MAX -
* MLDSA_CRYPTO_BYTES.
Comment on lines +345 to +346

@hanno-becker hanno-becker Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param[in] ctx Pointer to context string. May be NULL if ctxlen == 0.
* @param ctxlen Length of context string. Must be <= 255.
* @param[in] sk Bit-packed secret key.
* @param context Application context. Only present when
* MLD_CONFIG_CONTEXT_PARAMETER is defined; type set by
Expand All @@ -370,8 +371,8 @@ __contract__(
requires(memory_no_alias(sm, MLDSA_CRYPTO_BYTES + mlen))
requires(memory_no_alias(smlen, sizeof(size_t)))
requires(m == sm || memory_no_alias(m, mlen))
requires(ctxlen <= MLD_MAX_BUFFER_SIZE)
requires(memory_no_alias(ctx, ctxlen))
requires(ctxlen <= 255)
requires(ctxlen == 0 || memory_no_alias(ctx, ctxlen))
requires(memory_no_alias(sk, MLDSA_CRYPTO_SECRETKEYBYTES))
assigns(memory_slice(sm, MLDSA_CRYPTO_BYTES + mlen))
assigns(object_whole(smlen))
Expand Down Expand Up @@ -787,8 +788,8 @@ __contract__(
* @param[in] ph Pointer to pre-hashed message (ignored for pure
* ML-DSA).
* @param phlen Length of pre-hashed message (ignored for pure ML-DSA).
* @param[in] ctx Pointer to context string (may be NULL).
* @param ctxlen Length of context string.
* @param[in] ctx Pointer to context string. May be NULL if ctxlen == 0.
* @param ctxlen Length of context string. Must be <= 255.
* @param hashalg Hash algorithm constant (MLD_PREHASH_NONE for pure
* ML-DSA, or MLD_PREHASH_* for HashML-DSA).
*
Expand Down
97 changes: 95 additions & 2 deletions test/src/test_alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,29 @@ void custom_free(test_ctx_t *ctx, void *p, size_t sz, const char *file,
} \
} while (0)

#define TEST_NO_ALLOC_FAILURE(test_name, call, expected_rc) \

@hanno-becker hanno-becker Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed once we reorder the checks past allocation.

do \
{ \
int rc; \
ctx->high_mark = 0; \
reset_all(ctx); \
ctx->fail_on_counter = 0; \
rc = call; \
if (rc != (expected_rc)) \
{ \
fprintf(stderr, "ERROR: %s failed with %d (expected %d)\n", test_name, \
rc, expected_rc); \
return 1; \
} \
if (ctx->alloc_counter != 0 || ctx->alloc_stack_top != 0 || \
ctx->offset != 0) \
{ \
fprintf(stderr, "ERROR: %s performed allocation before rejection\n", \
test_name); \
return 1; \
} \
} while (0)

/* Keygen tests */

#if !defined(MLD_CONFIG_NO_KEYPAIR_API)
Expand Down Expand Up @@ -377,6 +400,49 @@ static int test_sign_alloc_failure(test_ctx_t *ctx)
return 0;
}

static int test_sign_invalid_context_preflight(test_ctx_t *ctx)
{
uint8_t sig[CRYPTO_BYTES];
uint8_t ctx_bytes[256] = {0};
size_t siglen = CRYPTO_BYTES;

TEST_NO_ALLOC_FAILURE(
"mld_signature invalid context length",
mld_signature(sig, &siglen, (const uint8_t *)TEST_VECTOR_MSG,
TEST_VECTOR_MSG_LEN, ctx_bytes, sizeof(ctx_bytes),
test_vector_sk, ctx),
MLD_ERR_FAIL);
return 0;
}

static int test_sign_null_context_preflight(test_ctx_t *ctx)
{
uint8_t sig[CRYPTO_BYTES];
size_t siglen = CRYPTO_BYTES;

TEST_NO_ALLOC_FAILURE(
"mld_signature null nonempty context",
mld_signature(sig, &siglen, (const uint8_t *)TEST_VECTOR_MSG,
TEST_VECTOR_MSG_LEN, NULL, 1, test_vector_sk, ctx),
MLD_ERR_FAIL);
return 0;
}

static int test_signature_internal_invalid_externalmu_preflight(test_ctx_t *ctx)
{
uint8_t sig[CRYPTO_BYTES];
uint8_t mu[MLDSA_CRHBYTES] = {0};
uint8_t rnd[MLDSA_RNDBYTES] = {0};
size_t siglen = CRYPTO_BYTES;

TEST_NO_ALLOC_FAILURE(
"mld_signature_internal invalid external mu length",
mld_signature_internal(sig, &siglen, mu, MLDSA_CRHBYTES - 1, NULL, 0, rnd,
test_vector_sk, 1, ctx),
MLD_ERR_FAIL);
return 0;
}

static int test_sign_combined_alloc_failure(test_ctx_t *ctx)
{
uint8_t sm[CRYPTO_BYTES + TEST_VECTOR_MSG_LEN];
Expand Down Expand Up @@ -420,6 +486,28 @@ static int test_signature_pre_hash_shake256_alloc_failure(test_ctx_t *ctx)
#endif /* !MLD_CONFIG_NO_SIGN_API */

#if !defined(MLD_CONFIG_NO_VERIFY_API)
static int test_verify_invalid_siglen_preflight(test_ctx_t *ctx)
{
TEST_NO_ALLOC_FAILURE(
"mld_verify_internal invalid signature length",
mld_verify_internal(test_vector_sig, CRYPTO_BYTES - 1,
(const uint8_t *)TEST_VECTOR_MSG, TEST_VECTOR_MSG_LEN,
(const uint8_t *)TEST_VECTOR_CTX, TEST_VECTOR_CTX_LEN,
test_vector_pk, 0, ctx),
MLD_ERR_FAIL);
return 0;
}

static int test_verify_invalid_externalmu_preflight(test_ctx_t *ctx)
{
TEST_NO_ALLOC_FAILURE(
"mld_verify_internal invalid external mu length",
mld_verify_internal(test_vector_sig_extmu, CRYPTO_BYTES, test_vector_mu,
MLDSA_CRHBYTES - 1, NULL, 0, test_vector_pk, 1, ctx),
MLD_ERR_FAIL);
return 0;
}

static int test_verify_alloc_failure(test_ctx_t *ctx)
{
TEST_ALLOC_FAILURE(
Expand Down Expand Up @@ -517,16 +605,21 @@ int main(void)
r |= test_pk_from_sk_alloc_failure(&ctx);
#endif

/* Sign tests */
#if !defined(MLD_CONFIG_NO_SIGN_API)
/* Sign tests */
r |= test_sign_alloc_failure(&ctx);
r |= test_sign_invalid_context_preflight(&ctx);
r |= test_sign_null_context_preflight(&ctx);
r |= test_signature_internal_invalid_externalmu_preflight(&ctx);
r |= test_sign_combined_alloc_failure(&ctx);
r |= test_signature_extmu_alloc_failure(&ctx);
r |= test_signature_pre_hash_shake256_alloc_failure(&ctx);
#endif /* !MLD_CONFIG_NO_SIGN_API */

/* Verify tests */
#if !defined(MLD_CONFIG_NO_VERIFY_API)
/* Verify tests */
r |= test_verify_invalid_siglen_preflight(&ctx);
r |= test_verify_invalid_externalmu_preflight(&ctx);
r |= test_verify_alloc_failure(&ctx);
r |= test_verify_extmu_alloc_failure(&ctx);
r |= test_verify_pre_hash_shake256_alloc_failure(&ctx);
Expand Down
41 changes: 41 additions & 0 deletions test/src/test_mldsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include "../notrandombytes/notrandombytes.h"
Expand All @@ -19,6 +20,8 @@
MLD_API_NAMESPACE(signature_pre_hash_shake256)
#define crypto_sign_verify_pre_hash_shake256 \
MLD_API_NAMESPACE(verify_pre_hash_shake256)
#define crypto_sign_prepare_domain_separation_prefix \
MLD_API_NAMESPACE(prepare_domain_separation_prefix)
#define crypto_sign_pk_from_sk MLD_API_NAMESPACE(pk_from_sk)

#ifndef NTESTS
Expand Down Expand Up @@ -134,6 +137,42 @@ static int test_sign_unaligned(void)
return test_sign_core(pk + 1, sk + 1, sm + 1, m + 1, m2 + 1, ctx + 1);
}

static int test_sign_oversized_message_rejected(void)
{
uint8_t pk[CRYPTO_PUBLICKEYBYTES];
uint8_t sk[CRYPTO_SECRETKEYBYTES];
uint8_t sm[CRYPTO_BYTES];
uint8_t m[1] = {0};
uint8_t ctx[CTXLEN];
size_t smlen = CRYPTO_BYTES;
int rc;

CHECK(crypto_sign_keypair(pk, sk) == 0);
CHECK(randombytes(ctx, sizeof(ctx)) == 0);
MLD_CT_TESTING_SECRET(ctx, sizeof(ctx));

rc = crypto_sign(sm, &smlen, m, SIZE_MAX, ctx, sizeof(ctx), sk);
MLD_CT_TESTING_DECLASSIFY(&rc, sizeof(rc));
MLD_CT_TESTING_DECLASSIFY(&smlen, sizeof(smlen));

CHECK(rc == MLD_ERR_FAIL);
CHECK(smlen == 0);

return 0;
}

static int test_prepare_prefix_null_context_rejected(void)
{
uint8_t prefix[MLD_DOMAIN_SEPARATION_MAX_BYTES];

CHECK(crypto_sign_prepare_domain_separation_prefix(prefix, NULL, 0, NULL, 0,
MLD_PREHASH_NONE) == 2);
CHECK(crypto_sign_prepare_domain_separation_prefix(prefix, NULL, 0, NULL, 1,
MLD_PREHASH_NONE) == 0);

return 0;
}

static int test_sign_extmu(void)
{
uint8_t pk[CRYPTO_PUBLICKEYBYTES];
Expand Down Expand Up @@ -509,6 +548,8 @@ int main(void)
!defined(MLD_CONFIG_NO_VERIFY_API)
r |= test_sign();
r |= test_sign_unaligned();
r |= test_sign_oversized_message_rejected();
r |= test_prepare_prefix_null_context_rejected();
r |= test_wrong_pk();
r |= test_wrong_sig();
r |= test_wrong_ctx();
Expand Down
Loading