From edd371a82ca838110de0b2dbc44c412b360080bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Jan 2015 18:20:40 +0100 Subject: [PATCH 1/6] Enhance doc on ssl_write() --- include/polarssl/ssl.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 194e944718..4310ed6b71 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -1657,6 +1657,10 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ); * \note When this function returns POLARSSL_ERR_NET_WANT_WRITE, * it must be called later with the *same* arguments, * until it returns a positive value. + * + * \note This function may write less than the number of bytes + * requested if len is greater than the maximum record length. + * For arbitrary-sized messages, it should be called in a loop. */ int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ); From d76314c44cedb8e5af8dc27d84ecf6ff8ecbaa33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 7 Jan 2015 12:39:44 +0100 Subject: [PATCH 2/6] Add 1/n-1 record splitting --- include/polarssl/check_config.h | 5 ++++ include/polarssl/config.h | 12 +++++++++ include/polarssl/ssl.h | 3 +++ library/ssl_tls.c | 44 +++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+) diff --git a/include/polarssl/check_config.h b/include/polarssl/check_config.h index 328b881ea3..38d63fb42f 100644 --- a/include/polarssl/check_config.h +++ b/include/polarssl/check_config.h @@ -263,6 +263,11 @@ #error "POLARSSL_SSL_SESSION_TICKETS_C defined, but not all prerequisites" #endif +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) && \ + !defined(POLARSSL_SSL_PROTO_SSL3) && !defined(POLARSSL_SSL_PROTO_TLS1) +#error "POLARSSL_SSL_CBC_RECORD_SPLITTING defined, but not all prerequisites" +#endif + #if defined(POLARSSL_SSL_SERVER_NAME_INDICATION) && \ !defined(POLARSSL_X509_CRT_PARSE_C) #error "POLARSSL_SSL_SERVER_NAME_INDICATION defined, but not all prerequisites" diff --git a/include/polarssl/config.h b/include/polarssl/config.h index 50b4e339e4..584319210a 100644 --- a/include/polarssl/config.h +++ b/include/polarssl/config.h @@ -821,6 +821,18 @@ */ //#define POLARSSL_SSL_HW_RECORD_ACCEL +/** + * \def POLARSSL_SSL_CBC_RECORD_SPLITTING + * + * Enable 1/n-1 record splitting for CBC mode in SSLv3 and TLS 1.0. + * + * This is a countermeasure to the BEAST attack, which also minimizes the risk + * of interoperability issues compared to sending 0-length records. + * + * Comment this macro to disable 1/n-1 record splitting. + */ +#define POLARSSL_SSL_CBC_RECORD_SPLITTING + /** * \def POLARSSL_SSL_SRV_SUPPORT_SSLV2_CLIENT_HELLO * diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 4310ed6b71..718e6d5590 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -784,6 +784,9 @@ struct _ssl_context #if defined(POLARSSL_SSL_MAX_FRAGMENT_LENGTH) unsigned char mfl_code; /*!< MaxFragmentLength chosen by us */ #endif /* POLARSSL_SSL_MAX_FRAGMENT_LENGTH */ +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) + unsigned char split_done; /*!< flag for record splitting */ +#endif /* * PKI layer diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5f080defea..00e99f7989 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3482,6 +3482,9 @@ int ssl_session_reset( ssl_context *ssl ) ssl->out_msgtype = 0; ssl->out_msglen = 0; ssl->out_left = 0; +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) + ssl->split_done = 0; +#endif ssl->transform_in = NULL; ssl->transform_out = NULL; @@ -4431,7 +4434,11 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) /* * Send application data to be encrypted by the SSL layer */ +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) +static int ssl_write_real( ssl_context *ssl, const unsigned char *buf, size_t len ) +#else int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ) +#endif { int ret; size_t n; @@ -4492,6 +4499,43 @@ int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ) return( (int) n ); } +/* + * Write application data, doing 1/n-1 splitting if necessary. + * + * With non-blocking I/O, ssl_write_real() may return WANT_WRITE, + * then the caller wil call us again with the same arguments. + */ +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) +int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ) +{ + int ret; + + if( ssl->minor_ver > SSL_MINOR_VERSION_1 || + len == 0 || + cipher_get_cipher_mode( &ssl->transform_out->cipher_ctx_enc ) + != POLARSSL_MODE_CBC ) + { + return( ssl_write_real( ssl, buf, len ) ); + } + + if( ssl->split_done == 0 ) + { + ssl->split_done = 1; + if( ( ret = ssl_write_real( ssl, buf, 1 ) ) < 0 ) + return( ret ); + } + + if( ssl->split_done == 1 && len > 1 ) + { + ssl->split_done = 0; + if( ( ret = ssl_write_real( ssl, buf + 1, len - 1 ) ) < 0 ) + return( ret ); + } + + return( ret + 1 ); +} +#endif /* POLARSSL_SSL_CBC_RECORD_SPLITTING */ + /* * Notify the peer that the connection is being closed */ From cfa477ef2f10be4839298baa9b33be4c79c8c430 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 7 Jan 2015 14:50:54 +0100 Subject: [PATCH 3/6] Allow disabling record splitting at runtime --- include/polarssl/ssl.h | 21 ++++++++++++++++++++- library/ssl_tls.c | 20 +++++++++++++++----- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 718e6d5590..b29c5b4429 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -238,6 +238,9 @@ #define SSL_SESSION_TICKETS_DISABLED 0 #define SSL_SESSION_TICKETS_ENABLED 1 +#define SSL_CBC_RECORD_SPLITTING_DISABLED -1 +#define SSL_CBC_RECORD_SPLITTING_ENABLED 0 + /** * \name SECTION: Module settings * @@ -785,7 +788,8 @@ struct _ssl_context unsigned char mfl_code; /*!< MaxFragmentLength chosen by us */ #endif /* POLARSSL_SSL_MAX_FRAGMENT_LENGTH */ #if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) - unsigned char split_done; /*!< flag for record splitting */ + char split_done; /*!< flag for record splitting: + -1 disabled, 0 todo, 1 done */ #endif /* @@ -1420,6 +1424,21 @@ int ssl_set_max_frag_len( ssl_context *ssl, unsigned char mfl_code ); int ssl_set_truncated_hmac( ssl_context *ssl, int truncate ); #endif /* POLARSSL_SSL_TRUNCATED_HMAC */ +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) +/** + * \brief Enable / Disable 1/n-1 record splitting + * (Default: SSL_CBC_RECORD_SPLITTING_ENABLED) + * + * \note Only affects SSLv3 and TLS 1.0, not higher versions. + * Does not affect non-CBC ciphersuites in any version. + * + * \param ssl SSL context + * \param split SSL_CBC_RECORD_SPLITTING_ENABLED or + * SSL_CBC_RECORD_SPLITTING_DISABLED + */ +void ssl_set_cbc_record_splitting( ssl_context *ssl, char split ); +#endif /* POLARSSL_SSL_CBC_RECORD_SPLITTING */ + #if defined(POLARSSL_SSL_SESSION_TICKETS) /** * \brief Enable / Disable session tickets diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 00e99f7989..940bbc59ff 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3483,7 +3483,8 @@ int ssl_session_reset( ssl_context *ssl ) ssl->out_msglen = 0; ssl->out_left = 0; #if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) - ssl->split_done = 0; + if( ssl->split_done != SSL_CBC_RECORD_SPLITTING_DISABLED ) + ssl->split_done = 0; #endif ssl->transform_in = NULL; @@ -4007,6 +4008,13 @@ int ssl_set_truncated_hmac( ssl_context *ssl, int truncate ) } #endif /* POLARSSL_SSL_TRUNCATED_HMAC */ +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) +void ssl_set_cbc_record_splitting( ssl_context *ssl, char split ) +{ + ssl->split_done = split; +} +#endif + void ssl_set_renegotiation( ssl_context *ssl, int renegotiation ) { ssl->disable_renegotiation = renegotiation; @@ -4503,15 +4511,17 @@ int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ) * Write application data, doing 1/n-1 splitting if necessary. * * With non-blocking I/O, ssl_write_real() may return WANT_WRITE, - * then the caller wil call us again with the same arguments. + * then the caller will call us again with the same arguments, so + * remember wether we already did the split or not. */ #if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ) { int ret; - if( ssl->minor_ver > SSL_MINOR_VERSION_1 || - len == 0 || + if( ssl->split_done == SSL_CBC_RECORD_SPLITTING_DISABLED || + len <= 1 || + ssl->minor_ver > SSL_MINOR_VERSION_1 || cipher_get_cipher_mode( &ssl->transform_out->cipher_ctx_enc ) != POLARSSL_MODE_CBC ) { @@ -4525,7 +4535,7 @@ int ssl_write( ssl_context *ssl, const unsigned char *buf, size_t len ) return( ret ); } - if( ssl->split_done == 1 && len > 1 ) + if( ssl->split_done == 1 ) { ssl->split_done = 0; if( ( ret = ssl_write_real( ssl, buf + 1, len - 1 ) ) < 0 ) From c82ee3555fd73770e8e2be03747662299a876e8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 7 Jan 2015 16:35:25 +0100 Subject: [PATCH 4/6] Fix tests that were failing with record splitting --- programs/ssl/ssl_client2.c | 24 ++++++++++++++++++++++++ tests/ssl-opt.sh | 12 +++++++----- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 5b7a488c92..e08db04ca0 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -91,6 +91,7 @@ int main( int argc, char *argv[] ) #define DFL_AUTH_MODE SSL_VERIFY_REQUIRED #define DFL_MFL_CODE SSL_MAX_FRAG_LEN_NONE #define DFL_TRUNC_HMAC 0 +#define DFL_RECSPLIT -1 #define DFL_RECONNECT 0 #define DFL_RECO_DELAY 0 #define DFL_TICKETS SSL_SESSION_TICKETS_ENABLED @@ -128,6 +129,7 @@ struct options int auth_mode; /* verify mode for connection */ unsigned char mfl_code; /* code for maximum fragment length */ int trunc_hmac; /* negotiate truncated hmac or not */ + int recsplit; /* enable record splitting? */ int reconnect; /* attempt to resume session */ int reco_delay; /* delay in seconds before resuming session */ int tickets; /* enable / disable session tickets */ @@ -269,6 +271,13 @@ static int my_verify( void *data, x509_crt *crt, int depth, int *flags ) #define USAGE_MAX_FRAG_LEN "" #endif /* POLARSSL_SSL_MAX_FRAGMENT_LENGTH */ +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) +#define USAGE_RECSPLIT \ + " recplit=%%d default: (library default)\n" +#else +#define USAGE_RECSPLIT +#endif + #if defined(POLARSSL_TIMING_C) #define USAGE_TIME \ " reco_delay=%%d default: 0 seconds\n" @@ -313,6 +322,7 @@ static int my_verify( void *data, x509_crt *crt, int depth, int *flags ) USAGE_MAX_FRAG_LEN \ USAGE_TRUNC_HMAC \ USAGE_ALPN \ + USAGE_RECSPLIT \ "\n" \ " min_version=%%s default: \"\" (ssl3)\n" \ " max_version=%%s default: \"\" (tls1_2)\n" \ @@ -409,6 +419,7 @@ int main( int argc, char *argv[] ) opt.auth_mode = DFL_AUTH_MODE; opt.mfl_code = DFL_MFL_CODE; opt.trunc_hmac = DFL_TRUNC_HMAC; + opt.recsplit = DFL_RECSPLIT; opt.reconnect = DFL_RECONNECT; opt.reco_delay = DFL_RECO_DELAY; opt.tickets = DFL_TICKETS; @@ -600,6 +611,12 @@ int main( int argc, char *argv[] ) if( opt.trunc_hmac < 0 || opt.trunc_hmac > 1 ) goto usage; } + else if( strcmp( p, "recsplit" ) == 0 ) + { + opt.recsplit = atoi( q ); + if( opt.recsplit < 0 || opt.recsplit > 1 ) + goto usage; + } else goto usage; } @@ -882,6 +899,13 @@ int main( int argc, char *argv[] ) } #endif +#if defined(POLARSSL_SSL_CBC_RECORD_SPLITTING) + if( opt.recsplit != DFL_RECSPLIT ) + ssl_set_cbc_record_splitting( &ssl, opt.recsplit + ? SSL_CBC_RECORD_SPLITTING_ENABLED + : SSL_CBC_RECORD_SPLITTING_DISABLED ); +#endif + #if defined(POLARSSL_SSL_ALPN) if( opt.alpn_string != NULL ) if( ( ret = ssl_set_alpn_protocols( &ssl, alpn_list ) ) != 0 ) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index f94808d2cc..e6b63a18a0 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1684,7 +1684,8 @@ run_test "Small packet TLS 1.2 BlockCipher" \ run_test "Small packet TLS 1.2 BlockCipher larger MAC" \ "$P_SRV" \ - "$P_CLI request_size=1 force_version=tls1_2 force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384" \ + "$P_CLI request_size=1 force_version=tls1_2 \ + force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384" \ 0 \ -s "Read from client: 1 bytes read" @@ -1729,7 +1730,7 @@ run_test "Small packet TLS 1.2 AEAD shorter tag" \ run_test "Large packet SSLv3 BlockCipher" \ "$P_SRV" \ - "$P_CLI request_size=16384 force_version=ssl3 \ + "$P_CLI request_size=16384 force_version=ssl3 recsplit=0 \ force_ciphersuite=TLS-RSA-WITH-AES-256-CBC-SHA" \ 0 \ -s "Read from client: 16384 bytes read" @@ -1743,14 +1744,14 @@ run_test "Large packet SSLv3 StreamCipher" \ run_test "Large packet TLS 1.0 BlockCipher" \ "$P_SRV" \ - "$P_CLI request_size=16384 force_version=tls1 \ + "$P_CLI request_size=16384 force_version=tls1 recsplit=0 \ force_ciphersuite=TLS-RSA-WITH-AES-256-CBC-SHA" \ 0 \ -s "Read from client: 16384 bytes read" run_test "Large packet TLS 1.0 BlockCipher truncated MAC" \ "$P_SRV" \ - "$P_CLI request_size=16384 force_version=tls1 \ + "$P_CLI request_size=16384 force_version=tls1 recsplit=0 \ force_ciphersuite=TLS-RSA-WITH-AES-256-CBC-SHA \ trunc_hmac=1" \ 0 \ @@ -1803,7 +1804,8 @@ run_test "Large packet TLS 1.2 BlockCipher" \ run_test "Large packet TLS 1.2 BlockCipher larger MAC" \ "$P_SRV" \ - "$P_CLI request_size=16384 force_version=tls1_2 force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384" \ + "$P_CLI request_size=16384 force_version=tls1_2 \ + force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384" \ 0 \ -s "Read from client: 16384 bytes read" From 3ff78239fed152adc64cb334f428926f38d93adc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 8 Jan 2015 11:15:09 +0100 Subject: [PATCH 5/6] Add tests for CBC record splitting --- ChangeLog | 5 +++++ tests/ssl-opt.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/ChangeLog b/ChangeLog index fd83b9e608..24c545585b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ PolarSSL ChangeLog (Sorted per branch, date) += PolarSSL 1.3.10 released ??? + +Features + * Support for 1/n-1 record splitting, a countermeasure against BEAST. + = PolarSSL 1.3.9 released 2014-10-20 Security * Lowest common hash was selected from signature_algorithms extension in diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index e6b63a18a0..964eaadd50 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -426,6 +426,62 @@ run_test "Truncated HMAC: actual test" \ 0 \ -s "dumping 'computed mac' (10 bytes)" +# Tests for CBC 1/n-1 record splitting + +run_test "CBC Record splitting: TLS 1.2, no splitting" \ + "$P_SRV" \ + "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ + request_size=123 force_version=tls1_2" \ + 0 \ + -s "Read from client: 123 bytes read" \ + -S "Read from client: 1 bytes read" \ + -S "122 bytes read" + +run_test "CBC Record splitting: TLS 1.1, no splitting" \ + "$P_SRV" \ + "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ + request_size=123 force_version=tls1_1" \ + 0 \ + -s "Read from client: 123 bytes read" \ + -S "Read from client: 1 bytes read" \ + -S "122 bytes read" + +run_test "CBC Record splitting: TLS 1.0, splitting" \ + "$P_SRV" \ + "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ + request_size=123 force_version=tls1" \ + 0 \ + -S "Read from client: 123 bytes read" \ + -s "Read from client: 1 bytes read" \ + -s "122 bytes read" + +run_test "CBC Record splitting: SSLv3, splitting" \ + "$P_SRV" \ + "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ + request_size=123 force_version=ssl3" \ + 0 \ + -S "Read from client: 123 bytes read" \ + -s "Read from client: 1 bytes read" \ + -s "122 bytes read" + +run_test "CBC Record splitting: TLS 1.0 RC4, no splitting" \ + "$P_SRV" \ + "$P_CLI force_ciphersuite=TLS-RSA-WITH-RC4-128-SHA \ + request_size=123 force_version=tls1" \ + 0 \ + -s "Read from client: 123 bytes read" \ + -S "Read from client: 1 bytes read" \ + -S "122 bytes read" + +run_test "CBC Record splitting: TLS 1.0, splitting disabled" \ + "$P_SRV" \ + "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ + request_size=123 force_version=tls1 recsplit=0" \ + 0 \ + -s "Read from client: 123 bytes read" \ + -S "Read from client: 1 bytes read" \ + -S "122 bytes read" + # Tests for Session Tickets run_test "Session resume using tickets: basic" \ From b2eaac154b960a49430343895a76567cec28fc7f Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Tue, 13 Jan 2015 17:15:31 +0100 Subject: [PATCH 6/6] Stop assuming chars are signed --- programs/ssl/ssl_server2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index b02b1765ee..2980cf5689 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -180,8 +180,8 @@ struct options char *sni; /* string describing sni information */ const char *alpn_string; /* ALPN supported protocols */ const char *dhm_file; /* the file with the DH parameters */ - char extended_ms; /* allow negotiation of extended MS? */ - char etm; /* allow negotiation of encrypt-then-MAC? */ + int extended_ms; /* allow negotiation of extended MS? */ + int etm; /* allow negotiation of encrypt-then-MAC? */ } opt; static void my_debug( void *ctx, int level, const char *str )