From 738d2310a7d300faf3e16faff35b7745a6a50157 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 5 Feb 2021 17:49:23 +0000 Subject: [PATCH 01/15] Attempt to make Base64 table access constant flow Add constant flow table access code, and use that exclusively to access the base64 lookup table Signed-off-by: Paul Elliott --- library/base64.c | 71 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 10 deletions(-) diff --git a/library/base64.c b/library/base64.c index bfafb0535..49a0612d2 100644 --- a/library/base64.c +++ b/library/base64.c @@ -96,6 +96,44 @@ static const unsigned char base64_dec_map[128] = #define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ +static void mbedtls_base64_cond_assign(unsigned char * dest, const unsigned char * const src, + unsigned char condition) +{ + /* make sure assign is 0 or 1 in a time-constant manner */ + condition = (condition | (unsigned char)-condition) >> 7; + + *dest = ( *dest ) * ( 1 - condition ) + ( *src ) * condition; +} + +/* + * Constant time check for equality +*/ +static unsigned char mbedtls_base64_eq(uint32_t in_a, uint32_t in_b) +{ + uint32_t difference = in_a ^ in_b; + + difference |= -difference; + difference >>= 31; + return (unsigned char) ( 1 ^ difference ); +} + +/* + * Constant time lookup into table. +*/ +static unsigned char mbedtls_base64_table_lookup(const unsigned char * const table, + const size_t table_size, const size_t table_index) +{ + size_t i; + unsigned char result = 0; + + for( i = 0; i < table_size; ++i ) + { + mbedtls_base64_cond_assign(&result, &table[i], mbedtls_base64_eq(i, table_index)); + } + + return result; +} + /* * Encode a buffer into base64 format */ @@ -136,10 +174,17 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, C2 = *src++; C3 = *src++; - *p++ = base64_enc_map[(C1 >> 2) & 0x3F]; - *p++ = base64_enc_map[(((C1 & 3) << 4) + (C2 >> 4)) & 0x3F]; - *p++ = base64_enc_map[(((C2 & 15) << 2) + (C3 >> 6)) & 0x3F]; - *p++ = base64_enc_map[C3 & 0x3F]; + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( ( C1 >> 2 ) & 0x3F ) ); + + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ) ); + + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( ( ( ( C2 & 15 ) << 2 ) + ( C3 >> 6 ) ) & 0x3F ) ); + + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( C3 & 0x3F ) ); } if( i < slen ) @@ -147,11 +192,15 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, C1 = *src++; C2 = ( ( i + 1 ) < slen ) ? *src++ : 0; - *p++ = base64_enc_map[(C1 >> 2) & 0x3F]; - *p++ = base64_enc_map[(((C1 & 3) << 4) + (C2 >> 4)) & 0x3F]; + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( ( C1 >> 2 ) & 0x3F ) ); + + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ) ); if( ( i + 1 ) < slen ) - *p++ = base64_enc_map[((C2 & 15) << 2) & 0x3F]; + *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), + ( ( ( C2 & 15 ) << 2 ) & 0x3F ) ); else *p++ = '='; *p++ = '='; @@ -202,7 +251,8 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( src[i] == '=' && ++j > 2 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - if( src[i] > 127 || base64_dec_map[src[i]] == 127 ) + if( src[i] > 127 || + mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), src[i] ) == 127 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); if( base64_dec_map[src[i]] < 64 && j != 0 ) @@ -235,8 +285,9 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( *src == '\r' || *src == '\n' || *src == ' ' ) continue; - j -= ( base64_dec_map[*src] == 64 ); - x = ( x << 6 ) | ( base64_dec_map[*src] & 0x3F ); + j -= ( mbedtls_base64_table_lookup(base64_dec_map, sizeof( base64_dec_map ), *src ) == 64 ); + x = ( x << 6 ) | + ( mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), *src ) & 0x3F ); if( ++n == 4 ) { From 69b904b6796df02aacfdd8303a2e9b68de482e01 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 24 Feb 2021 15:32:42 +0000 Subject: [PATCH 02/15] First pass at constant flow tests for base64 This contains working CF tests for encode, however I have not yet got decode to pass the tests. Signed-off-by: Paul Elliott --- tests/suites/test_suite_base64.function | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 3a8bf430f..d7e888f40 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -13,13 +13,18 @@ void mbedtls_base64_encode( char * src_string, char * dst_string, { unsigned char src_str[1000]; unsigned char dst_str[1000]; - size_t len; + size_t len, src_len; memset(src_str, 0x00, 1000); memset(dst_str, 0x00, 1000); strncpy( (char *) src_str, src_string, sizeof(src_str) - 1 ); - TEST_ASSERT( mbedtls_base64_encode( dst_str, dst_buf_size, &len, src_str, strlen( (char *) src_str ) ) == result ); + src_len = strlen( (char *) src_str ); + + TEST_CF_SECRET( src_str, sizeof( src_str ) ); + TEST_ASSERT( mbedtls_base64_encode( dst_str, dst_buf_size, &len, src_str, src_len) == result ); + TEST_CF_PUBLIC( src_str, sizeof( src_str ) ); + if( result == 0 ) { TEST_ASSERT( strcmp( (char *) dst_str, dst_string ) == 0 ); @@ -57,7 +62,10 @@ void base64_encode_hex( data_t * src, char * dst, int dst_buf_size, res = zero_alloc( dst_buf_size ); + TEST_CF_SECRET( src->x, src->len ); TEST_ASSERT( mbedtls_base64_encode( res, dst_buf_size, &len, src->x, src->len ) == result ); + TEST_CF_PUBLIC( src->x, src->len ); + if( result == 0 ) { TEST_ASSERT( len == strlen( dst ) ); From 8d265f75a4ceb1d1e7b5bd4c532c804629311d76 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 25 Feb 2021 10:47:56 +0000 Subject: [PATCH 03/15] Add Changelog entry Signed-off-by: Paul Elliott --- ChangeLog.d/make_base64_table_access_constant_flow.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/make_base64_table_access_constant_flow.txt diff --git a/ChangeLog.d/make_base64_table_access_constant_flow.txt b/ChangeLog.d/make_base64_table_access_constant_flow.txt new file mode 100644 index 000000000..a11938912 --- /dev/null +++ b/ChangeLog.d/make_base64_table_access_constant_flow.txt @@ -0,0 +1,4 @@ +Security + * Guard against strong local side channel attack against base64 tables by + making access aceess to them use constant flow code. + From f13a47bbb2f1dbcc718d9ddd5c7a4acc3232e045 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 25 Feb 2021 12:28:49 +0000 Subject: [PATCH 04/15] Fixes for MSVC warnings Also added a couple of missing comment blocks. Signed-off-by: Paul Elliott --- library/base64.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/library/base64.c b/library/base64.c index 49a0612d2..201055b67 100644 --- a/library/base64.c +++ b/library/base64.c @@ -96,6 +96,9 @@ static const unsigned char base64_dec_map[128] = #define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ +/* + * Constant flow conditional assignment +*/ static void mbedtls_base64_cond_assign(unsigned char * dest, const unsigned char * const src, unsigned char condition) { @@ -106,19 +109,31 @@ static void mbedtls_base64_cond_assign(unsigned char * dest, const unsigned char } /* - * Constant time check for equality + * Constant flow check for equality */ -static unsigned char mbedtls_base64_eq(uint32_t in_a, uint32_t in_b) +static unsigned char mbedtls_base64_eq(size_t in_a, size_t in_b) { uint32_t difference = in_a ^ in_b; + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + difference |= -difference; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + difference >>= 31; return (unsigned char) ( 1 ^ difference ); } /* - * Constant time lookup into table. + * Constant flow lookup into table. */ static unsigned char mbedtls_base64_table_lookup(const unsigned char * const table, const size_t table_size, const size_t table_index) From 86cb928e54c117b68aac339735e07f1dd0a514b9 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Mon, 1 Mar 2021 17:49:42 +0000 Subject: [PATCH 05/15] Fix incorrect assumptions about the size of size_t Signed-off-by: Paul Elliott --- library/base64.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/base64.c b/library/base64.c index 201055b67..3b2b35dae 100644 --- a/library/base64.c +++ b/library/base64.c @@ -113,7 +113,7 @@ static void mbedtls_base64_cond_assign(unsigned char * dest, const unsigned char */ static unsigned char mbedtls_base64_eq(size_t in_a, size_t in_b) { - uint32_t difference = in_a ^ in_b; + size_t difference = in_a ^ in_b; /* MSVC has a warning about unary minus on unsigned integer types, * but this is well-defined and precisely what we want to do here. */ @@ -128,7 +128,9 @@ static unsigned char mbedtls_base64_eq(size_t in_a, size_t in_b) #pragma warning( pop ) #endif - difference >>= 31; + /* cope with the varying size of size_t per platform */ + difference >>= ( sizeof( difference ) * 8 - 1 ); + return (unsigned char) ( 1 ^ difference ); } From 6a66737433ea4a1fe837b27be463d613b341c1c9 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Mon, 1 Mar 2021 18:33:09 +0000 Subject: [PATCH 06/15] Optimise unneccesary cf table accesses away Also fix missed bare access of base_64_dec_map Signed-off-by: Paul Elliott --- library/base64.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/library/base64.c b/library/base64.c index 3b2b35dae..0bc9437a7 100644 --- a/library/base64.c +++ b/library/base64.c @@ -238,6 +238,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, size_t i, n; uint32_t j, x; unsigned char *p; + unsigned char dec_map_lookup; /* First pass: check for validity and get output length */ for( i = n = j = 0; i < slen; i++ ) @@ -268,11 +269,12 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( src[i] == '=' && ++j > 2 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - if( src[i] > 127 || - mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), src[i] ) == 127 ) + dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), src[i] ); + + if( src[i] > 127 || dec_map_lookup == 127 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - if( base64_dec_map[src[i]] < 64 && j != 0 ) + if( dec_map_lookup < 64 && j != 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); n++; @@ -302,9 +304,10 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( *src == '\r' || *src == '\n' || *src == ' ' ) continue; - j -= ( mbedtls_base64_table_lookup(base64_dec_map, sizeof( base64_dec_map ), *src ) == 64 ); - x = ( x << 6 ) | - ( mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), *src ) & 0x3F ); + dec_map_lookup = mbedtls_base64_table_lookup(base64_dec_map, sizeof( base64_dec_map ), *src ); + + j -= ( dec_map_lookup == 64 ); + x = ( x << 6 ) | ( dec_map_lookup & 0x3F ); if( ++n == 4 ) { From e8ee0f1a94da6bdff4fb3611c07db878779d0f9e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Mon, 1 Mar 2021 19:15:43 +0000 Subject: [PATCH 07/15] Fix Non CF access to table in base64 decrypt Signed-off-by: Paul Elliott --- library/base64.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/library/base64.c b/library/base64.c index 0bc9437a7..c16d71164 100644 --- a/library/base64.c +++ b/library/base64.c @@ -97,9 +97,9 @@ static const unsigned char base64_dec_map[128] = #define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ /* - * Constant flow conditional assignment + * Constant flow conditional assignment to unsigned char */ -static void mbedtls_base64_cond_assign(unsigned char * dest, const unsigned char * const src, +static void mbedtls_base64_cond_assign_uchar(unsigned char * dest, const unsigned char * const src, unsigned char condition) { /* make sure assign is 0 or 1 in a time-constant manner */ @@ -108,6 +108,19 @@ static void mbedtls_base64_cond_assign(unsigned char * dest, const unsigned char *dest = ( *dest ) * ( 1 - condition ) + ( *src ) * condition; } +/* + * Constant flow conditional assignment to uint_32 +*/ +static void mbedtls_base64_cond_assign_uint32(uint32_t * dest, const uint32_t src, + unsigned char condition) +{ + /* make sure assign is 0 or 1 in a time-constant manner */ + condition = (condition | (unsigned char)-condition) >> 7; + + *dest = ( *dest ) * ( 1 - condition ) + ( src ) * condition; +} + + /* * Constant flow check for equality */ @@ -145,7 +158,7 @@ static unsigned char mbedtls_base64_table_lookup(const unsigned char * const tab for( i = 0; i < table_size; ++i ) { - mbedtls_base64_cond_assign(&result, &table[i], mbedtls_base64_eq(i, table_index)); + mbedtls_base64_cond_assign_uchar(&result, &table[i], mbedtls_base64_eq(i, table_index)); } return result; @@ -306,7 +319,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, dec_map_lookup = mbedtls_base64_table_lookup(base64_dec_map, sizeof( base64_dec_map ), *src ); - j -= ( dec_map_lookup == 64 ); + mbedtls_base64_cond_assign_uint32( &j, j - 1, mbedtls_base64_eq( dec_map_lookup, 64 ) ); x = ( x << 6 ) | ( dec_map_lookup & 0x3F ); if( ++n == 4 ) From 7dac825a3c3c40e929b1e244eaa7805d4ae15587 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 2 Mar 2021 22:44:37 +0000 Subject: [PATCH 08/15] Add further more rigorous tests for base64 Original author was gilles.peskine@arm.com Signed-off-by: Paul Elliott --- tests/suites/test_suite_base64.data | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index da99ffa87..39b9ca7f6 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -151,6 +151,20 @@ base64_encode_hex:"0102030405060708":"AQIDBAUGBwg=":13:0 Base64 encode hex #4 base64_encode_hex:"01020304050607":"AQIDBAUGBw==":13:0 +# Rotate the bytes around so that they end up at each offset modulo 3 in +# successive test cases. +Base64 encode hex all valid input bytes #0 +base64_encode_hex:"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f909192939495969798999a9b9c9d9e9fa0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebfc0c1c2c3c4c5c6c7c8c9cacbcccdcecfd0d1d2d3d4d5d6d7d8d9dadbdcdddedfe0e1e2e3e4e5e6e7e8e9eaebecedeeeff0f1f2f3f4f5f6f7f8f9fafbfcfdfeff":"AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5fYGFiY2RlZmdoaWprbG1ub3BxcnN0dXZ3eHl6e3x9fn+AgYKDhIWGh4iJiouMjY6PkJGSk5SVlpeYmZqbnJ2en6ChoqOkpaanqKmqq6ytrq+wsbKztLW2t7i5uru8vb6/wMHCw8TFxsfIycrLzM3Oz9DR0tPU1dbX2Nna29zd3t/g4eLj5OXm5+jp6uvs7e7v8PHy8/T19vf4+fr7/P3+/w==":345:0 + +Base64 encode hex all valid input bytes #1 +base64_encode_hex:"0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f909192939495969798999a9b9c9d9e9fa0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebfc0c1c2c3c4c5c6c7c8c9cacbcccdcecfd0d1d2d3d4d5d6d7d8d9dadbdcdddedfe0e1e2e3e4e5e6e7e8e9eaebecedeeeff0f1f2f3f4f5f6f7f8f9fafbfcfdfeff00":"AQIDBAUGBwgJCgsMDQ4PEBESExQVFhcYGRobHB0eHyAhIiMkJSYnKCkqKywtLi8wMTIzNDU2Nzg5Ojs8PT4/QEFCQ0RFRkdISUpLTE1OT1BRUlNUVVZXWFlaW1xdXl9gYWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXp7fH1+f4CBgoOEhYaHiImKi4yNjo+QkZKTlJWWl5iZmpucnZ6foKGio6SlpqeoqaqrrK2ur7CxsrO0tba3uLm6u7y9vr/AwcLDxMXGx8jJysvMzc7P0NHS09TV1tfY2drb3N3e3+Dh4uPk5ebn6Onq6+zt7u/w8fLz9PX29/j5+vv8/f7/AA==":345:0 + +Base64 encode hex all valid input bytes #2 +base64_encode_hex:"02030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f909192939495969798999a9b9c9d9e9fa0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebfc0c1c2c3c4c5c6c7c8c9cacbcccdcecfd0d1d2d3d4d5d6d7d8d9dadbdcdddedfe0e1e2e3e4e5e6e7e8e9eaebecedeeeff0f1f2f3f4f5f6f7f8f9fafbfcfdfeff0001":"AgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fICEiIyQlJicoKSorLC0uLzAxMjM0NTY3ODk6Ozw9Pj9AQUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVpbXF1eX2BhYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ent8fX5/gIGCg4SFhoeIiYqLjI2Oj5CRkpOUlZaXmJmam5ydnp+goaKjpKWmp6ipqqusra6vsLGys7S1tre4ubq7vL2+v8DBwsPExcbHyMnKy8zNzs/Q0dLT1NXW19jZ2tvc3d7f4OHi4+Tl5ufo6err7O3u7/Dx8vP09fb3+Pn6+/z9/v8AAQ==":345:0 + +Base64 decode all valid output characters at all offsets +base64_encode_hex:"00108310518720928b30d38f41149351559761969b71d79f8218a39259a7a29aabb2dbafc31cb3d35db7e39ebbf3dfbff800420c41461c824a2cc34e3d04524d45565d865a6dc75e7e08628e49669e8a6aaecb6ebf0c72cf4d76df8e7aefcf7effe00108310518720928b30d38f41149351559761969b71d79f8218a39259a7a29aabb2dbafc31cb3d35db7e39ebbf3dfbff800420c41461c824a2cc34e3d04524d45565d865a6dc75e7e08628e49669e8a6aaecb6ebf0c72cf4d76df8e7aefcf7efd0":"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/Q":261:0 + Base64 decode hex #1 base64_decode_hex:"AQIDBAUGBwgJ":"010203040506070809":9:0 @@ -166,6 +180,9 @@ base64_decode_hex:"AQIDBAUGBw==":"01020304050607":7:0 Base64 decode hex #5 (buffer too small) base64_decode_hex:"AQIDBAUGBw==":"01020304050607":6:MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL +Base64 decode all valid input characters at all offsets +base64_decode_hex:"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/Q":"00108310518720928b30d38f41149351559761969b71d79f8218a39259a7a29aabb2dbafc31cb3d35db7e39ebbf3dfbff800420c41461c824a2cc34e3d04524d45565d865a6dc75e7e08628e49669e8a6aaecb6ebf0c72cf4d76df8e7aefcf7effe00108310518720928b30d38f41149351559761969b71d79f8218a39259a7a29aabb2dbafc31cb3d35db7e39ebbf3dfbff800420c41461c824a2cc34e3d04524d45565d865a6dc75e7e08628e49669e8a6aaecb6ebf0c72cf4d76df8e7aefcf7efd0":195:0 + Base64 Selftest depends_on:MBEDTLS_SELF_TEST base64_selftest: From 4c71707987112b3254db23e10068d27ca3b585da Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 2 Mar 2021 22:48:40 +0000 Subject: [PATCH 09/15] Prevent false positive CF Test Failures Marked dirty memory ends up in the result buffer after encoding (due to the input having been marked dirty), and then the final comparison to make sure that we got what we expected was triggering the constant flow checker. Signed-off-by: Paul Elliott --- tests/suites/test_suite_base64.function | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index d7e888f40..0185f3b24 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -25,6 +25,10 @@ void mbedtls_base64_encode( char * src_string, char * dst_string, TEST_ASSERT( mbedtls_base64_encode( dst_str, dst_buf_size, &len, src_str, src_len) == result ); TEST_CF_PUBLIC( src_str, sizeof( src_str ) ); + /* dest_str will have had tainted data copied to it, prevent the TEST_ASSERT below from triggering + CF failures by unmarking it. */ + TEST_CF_PUBLIC( dst_str, len ); + if( result == 0 ) { TEST_ASSERT( strcmp( (char *) dst_str, dst_string ) == 0 ); @@ -66,6 +70,10 @@ void base64_encode_hex( data_t * src, char * dst, int dst_buf_size, TEST_ASSERT( mbedtls_base64_encode( res, dst_buf_size, &len, src->x, src->len ) == result ); TEST_CF_PUBLIC( src->x, src->len ); + /* res will have had tainted data copied to it, prevent the TEST_ASSERT below from triggering + CF failures by unmarking it. */ + TEST_CF_PUBLIC( res, len ); + if( result == 0 ) { TEST_ASSERT( len == strlen( dst ) ); From 6f69d4c7e22a54fd8830fad9cc1763e25d9b7d7f Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 3 Mar 2021 15:31:17 +0000 Subject: [PATCH 10/15] Remove multiplication from conditional assignments Multiplication is not constant flow on any CPU we are generally targetting, so replace this with bit twiddling. Signed-off-by: Paul Elliott --- library/base64.c | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/library/base64.c b/library/base64.c index c16d71164..6b0cb7169 100644 --- a/library/base64.c +++ b/library/base64.c @@ -102,22 +102,44 @@ static const unsigned char base64_dec_map[128] = static void mbedtls_base64_cond_assign_uchar(unsigned char * dest, const unsigned char * const src, unsigned char condition) { - /* make sure assign is 0 or 1 in a time-constant manner */ - condition = (condition | (unsigned char)-condition) >> 7; + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif - *dest = ( *dest ) * ( 1 - condition ) + ( *src ) * condition; + /* Generate bitmask from condition, mask will either be 0xFFFFFFFF or 0 */ + unsigned char mask = -( unsigned char )( ( condition | -condition ) >> 7 ); + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + *dest = ( ( *src ) & mask ) | ( ( *dest ) & ~mask ); } /* * Constant flow conditional assignment to uint_32 */ static void mbedtls_base64_cond_assign_uint32(uint32_t * dest, const uint32_t src, - unsigned char condition) + uint32_t condition) { - /* make sure assign is 0 or 1 in a time-constant manner */ - condition = (condition | (unsigned char)-condition) >> 7; + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif - *dest = ( *dest ) * ( 1 - condition ) + ( src ) * condition; + /* Generate bitmask from condition, mask will either be 0xFFFFFFFF or 0 */ + uint32_t mask = -( uint32_t )( ( condition | -condition ) >> 31 ); + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + *dest = ( src & mask ) | ( ( *dest ) & ~mask ); } From 490c5d462817acfba9dc33b221652b968ab5a0e0 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 3 Mar 2021 17:11:32 +0000 Subject: [PATCH 11/15] Fix constant flow mask maths Signed-off-by: Paul Elliott --- library/base64.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/library/base64.c b/library/base64.c index 6b0cb7169..9bc6a522b 100644 --- a/library/base64.c +++ b/library/base64.c @@ -110,7 +110,9 @@ static void mbedtls_base64_cond_assign_uchar(unsigned char * dest, const unsigne #endif /* Generate bitmask from condition, mask will either be 0xFFFFFFFF or 0 */ - unsigned char mask = -( unsigned char )( ( condition | -condition ) >> 7 ); + unsigned char mask = ( condition | -condition ); + mask >>= 7; + mask = -mask; #if defined(_MSC_VER) #pragma warning( pop ) @@ -133,7 +135,9 @@ static void mbedtls_base64_cond_assign_uint32(uint32_t * dest, const uint32_t sr #endif /* Generate bitmask from condition, mask will either be 0xFFFFFFFF or 0 */ - uint32_t mask = -( uint32_t )( ( condition | -condition ) >> 31 ); + uint32_t mask = ( condition | -condition ); + mask >>= 31; + mask = -mask; #if defined(_MSC_VER) #pragma warning( pop ) @@ -142,7 +146,6 @@ static void mbedtls_base64_cond_assign_uint32(uint32_t * dest, const uint32_t sr *dest = ( src & mask ) | ( ( *dest ) & ~mask ); } - /* * Constant flow check for equality */ From 4823624542a535ef46b597124263168b8369bc95 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 3 Mar 2021 17:21:17 +0000 Subject: [PATCH 12/15] Fix carelessly copy pasted comment Signed-off-by: Paul Elliott --- library/base64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/base64.c b/library/base64.c index 9bc6a522b..6b673ca9f 100644 --- a/library/base64.c +++ b/library/base64.c @@ -109,7 +109,7 @@ static void mbedtls_base64_cond_assign_uchar(unsigned char * dest, const unsigne #pragma warning( disable : 4146 ) #endif - /* Generate bitmask from condition, mask will either be 0xFFFFFFFF or 0 */ + /* Generate bitmask from condition, mask will either be 0xFF or 0 */ unsigned char mask = ( condition | -condition ); mask >>= 7; mask = -mask; From c22d0b9d5ba9164534a6a93bc711204d44f8ffe3 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 4 Mar 2021 14:23:03 +0000 Subject: [PATCH 13/15] Fix misnamed base64 test Signed-off-by: Paul Elliott --- tests/suites/test_suite_base64.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 39b9ca7f6..3a892f479 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -162,7 +162,7 @@ base64_encode_hex:"0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1 Base64 encode hex all valid input bytes #2 base64_encode_hex:"02030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f909192939495969798999a9b9c9d9e9fa0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebfc0c1c2c3c4c5c6c7c8c9cacbcccdcecfd0d1d2d3d4d5d6d7d8d9dadbdcdddedfe0e1e2e3e4e5e6e7e8e9eaebecedeeeff0f1f2f3f4f5f6f7f8f9fafbfcfdfeff0001":"AgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fICEiIyQlJicoKSorLC0uLzAxMjM0NTY3ODk6Ozw9Pj9AQUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVpbXF1eX2BhYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ent8fX5/gIGCg4SFhoeIiYqLjI2Oj5CRkpOUlZaXmJmam5ydnp+goaKjpKWmp6ipqqusra6vsLGys7S1tre4ubq7vL2+v8DBwsPExcbHyMnKy8zNzs/Q0dLT1NXW19jZ2tvc3d7f4OHi4+Tl5ufo6err7O3u7/Dx8vP09fb3+Pn6+/z9/v8AAQ==":345:0 -Base64 decode all valid output characters at all offsets +Base64 encode all valid output characters at all offsets base64_encode_hex:"00108310518720928b30d38f41149351559761969b71d79f8218a39259a7a29aabb2dbafc31cb3d35db7e39ebbf3dfbff800420c41461c824a2cc34e3d04524d45565d865a6dc75e7e08628e49669e8a6aaecb6ebf0c72cf4d76df8e7aefcf7effe00108310518720928b30d38f41149351559761969b71d79f8218a39259a7a29aabb2dbafc31cb3d35db7e39ebbf3dfbff800420c41461c824a2cc34e3d04524d45565d865a6dc75e7e08628e49669e8a6aaecb6ebf0c72cf4d76df8e7aefcf7efd0":"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/+ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/Q":261:0 Base64 decode hex #1 From 0c0f9adab180ee67ef674d2e7e12d3de21f94ad8 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 4 Mar 2021 14:24:57 +0000 Subject: [PATCH 14/15] Fixup changelog formatting Signed-off-by: Paul Elliott --- ChangeLog.d/make_base64_table_access_constant_flow.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/make_base64_table_access_constant_flow.txt b/ChangeLog.d/make_base64_table_access_constant_flow.txt index a11938912..733c972d0 100644 --- a/ChangeLog.d/make_base64_table_access_constant_flow.txt +++ b/ChangeLog.d/make_base64_table_access_constant_flow.txt @@ -1,4 +1,4 @@ Security * Guard against strong local side channel attack against base64 tables by - making access aceess to them use constant flow code. + making access aceess to them use constant flow code. From cd65b62f213a2c97e797a8d2419f1c013368596f Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 4 Mar 2021 14:34:50 +0000 Subject: [PATCH 15/15] Code style fixups Signed-off-by: Paul Elliott --- library/base64.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/library/base64.c b/library/base64.c index 6b673ca9f..692e11e3f 100644 --- a/library/base64.c +++ b/library/base64.c @@ -98,9 +98,9 @@ static const unsigned char base64_dec_map[128] = /* * Constant flow conditional assignment to unsigned char -*/ -static void mbedtls_base64_cond_assign_uchar(unsigned char * dest, const unsigned char * const src, - unsigned char condition) + */ +static void mbedtls_base64_cond_assign_uchar( unsigned char * dest, const unsigned char * const src, + unsigned char condition ) { /* MSVC has a warning about unary minus on unsigned integer types, * but this is well-defined and precisely what we want to do here. */ @@ -123,9 +123,9 @@ static void mbedtls_base64_cond_assign_uchar(unsigned char * dest, const unsigne /* * Constant flow conditional assignment to uint_32 -*/ -static void mbedtls_base64_cond_assign_uint32(uint32_t * dest, const uint32_t src, - uint32_t condition) + */ +static void mbedtls_base64_cond_assign_uint32( uint32_t * dest, const uint32_t src, + uint32_t condition ) { /* MSVC has a warning about unary minus on unsigned integer types, * but this is well-defined and precisely what we want to do here. */ @@ -148,8 +148,8 @@ static void mbedtls_base64_cond_assign_uint32(uint32_t * dest, const uint32_t sr /* * Constant flow check for equality -*/ -static unsigned char mbedtls_base64_eq(size_t in_a, size_t in_b) + */ +static unsigned char mbedtls_base64_eq( size_t in_a, size_t in_b ) { size_t difference = in_a ^ in_b; @@ -174,16 +174,16 @@ static unsigned char mbedtls_base64_eq(size_t in_a, size_t in_b) /* * Constant flow lookup into table. -*/ -static unsigned char mbedtls_base64_table_lookup(const unsigned char * const table, - const size_t table_size, const size_t table_index) + */ +static unsigned char mbedtls_base64_table_lookup( const unsigned char * const table, + const size_t table_size, const size_t table_index ) { size_t i; unsigned char result = 0; for( i = 0; i < table_size; ++i ) { - mbedtls_base64_cond_assign_uchar(&result, &table[i], mbedtls_base64_eq(i, table_index)); + mbedtls_base64_cond_assign_uchar( &result, &table[i], mbedtls_base64_eq( i, table_index ) ); } return result; @@ -342,7 +342,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( *src == '\r' || *src == '\n' || *src == ' ' ) continue; - dec_map_lookup = mbedtls_base64_table_lookup(base64_dec_map, sizeof( base64_dec_map ), *src ); + dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), *src ); mbedtls_base64_cond_assign_uint32( &j, j - 1, mbedtls_base64_eq( dec_map_lookup, 64 ) ); x = ( x << 6 ) | ( dec_map_lookup & 0x3F );