From bf4b5ed7a4e02358cb008bb43c20f5f3c309b7c1 Mon Sep 17 00:00:00 2001 From: Chien Wong Date: Mon, 22 Jan 2024 20:43:54 +0800 Subject: [PATCH 1/9] Add back restriction on AD length of GCM Fixes: bd513bb53d80276431161e5a64a2ae61740c4e68 Signed-off-by: Chien Wong --- library/gcm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/gcm.c b/library/gcm.c index c677ca4d7..b31003f83 100644 --- a/library/gcm.c +++ b/library/gcm.c @@ -354,9 +354,12 @@ int mbedtls_gcm_update_ad(mbedtls_gcm_context *ctx, { const unsigned char *p; size_t use_len, offset; + uint64_t new_add_len; - /* IV is limited to 2^64 bits, so 2^61 bytes */ - if ((uint64_t) add_len >> 61 != 0) { + /* AD is limited to 2^64 bits, ie 2^61 bytes + * Also check for possible overflow */ + new_add_len = ctx->add_len + add_len; + if (new_add_len < ctx->add_len || new_add_len >> 61 != 0) { return MBEDTLS_ERR_GCM_BAD_INPUT; } From 858bc65d7485b8af9c49e96d0cf0bf803606a120 Mon Sep 17 00:00:00 2001 From: Chien Wong Date: Mon, 22 Jan 2024 20:47:26 +0800 Subject: [PATCH 2/9] Add comment on impossible overflows Signed-off-by: Chien Wong --- library/gcm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/gcm.c b/library/gcm.c index b31003f83..337145b71 100644 --- a/library/gcm.c +++ b/library/gcm.c @@ -542,6 +542,9 @@ int mbedtls_gcm_finish(mbedtls_gcm_context *ctx, (void) output_size; *output_length = 0; + /* Total length is restricted to 2^39 - 256 bits, ie 2^36 - 2^5 bytes + * and AD length is restricted to 2^64 bits, ie 2^61 bytes so neither of + * the two multiplications would overflow. */ orig_len = ctx->len * 8; orig_add_len = ctx->add_len * 8; From 019c2a7817c702f5d7826bc14badf2a5c7a36c4d Mon Sep 17 00:00:00 2001 From: Chien Wong Date: Tue, 23 Jan 2024 21:38:06 +0800 Subject: [PATCH 3/9] Handle sizeof(size_t) > sizeof(uint64_t) Signed-off-by: Chien Wong --- library/gcm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/library/gcm.c b/library/gcm.c index 337145b71..033cb5901 100644 --- a/library/gcm.c +++ b/library/gcm.c @@ -358,7 +358,12 @@ int mbedtls_gcm_update_ad(mbedtls_gcm_context *ctx, /* AD is limited to 2^64 bits, ie 2^61 bytes * Also check for possible overflow */ - new_add_len = ctx->add_len + add_len; +#if SIZE_MAX > 0xFFFFFFFFFFFFFFFFULL + if (add_len > 0xFFFFFFFFFFFFFFFFULL) { + return MBEDTLS_ERR_GCM_BAD_INPUT; + } +#endif + new_add_len = ctx->add_len + (uint64_t) add_len; if (new_add_len < ctx->add_len || new_add_len >> 61 != 0) { return MBEDTLS_ERR_GCM_BAD_INPUT; } From 99ff1f505b706006c75c3d2047e7b19ed5f6ac81 Mon Sep 17 00:00:00 2001 From: Chien Wong Date: Wed, 24 Jan 2024 20:44:01 +0800 Subject: [PATCH 4/9] Add test cases on GCM AD, input, IV length Signed-off-by: Chien Wong --- tests/suites/test_suite_gcm.function | 107 ++++++++++++++++++++++++++ tests/suites/test_suite_gcm.misc.data | 12 +++ 2 files changed, 119 insertions(+) diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index 599c9266e..07a6e4593 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -153,6 +153,20 @@ exit: mbedtls_free(output); } +static void gcm_reset_ctx(mbedtls_gcm_context *ctx, const uint8_t *key, + size_t key_bits, const uint8_t *iv, size_t iv_len, + int starts_ret) +{ + int mode = MBEDTLS_GCM_ENCRYPT; + mbedtls_cipher_id_t valid_cipher = MBEDTLS_CIPHER_ID_AES; + + mbedtls_gcm_init(ctx); + TEST_EQUAL(mbedtls_gcm_setkey(ctx, valid_cipher, key, key_bits), 0); + TEST_EQUAL(starts_ret, mbedtls_gcm_starts(ctx, mode, iv, iv_len)); +exit: + /* empty */ +} + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -478,6 +492,99 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void gcm_invalid_iv_len(void) +{ + mbedtls_gcm_context ctx; + uint8_t b16[16] = { 0 }; + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, 0, MBEDTLS_ERR_GCM_BAD_INPUT); + mbedtls_gcm_free(&ctx); + +#if SIZE_MAX >= UINT64_MAX + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, 1ULL << 61, MBEDTLS_ERR_GCM_BAD_INPUT); + mbedtls_gcm_free(&ctx); +#endif + + goto exit; /* To suppress error that exit is defined but not used */ +exit: + /* empty */ +} +/* END_CASE */ + +/* BEGIN_CASE */ +/* + * Test if GCM rejects total ad_len >= 2^61 bytes. + * Also test if GCM handles potential total ad_len overflow properly. + + * Only testable on platforms where sizeof(size_t) >= 8. + */ +void gcm_add_len_too_long(void) +{ +#if SIZE_MAX >= UINT64_MAX + mbedtls_gcm_context ctx; + uint8_t b16[16] = { 0 }; + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, 1ULL << 61), + MBEDTLS_ERR_GCM_BAD_INPUT); + mbedtls_gcm_free(&ctx); + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, 1), 0); + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, (1ULL << 61) - 1), + MBEDTLS_ERR_GCM_BAD_INPUT); + mbedtls_gcm_free(&ctx); + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, 1), 0); + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, UINT64_MAX), MBEDTLS_ERR_GCM_BAD_INPUT); + +exit: + mbedtls_gcm_free(&ctx); +#endif +} +/* END_CASE */ + +/* BEGIN_CASE */ +/* + * Test if GCM rejects total input length > 2^36 - 32 bytes. + * Also test if GCM handles potential total input length overflow properly. + + * Only testable on platforms where sizeof(size_t) >= 8. + */ +void gcm_input_len_too_long(void) +{ +#if SIZE_MAX >= UINT64_MAX + mbedtls_gcm_context ctx; + uint8_t b16[16] = { 0 }; + size_t out_len; + uint64_t len_max = (1ULL << 36) - 32; + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max + 1, b16, len_max + 1, + &out_len), + MBEDTLS_ERR_GCM_BAD_INPUT); + mbedtls_gcm_free(&ctx); + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, b16, 1, &out_len), 0); + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max, b16, len_max, &out_len), + MBEDTLS_ERR_GCM_BAD_INPUT); + mbedtls_gcm_free(&ctx); + + gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, b16, 1, &out_len), 0); + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, UINT64_MAX, b16, UINT64_MAX, + &out_len), + MBEDTLS_ERR_GCM_BAD_INPUT); + +exit: + mbedtls_gcm_free(&ctx); +#endif +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST:MBEDTLS_CCM_GCM_CAN_AES */ void gcm_selftest() { diff --git a/tests/suites/test_suite_gcm.misc.data b/tests/suites/test_suite_gcm.misc.data index f22b7a3b7..57f05caf5 100644 --- a/tests/suites/test_suite_gcm.misc.data +++ b/tests/suites/test_suite_gcm.misc.data @@ -1,2 +1,14 @@ GCM - Invalid parameters gcm_invalid_param: + +GCM - Invalid IV length +depends_on:MBEDTLS_AES_C:MBEDTLS_GCM_C +gcm_invalid_iv_len: + +GCM - Additional data length too long +depends_on:MBEDTLS_AES_C:MBEDTLS_GCM_C +gcm_add_len_too_long: + +GCM - Input length too long +depends_on:MBEDTLS_AES_C:MBEDTLS_GCM_C +gcm_input_len_too_long: From 92c17c456c6cef1058598062b207c032d8b9fad3 Mon Sep 17 00:00:00 2001 From: Chien Wong Date: Thu, 25 Jan 2024 19:11:03 +0800 Subject: [PATCH 5/9] Use separate input/output buffer. Explain why error is expected Signed-off-by: Chien Wong --- tests/suites/test_suite_gcm.function | 43 ++++++++++++++++------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index 07a6e4593..e23d8d03d 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -493,15 +493,20 @@ exit: /* END_CASE */ /* BEGIN_CASE */ +/* NISP SP 800-38D, Section 5.2.1.1 requires that bit length of IV should + * satisfy 1 <= bit_len(IV) <= 2^64 - 1. */ void gcm_invalid_iv_len(void) { mbedtls_gcm_context ctx; uint8_t b16[16] = { 0 }; + // Invalid IV length 0 gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, 0, MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); + // Only testable on platforms where sizeof(size_t) >= 8. #if SIZE_MAX >= UINT64_MAX + // Invalid IV length 2^61 gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, 1ULL << 61, MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); #endif @@ -513,30 +518,31 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -/* - * Test if GCM rejects total ad_len >= 2^61 bytes. - * Also test if GCM handles potential total ad_len overflow properly. - - * Only testable on platforms where sizeof(size_t) >= 8. - */ void gcm_add_len_too_long(void) { + // Only testable on platforms where sizeof(size_t) >= 8. #if SIZE_MAX >= UINT64_MAX mbedtls_gcm_context ctx; uint8_t b16[16] = { 0 }; + /* NISP SP 800-38D, Section 5.2.1.1 requires that bit length of AD should + * be <= 2^64 - 1, ie < 2^64. This is the minimum invalid length in bytes. */ + uint64_t len_max = 1ULL << 61; gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); - TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, 1ULL << 61), + // Feed AD that just exceeds the length limit + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, len_max), MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + // Feed AD that just exceeds the length limit in two calls TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, 1), 0); - TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, (1ULL << 61) - 1), + TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, len_max - 1), MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); + // Test if potential total AD length overflow is handled properly TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, 1), 0); TEST_EQUAL(mbedtls_gcm_update_ad(&ctx, b16, UINT64_MAX), MBEDTLS_ERR_GCM_BAD_INPUT); @@ -547,35 +553,36 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -/* - * Test if GCM rejects total input length > 2^36 - 32 bytes. - * Also test if GCM handles potential total input length overflow properly. - - * Only testable on platforms where sizeof(size_t) >= 8. - */ void gcm_input_len_too_long(void) { + // Only testable on platforms where sizeof(size_t) >= 8 #if SIZE_MAX >= UINT64_MAX mbedtls_gcm_context ctx; uint8_t b16[16] = { 0 }; + uint8_t out[1]; size_t out_len; + /* NISP SP 800-38D, Section 5.2.1.1 requires that bit length of input should + * be <= 2^39 - 256. This is the maximum valid length in bytes. */ uint64_t len_max = (1ULL << 36) - 32; gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); - TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max + 1, b16, len_max + 1, + // Feed input that just exceeds the length limit + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max + 1, out, len_max + 1, &out_len), MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); - TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, b16, 1, &out_len), 0); + // Feed input that just exceeds the length limit in two calls + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, out, 1, &out_len), 0); TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max, b16, len_max, &out_len), MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); - TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, b16, 1, &out_len), 0); - TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, UINT64_MAX, b16, UINT64_MAX, + // Test if potential total input length overflow is handled properly + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, out, 1, &out_len), 0); + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, UINT64_MAX, out, UINT64_MAX, &out_len), MBEDTLS_ERR_GCM_BAD_INPUT); From ef56795fd273b36a712dde7987a10fd54065ec79 Mon Sep 17 00:00:00 2001 From: Chien Wong Date: Thu, 25 Jan 2024 19:22:50 +0800 Subject: [PATCH 6/9] Fix 1 forgotten separate input/output buffer Signed-off-by: Chien Wong --- tests/suites/test_suite_gcm.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index e23d8d03d..dac2a5df2 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -575,7 +575,7 @@ void gcm_input_len_too_long(void) gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, sizeof(b16), 0); // Feed input that just exceeds the length limit in two calls TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, 1, out, 1, &out_len), 0); - TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max, b16, len_max, &out_len), + TEST_EQUAL(mbedtls_gcm_update(&ctx, b16, len_max, out, len_max, &out_len), MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); From 6823247376c67aaf52badb20c5bcad69bf3a53b4 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 31 Jan 2024 15:59:06 +0000 Subject: [PATCH 7/9] Fix compile warning in tests Signed-off-by: Dave Rodgman --- tests/suites/test_suite_gcm.function | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index dac2a5df2..0af4209f4 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -165,6 +165,7 @@ static void gcm_reset_ctx(mbedtls_gcm_context *ctx, const uint8_t *key, TEST_EQUAL(starts_ret, mbedtls_gcm_starts(ctx, mode, iv, iv_len)); exit: /* empty */ + return; } /* END_HEADER */ @@ -514,6 +515,7 @@ void gcm_invalid_iv_len(void) goto exit; /* To suppress error that exit is defined but not used */ exit: /* empty */ + return; } /* END_CASE */ From ba8e9addd9c968ae25fa251dc0c40459f9c555f9 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 1 Feb 2024 13:54:46 +0000 Subject: [PATCH 8/9] Fix test dependencies Signed-off-by: Dave Rodgman --- tests/suites/test_suite_gcm.misc.data | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_gcm.misc.data b/tests/suites/test_suite_gcm.misc.data index 57f05caf5..108630ee8 100644 --- a/tests/suites/test_suite_gcm.misc.data +++ b/tests/suites/test_suite_gcm.misc.data @@ -2,13 +2,13 @@ GCM - Invalid parameters gcm_invalid_param: GCM - Invalid IV length -depends_on:MBEDTLS_AES_C:MBEDTLS_GCM_C +depends_on:MBEDTLS_GCM_C:MBEDTLS_CCM_GCM_CAN_AES gcm_invalid_iv_len: GCM - Additional data length too long -depends_on:MBEDTLS_AES_C:MBEDTLS_GCM_C +depends_on:MBEDTLS_GCM_C:MBEDTLS_CCM_GCM_CAN_AES gcm_add_len_too_long: GCM - Input length too long -depends_on:MBEDTLS_AES_C:MBEDTLS_GCM_C +depends_on:MBEDTLS_GCM_C:MBEDTLS_CCM_GCM_CAN_AES gcm_input_len_too_long: From 12285c5c7c658a92cecc05a095e36d8d256d828a Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 2 Feb 2024 17:52:41 +0000 Subject: [PATCH 9/9] Add calls to BLOCK_CIPHER_PSA_INIT / BLOCK_CIPHER_PSA_DONE Signed-off-by: Dave Rodgman --- tests/suites/test_suite_gcm.function | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index 0af4209f4..8bb7b8b8e 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -499,8 +499,11 @@ exit: void gcm_invalid_iv_len(void) { mbedtls_gcm_context ctx; + mbedtls_gcm_init(&ctx); uint8_t b16[16] = { 0 }; + BLOCK_CIPHER_PSA_INIT(); + // Invalid IV length 0 gcm_reset_ctx(&ctx, b16, sizeof(b16) * 8, b16, 0, MBEDTLS_ERR_GCM_BAD_INPUT); mbedtls_gcm_free(&ctx); @@ -514,8 +517,8 @@ void gcm_invalid_iv_len(void) goto exit; /* To suppress error that exit is defined but not used */ exit: - /* empty */ - return; + mbedtls_gcm_free(&ctx); + BLOCK_CIPHER_PSA_DONE(); } /* END_CASE */ @@ -525,7 +528,10 @@ void gcm_add_len_too_long(void) // Only testable on platforms where sizeof(size_t) >= 8. #if SIZE_MAX >= UINT64_MAX mbedtls_gcm_context ctx; + mbedtls_gcm_init(&ctx); uint8_t b16[16] = { 0 }; + BLOCK_CIPHER_PSA_INIT(); + /* NISP SP 800-38D, Section 5.2.1.1 requires that bit length of AD should * be <= 2^64 - 1, ie < 2^64. This is the minimum invalid length in bytes. */ uint64_t len_max = 1ULL << 61; @@ -550,6 +556,7 @@ void gcm_add_len_too_long(void) exit: mbedtls_gcm_free(&ctx); + BLOCK_CIPHER_PSA_DONE(); #endif } /* END_CASE */ @@ -563,6 +570,9 @@ void gcm_input_len_too_long(void) uint8_t b16[16] = { 0 }; uint8_t out[1]; size_t out_len; + mbedtls_gcm_init(&ctx); + BLOCK_CIPHER_PSA_INIT(); + /* NISP SP 800-38D, Section 5.2.1.1 requires that bit length of input should * be <= 2^39 - 256. This is the maximum valid length in bytes. */ uint64_t len_max = (1ULL << 36) - 32; @@ -590,6 +600,7 @@ void gcm_input_len_too_long(void) exit: mbedtls_gcm_free(&ctx); + BLOCK_CIPHER_PSA_DONE(); #endif } /* END_CASE */