From dadd10d656ec4c97a3553437ca3e448db91e87a8 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 d39474a08..0b4ed56b1 100644 --- a/library/base64.c +++ b/library/base64.c @@ -65,6 +65,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 */ @@ -105,10 +143,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 ) @@ -116,11 +161,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++ = '='; @@ -171,7 +220,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 ) @@ -204,8 +254,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 448d54610cf8716fe33377ac873ece9ded958e8d 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 | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index dc6ec153b..ec1003346 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -1,5 +1,6 @@ /* BEGIN_HEADER */ #include "mbedtls/base64.h" +#include /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -13,13 +14,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 +63,10 @@ void base64_encode_hex( data_t * src, char * dst, int dst_buf_size, res = mbedtls_test_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 b4e4bfdd0085402eb4c9feb9c2a8bd909718aea4 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 3e7908189a84273eaa73b484f2c8bae457732038 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 0b4ed56b1..97b430416 100644 --- a/library/base64.c +++ b/library/base64.c @@ -65,6 +65,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) { @@ -75,19 +78,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 717ba77e5262510c86c72e7870980c64883bc1e1 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 97b430416..ed7db83dc 100644 --- a/library/base64.c +++ b/library/base64.c @@ -82,7 +82,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. */ @@ -97,7 +97,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 6e152fa36219fe9944a1e77ba567a4e48b5155d3 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 ed7db83dc..1c16a8801 100644 --- a/library/base64.c +++ b/library/base64.c @@ -207,6 +207,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++ ) @@ -237,11 +238,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++; @@ -271,9 +273,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 0544d49330b9b12b244a54c9ff145d55c45f1aaf 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 1c16a8801..ce9591893 100644 --- a/library/base64.c +++ b/library/base64.c @@ -66,9 +66,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 */ @@ -77,6 +77,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 */ @@ -114,7 +127,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; @@ -275,7 +288,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 c1a895d89783fee7849ba932cf79f80ee3d228bd 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 c48cb80b1f040b9757c7ce963bfb247b783d3a0d 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 ec1003346..be9b6e8c3 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -26,6 +26,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 ); @@ -67,6 +71,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 88f2eb664fab897e0d9dace73ff6c0e18d12843f 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 ce9591893..f2ae49b41 100644 --- a/library/base64.c +++ b/library/base64.c @@ -71,22 +71,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 3ffd13465ac1eec00fcaabe80b9b8d5498d3f3c6 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 f2ae49b41..267dcc227 100644 --- a/library/base64.c +++ b/library/base64.c @@ -79,7 +79,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 ) @@ -102,7 +104,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 ) @@ -111,7 +115,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 07fa1f1a39c748bd61d3288c49c2a48c6bcd3492 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 267dcc227..edbcfcaaf 100644 --- a/library/base64.c +++ b/library/base64.c @@ -78,7 +78,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 3c973f4d32a526f556ead0abacef7d6d6bded9b2 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 a5dce14291b6678d825435fcea03cbff1ef98e01 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 be165bd32b875cb3b6673babc1fc65c212d5efd1 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 edbcfcaaf..1a05226ef 100644 --- a/library/base64.c +++ b/library/base64.c @@ -67,9 +67,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. */ @@ -92,9 +92,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. */ @@ -117,8 +117,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; @@ -143,16 +143,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; @@ -311,7 +311,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 );