From 2df73ae7425b902fef8feffeccc47a8d1fd80c05 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 1 Nov 2018 12:22:27 +0300 Subject: [PATCH 1/5] mbedtls: fix possible false success in ...check_tags() helpers We should report a error when the security check of the security tag was not made. In the other case false success is possible and is not observable by the software. Technically this could lead to a security flaw. Signed-off-by: Denis V. Lunev --- library/cipher.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 752d1fea2c..2f2e03ba18 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -505,7 +505,7 @@ int mbedtls_cipher_update_ad( mbedtls_cipher_context_t *ctx, } #endif - return( 0 ); + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } #endif /* MBEDTLS_GCM_C || MBEDTLS_CHACHAPOLY_C */ @@ -1134,7 +1134,7 @@ int mbedtls_cipher_write_tag( mbedtls_cipher_context_t *ctx, } #endif - return( 0 ); + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, @@ -1161,11 +1161,8 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, } #endif /* MBEDTLS_USE_PSA_CRYPTO */ - /* Status to return on a non-authenticated algorithm. It would make sense - * to return MBEDTLS_ERR_CIPHER_INVALID_CONTEXT or perhaps - * MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, but at the time I write this our - * unit tests assume 0. */ - ret = 0; + /* Status to return on a non-authenticated algorithm. */ + ret = MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; #if defined(MBEDTLS_GCM_C) if( MBEDTLS_MODE_GCM == ctx->cipher_info->mode ) From c621a6d38fa9bcb3e892136acdb4c34f8e3cdce4 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 30 Sep 2022 17:13:35 +0100 Subject: [PATCH 2/5] Update tests to account for CIPHER_FEATURE_UNAVAILABLE on non-authenticated alg Signed-off-by: Tom Cosgrove --- tests/suites/test_suite_cipher.function | 44 +++++++++++++++++++------ 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 37468df71a..7f5b7e2901 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -453,8 +453,12 @@ void enc_dec_buf( int cipher_id, char * cipher_string, int key_len, TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx_enc ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx_dec, ad, sizeof( ad ) - i ) ); - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx_enc, ad, sizeof( ad ) - i ) ); + int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || + cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_dec, ad, sizeof(ad) - i ) ); + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_enc, ad, sizeof(ad) - i ) ); #endif block_size = mbedtls_cipher_get_block_size( &ctx_enc ); @@ -473,7 +477,7 @@ void enc_dec_buf( int cipher_id, char * cipher_string, int key_len, total_len += outlen; #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_write_tag( &ctx_enc, tag, sizeof( tag ) ) ); + TEST_EQUAL( expected, mbedtls_cipher_write_tag( &ctx_enc, tag, sizeof(tag) ) ); #endif TEST_ASSERT( total_len == length || @@ -494,7 +498,7 @@ void enc_dec_buf( int cipher_id, char * cipher_string, int key_len, total_len += outlen; #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_check_tag( &ctx_dec, tag, sizeof( tag ) ) ); + TEST_EQUAL( expected, mbedtls_cipher_check_tag( &ctx_dec, tag, sizeof(tag) ) ); #endif /* check result */ @@ -550,7 +554,11 @@ void enc_fail( int cipher_id, int pad_mode, int key_len, int length_val, TEST_ASSERT( 0 == mbedtls_cipher_set_iv( &ctx, iv, 16 ) ); TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx, NULL, 0 ) ); + int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || + cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx, NULL, 0 ) ); #endif /* encode length number of bytes from inbuf */ @@ -612,7 +620,11 @@ void dec_empty_buf( int cipher, TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx_dec ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx_dec, NULL, 0 ) ); + int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || + cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_dec, NULL, 0 ) ); #endif /* decode 0-byte string */ @@ -713,8 +725,12 @@ void enc_dec_buf_multipart( int cipher_id, int key_len, int first_length_val, TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx_enc ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx_dec, NULL, 0 ) ); - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx_enc, NULL, 0 ) ); + int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || + cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_dec, NULL, 0 ) ); + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_enc, NULL, 0 ) ); #endif block_size = mbedtls_cipher_get_block_size( &ctx_enc ); @@ -798,7 +814,11 @@ void decrypt_test_vec( int cipher_id, int pad_mode, data_t * key, TEST_ASSERT( 0 == mbedtls_cipher_set_iv( &ctx, iv->x, iv->len ) ); TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( 0 == mbedtls_cipher_update_ad( &ctx, ad->x, ad->len ) ); + int expected = ( ctx.cipher_info->mode == MBEDTLS_MODE_GCM || + ctx.cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + + TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx, ad->x, ad->len ) ); #endif /* decode buffer and check tag->x */ @@ -809,7 +829,11 @@ void decrypt_test_vec( int cipher_id, int pad_mode, data_t * key, &outlen ) ); total_len += outlen; #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) - TEST_ASSERT( tag_result == mbedtls_cipher_check_tag( &ctx, tag->x, tag->len ) ); + int tag_expected = ( ctx.cipher_info->mode == MBEDTLS_MODE_GCM || + ctx.cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + tag_result : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; + + TEST_EQUAL( tag_expected, mbedtls_cipher_check_tag( &ctx, tag->x, tag->len ) ); #endif /* check plaintext only if everything went fine */ From 51a01638286cb0da13fbc79d553e6aa47f724113 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 30 Sep 2022 18:10:58 +0100 Subject: [PATCH 3/5] Add ChangeLog entry Signed-off-by: Tom Cosgrove --- ...fix-possible-false-success-in-mbedtls_cipher_check_tag.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt diff --git a/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt b/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt new file mode 100644 index 0000000000..01492438aa --- /dev/null +++ b/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt @@ -0,0 +1,4 @@ +Changes + * Calling AEAD tag-specific functions for non-AEAD algorithms (which should not + be done - they are documented for use only by AES-GCM and ChaCha20+Poly1305) + now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE instead of success (0). From edca207260d1570dec59b96eee00c82af5acbf1f Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 14 Oct 2022 12:10:40 +0100 Subject: [PATCH 4/5] MBEDTLS_CIPHER_CHACHA20_POLY1305 is an mbedtls_cipher_type_t not an mbedtls_cipher_mode_t Signed-off-by: Tom Cosgrove --- tests/suites/test_suite_cipher.function | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 7f5b7e2901..708adb09b1 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -454,7 +454,7 @@ void enc_dec_buf( int cipher_id, char * cipher_string, int key_len, #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || - cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_dec, ad, sizeof(ad) - i ) ); @@ -555,7 +555,7 @@ void enc_fail( int cipher_id, int pad_mode, int key_len, int length_val, TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || - cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx, NULL, 0 ) ); @@ -621,7 +621,7 @@ void dec_empty_buf( int cipher, #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || - cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_dec, NULL, 0 ) ); @@ -726,7 +726,7 @@ void enc_dec_buf_multipart( int cipher_id, int key_len, int first_length_val, #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int expected = ( cipher_info->mode == MBEDTLS_MODE_GCM || - cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx_dec, NULL, 0 ) ); @@ -815,7 +815,7 @@ void decrypt_test_vec( int cipher_id, int pad_mode, data_t * key, TEST_ASSERT( 0 == mbedtls_cipher_reset( &ctx ) ); #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int expected = ( ctx.cipher_info->mode == MBEDTLS_MODE_GCM || - ctx.cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + ctx.cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? 0 : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; TEST_EQUAL( expected, mbedtls_cipher_update_ad( &ctx, ad->x, ad->len ) ); @@ -830,7 +830,7 @@ void decrypt_test_vec( int cipher_id, int pad_mode, data_t * key, total_len += outlen; #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int tag_expected = ( ctx.cipher_info->mode == MBEDTLS_MODE_GCM || - ctx.cipher_info->mode == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? + ctx.cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 ) ? tag_result : MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE; TEST_EQUAL( tag_expected, mbedtls_cipher_check_tag( &ctx, tag->x, tag->len ) ); From 0f0b54851944c7c4523061810711850e8851ad73 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Wed, 16 Nov 2022 14:23:51 +0000 Subject: [PATCH 5/5] Limit ChangeLog entry to 80 characters Signed-off-by: Tom Cosgrove --- ...-possible-false-success-in-mbedtls_cipher_check_tag.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt b/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt index 01492438aa..1f9e0aa350 100644 --- a/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt +++ b/ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt @@ -1,4 +1,5 @@ Changes - * Calling AEAD tag-specific functions for non-AEAD algorithms (which should not - be done - they are documented for use only by AES-GCM and ChaCha20+Poly1305) - now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE instead of success (0). + * Calling AEAD tag-specific functions for non-AEAD algorithms (which + should not be done - they are documented for use only by AES-GCM and + ChaCha20+Poly1305) now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE + instead of success (0).