From dbd23079d0f2f571bf8aa27569a8b8fac0176f4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 4 Sep 2015 10:20:17 +0200 Subject: [PATCH 01/23] Add option reconnect_hard to ssl_client2 - interrupt the connection abruptly (no close_notify) - reconnect from the same port while server sill has an active connection from this port. Some real-world clients do that, see section 4.2.8 of RFC 6347. --- programs/ssl/ssl_client2.c | 45 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 99c2d2a5e..758ebf34a 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -90,6 +90,7 @@ int main( void ) #define DFL_DHMLEN -1 #define DFL_RECONNECT 0 #define DFL_RECO_DELAY 0 +#define DFL_RECONNECT_HARD 0 #define DFL_TICKETS MBEDTLS_SSL_SESSION_TICKETS_ENABLED #define DFL_ALPN_STRING NULL #define DFL_TRANSPORT MBEDTLS_SSL_TRANSPORT_STREAM @@ -238,6 +239,7 @@ int main( void ) " exchanges=%%d default: 1\n" \ " reconnect=%%d default: 0 (disabled)\n" \ " reco_delay=%%d default: 0 seconds\n" \ + " reconnect_hard=%%d default: 0 (disabled)\n" \ USAGE_TICKETS \ USAGE_MAX_FRAG_LEN \ USAGE_TRUNC_HMAC \ @@ -293,6 +295,7 @@ struct options int dhmlen; /* minimum DHM params len in bits */ int reconnect; /* attempt to resume session */ int reco_delay; /* delay in seconds before resuming session */ + int reconnect_hard; /* unexpectedly reconnect from the same port */ int tickets; /* enable / disable session tickets */ const char *alpn_string; /* ALPN supported protocols */ int transport; /* TLS or DTLS? */ @@ -481,6 +484,7 @@ int main( int argc, char *argv[] ) opt.dhmlen = DFL_DHMLEN; opt.reconnect = DFL_RECONNECT; opt.reco_delay = DFL_RECO_DELAY; + opt.reconnect_hard = DFL_RECONNECT_HARD; opt.tickets = DFL_TICKETS; opt.alpn_string = DFL_ALPN_STRING; opt.transport = DFL_TRANSPORT; @@ -603,6 +607,12 @@ int main( int argc, char *argv[] ) if( opt.reco_delay < 0 ) goto usage; } + else if( strcmp( p, "reconnect_hard" ) == 0 ) + { + opt.reconnect_hard = atoi( q ); + if( opt.reconnect_hard < 0 || opt.reconnect_hard > 1 ) + goto usage; + } else if( strcmp( p, "tickets" ) == 0 ) { opt.tickets = atoi( q ); @@ -1479,7 +1489,38 @@ send_request: } /* - * 7b. Continue doing data exchanges? + * 7b. Simulate hard reset and reconnect from same port? + */ + if( opt.reconnect_hard != 0 ) + { + opt.reconnect_hard = 0; + + mbedtls_printf( " . Restarting connection from same port..." ); + fflush( stdout ); + + if( ( ret = mbedtls_ssl_session_reset( &ssl ) ) != 0 ) + { + mbedtls_printf( " failed\n ! mbedtls_ssl_session_reset returned -0x%x\n\n", -ret ); + goto exit; + } + + while( ( ret = mbedtls_ssl_handshake( &ssl ) ) != 0 ) + { + if( ret != MBEDTLS_ERR_SSL_WANT_READ && + ret != MBEDTLS_ERR_SSL_WANT_WRITE ) + { + mbedtls_printf( " failed\n ! mbedtls_ssl_handshake returned -0x%x\n\n", -ret ); + goto exit; + } + } + + mbedtls_printf( " ok\n" ); + + goto send_request; + } + + /* + * 7c. Continue doing data exchanges? */ if( --opt.exchanges > 0 ) goto send_request; @@ -1489,6 +1530,7 @@ send_request: */ close_notify: mbedtls_printf( " . Closing the connection..." ); + fflush( stdout ); /* No error checking, the connection might be closed already */ do ret = mbedtls_ssl_close_notify( &ssl ); @@ -1513,7 +1555,6 @@ reconnect: #endif mbedtls_printf( " . Reconnecting with saved session..." ); - fflush( stdout ); if( ( ret = mbedtls_ssl_session_reset( &ssl ) ) != 0 ) { From 26d227ddfc5bf6283b44be0618a85fc3cebde63f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 4 Sep 2015 10:53:25 +0200 Subject: [PATCH 02/23] Add config flag for support of client port reuse --- include/mbedtls/config.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 66657da55..4956d0476 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1134,6 +1134,20 @@ */ #define MBEDTLS_SSL_DTLS_HELLO_VERIFY +/** + * \def MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE + * + * Enable server-side support for clients that reconnect from the same port. + * + * Some clients unexpectedly close the connection and try to reconnect using the + * same source port. This needs special support from the server to handle the + * new connection securely, as described in section 4.1.8 of RFC 6347. This + * flag enables that support. + * + * Comment this to disable support for clients reusing the source port. + */ +#define MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE + /** * \def MBEDTLS_SSL_DTLS_BADMAC_LIMIT * From 9650205df7fbe16facf1972aa07e4301300e8d5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 4 Sep 2015 12:58:55 +0200 Subject: [PATCH 03/23] Start detecting epoch 0 ClientHellos --- library/ssl_tls.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 14ee521ca..ded856948 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3341,13 +3341,35 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) if( rec_epoch != ssl->in_epoch ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "record from another epoch: " - "expected %d, received %d", - ssl->in_epoch, rec_epoch ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + "expected %d, received %d", + ssl->in_epoch, rec_epoch ) ); + +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && \ + defined(MBEDTLS_SSL_SRV_C) + /* + * Check for an epoch 0 ClientHello. We can't use in_msg here to + * access the first byte of record content (handshake type), as we + * have an active transform (possibly iv_len != 0), so use the + * fact that the record header len is 13 instead. + */ + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + rec_epoch == 0 && + ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && + ssl->in_left > 13 && + ssl->in_buf[13] == MBEDTLS_SSL_HS_CLIENT_HELLO ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "possible client reconnect " + "from the same port" ) ); + } + else +#endif /* MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) - if( mbedtls_ssl_dtls_replay_check( ssl ) != 0 ) + /* Replay detection only works for the current epoch */ + if( rec_epoch == ssl->in_epoch && + mbedtls_ssl_dtls_replay_check( ssl ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "replayed record" ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); From 11331fc25b2ed61cdb196e65e3b7b6c5c67a017d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 10:30:55 +0200 Subject: [PATCH 04/23] First working dirty version - uses too much resources - wrong API --- library/ssl_tls.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ded856948..75e14d5a6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3250,6 +3250,113 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_DTLS_ANTI_REPLAY */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) && \ + defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && \ + defined(MBEDTLS_SSL_SRV_C) +/* Dummy timer callbacks (temporary) */ +static void ssl_dummy_set_timer(void *ctx, uint32_t int_ms, uint32_t fin_ms) { + (void) ctx; (void) int_ms; (void) fin_ms; } +static int ssl_dummy_get_timer(void *ctx) { (void) ctx; return( 0 ); } + +/* Dummy recv callback (temporary) */ +static int ssl_dummy_recv(void *ctx, unsigned char *buf, size_t len) { + (void) ctx; (void) buf; (void) len; return( 0 ); } + +/* + * Handle possible client reconnect with the same UDP quadruplet + * (RFC 6347 Section 4.2.8). + * + * Called by ssl_parse_record_header() in case we receive an epoch 0 record + * that looks like a ClientHello. + * + * - if the input looks wrong, + * return MBEDTLS_ERR_SSL_INVALID_RECORD (ignore this record) + * - if the input looks like a ClientHello without cookies, + * send back HelloVerifyRequest, then + * return MBEDTLS_ERR_SSL_INVALID_RECORD (ignore this record) + * - if the input looks like a ClientHello with a valid cookie, + * reset the session of the current context, and + * return MBEDTLS_ERR_SSL_CLIENT_RECONNECT (WIP: TODO) + * + * Currently adopts a heavyweight strategy by allocating a secondary ssl + * context. Will be refactored into something more acceptable later. + */ +static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) +{ + int ret; + mbedtls_ssl_context tmp_ssl; + int cookie_is_good; + + mbedtls_ssl_init( &tmp_ssl ); + + /* Prepare temporary ssl context */ + ret = mbedtls_ssl_setup( &tmp_ssl, ssl->conf ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 0, "nested ssl_setup", ret ); + ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + goto cleanup; + } + + mbedtls_ssl_set_timer_cb( &tmp_ssl, NULL, ssl_dummy_set_timer, + ssl_dummy_get_timer ); + + ret = mbedtls_ssl_set_client_transport_id( &tmp_ssl, + ssl->cli_id, ssl->cli_id_len ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 0, "nested set_client_id", ret ); + ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + goto cleanup; + } + + mbedtls_ssl_set_bio( &tmp_ssl, ssl->p_bio, ssl->f_send, + ssl_dummy_recv, NULL ); + + memcpy( tmp_ssl.in_buf, ssl->in_buf, ssl->in_left ); + tmp_ssl.in_left = ssl->in_left; + + tmp_ssl.state = MBEDTLS_SSL_CLIENT_HELLO; + + /* Parse packet and check if cookie is good */ + ret = mbedtls_ssl_handshake_step( &tmp_ssl ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 0, "nested handshake_step", ret ); + ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + goto cleanup; + } + + cookie_is_good = tmp_ssl.handshake->verify_cookie_len == 0; + MBEDTLS_SSL_DEBUG_MSG( 0, ( "good ClientHello with %s cookie", + cookie_is_good ? "good" : "bad" ) ); + + /* Send HelloVerifyRequest? */ + if( !cookie_is_good ) + { + ret = mbedtls_ssl_handshake_step( &tmp_ssl ); + MBEDTLS_SSL_DEBUG_RET( 0, "nested handshake_step", ret ); + + ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + goto cleanup; + } + + /* We should retrieve the content of the ClientHello from tmp_ssl, + * instead let's play it dirty for this temporary version and just trust + * that the client will resend */ + mbedtls_ssl_session_reset( ssl ); + + /* ret = ... */ + +cleanup: + mbedtls_ssl_free( &tmp_ssl ); + + return( ret ); +} +#endif /* MBEDTLS_SSL_PROTO_DTLS && + MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && + MBEDTLS_SSL_SRV_C */ + /* * ContentType type; * ProtocolVersion version; @@ -3360,6 +3467,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "possible client reconnect " "from the same port" ) ); + return( ssl_handle_possible_reconnect( ssl ) ); } else #endif /* MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ From be619c1264686e2c0e06de3b5ff6cdbe9360ea13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 11:21:21 +0200 Subject: [PATCH 05/23] Clean up error codes --- include/mbedtls/ssl.h | 1 + library/error.c | 2 ++ library/ssl_tls.c | 26 +++++++++++++------------- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index bd88918ca..8ff453929 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -125,6 +125,7 @@ #define MBEDTLS_ERR_SSL_WANT_READ -0x6900 /**< Connection requires a read call. */ #define MBEDTLS_ERR_SSL_WANT_WRITE -0x6880 /**< Connection requires a write call. */ #define MBEDTLS_ERR_SSL_TIMEOUT -0x6800 /**< The operation timed out. */ +#define MBEDTLS_ERR_SSL_CLIENT_RECONNECT -0x6780 /**< The client initiated a reconnect from the same port. */ /* * Various constants diff --git a/library/error.c b/library/error.c index a7de11007..a1cf83aed 100644 --- a/library/error.c +++ b/library/error.c @@ -428,6 +428,8 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen ) mbedtls_snprintf( buf, buflen, "SSL - Connection requires a write call" ); if( use_ret == -(MBEDTLS_ERR_SSL_TIMEOUT) ) mbedtls_snprintf( buf, buflen, "SSL - The operation timed out" ); + if( use_ret == -(MBEDTLS_ERR_SSL_CLIENT_RECONNECT) ) + mbedtls_snprintf( buf, buflen, "SSL - The client initiated a reconnect from the same port" ); #endif /* MBEDTLS_SSL_TLS_C */ #if defined(MBEDTLS_X509_USE_C) || defined(MBEDTLS_X509_CREATE_C) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 75e14d5a6..c4dfd61e7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3276,7 +3276,7 @@ static int ssl_dummy_recv(void *ctx, unsigned char *buf, size_t len) { * return MBEDTLS_ERR_SSL_INVALID_RECORD (ignore this record) * - if the input looks like a ClientHello with a valid cookie, * reset the session of the current context, and - * return MBEDTLS_ERR_SSL_CLIENT_RECONNECT (WIP: TODO) + * return MBEDTLS_ERR_SSL_CLIENT_RECONNECT * * Currently adopts a heavyweight strategy by allocating a secondary ssl * context. Will be refactored into something more acceptable later. @@ -3293,8 +3293,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) ret = mbedtls_ssl_setup( &tmp_ssl, ssl->conf ); if( ret != 0 ) { - MBEDTLS_SSL_DEBUG_RET( 0, "nested ssl_setup", ret ); - ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + MBEDTLS_SSL_DEBUG_RET( 1, "nested ssl_setup", ret ); goto cleanup; } @@ -3305,8 +3304,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) ssl->cli_id, ssl->cli_id_len ); if( ret != 0 ) { - MBEDTLS_SSL_DEBUG_RET( 0, "nested set_client_id", ret ); - ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + MBEDTLS_SSL_DEBUG_RET( 1, "nested set_client_id", ret ); goto cleanup; } @@ -3322,22 +3320,20 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) ret = mbedtls_ssl_handshake_step( &tmp_ssl ); if( ret != 0 ) { - MBEDTLS_SSL_DEBUG_RET( 0, "nested handshake_step", ret ); - ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + MBEDTLS_SSL_DEBUG_RET( 1, "nested handshake_step", ret ); goto cleanup; } cookie_is_good = tmp_ssl.handshake->verify_cookie_len == 0; - MBEDTLS_SSL_DEBUG_MSG( 0, ( "good ClientHello with %s cookie", + MBEDTLS_SSL_DEBUG_MSG( 1, ( "good ClientHello with %s cookie", cookie_is_good ? "good" : "bad" ) ); /* Send HelloVerifyRequest? */ if( !cookie_is_good ) { ret = mbedtls_ssl_handshake_step( &tmp_ssl ); - MBEDTLS_SSL_DEBUG_RET( 0, "nested handshake_step", ret ); - - ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + if( ret != 0 ) + MBEDTLS_SSL_DEBUG_RET( 1, "nested handshake_step", ret ); goto cleanup; } @@ -3346,11 +3342,14 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) * that the client will resend */ mbedtls_ssl_session_reset( ssl ); - /* ret = ... */ + ret = MBEDTLS_ERR_SSL_CLIENT_RECONNECT; cleanup: mbedtls_ssl_free( &tmp_ssl ); + if( ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) + ret = MBEDTLS_ERR_SSL_INVALID_RECORD; + return( ret ); } #endif /* MBEDTLS_SSL_PROTO_DTLS && @@ -3658,7 +3657,8 @@ read_record_header: if( ( ret = ssl_parse_record_header( ssl ) ) != 0 ) { #if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) { /* Ignore bad record and get next one; drop the whole datagram * since current header cannot be trusted to find the next record From 3f09b6d4c209f42d9cfe8f1de1bb7285c50d2a56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 11:58:14 +0200 Subject: [PATCH 06/23] Fix API --- library/ssl_tls.c | 43 +++++++++++++++++++++++++++++--------- programs/ssl/ssl_server2.c | 7 +++++++ 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c4dfd61e7..4addeb7d9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3262,6 +3262,9 @@ static int ssl_dummy_get_timer(void *ctx) { (void) ctx; return( 0 ); } static int ssl_dummy_recv(void *ctx, unsigned char *buf, size_t len) { (void) ctx; (void) buf; (void) len; return( 0 ); } +/* Forward declatation */ +static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); + /* * Handle possible client reconnect with the same UDP quadruplet * (RFC 6347 Section 4.2.8). @@ -3337,10 +3340,13 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) goto cleanup; } - /* We should retrieve the content of the ClientHello from tmp_ssl, - * instead let's play it dirty for this temporary version and just trust - * that the client will resend */ - mbedtls_ssl_session_reset( ssl ); + /* Reset context while preserving some information */ + ret = ssl_session_reset_int( ssl, 1 ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "reset", ret ); + goto cleanup; + } ret = MBEDTLS_ERR_SSL_CLIENT_RECONNECT; @@ -5253,8 +5259,11 @@ int mbedtls_ssl_setup( mbedtls_ssl_context *ssl, /* * Reset an initialized and used SSL context for re-use while retaining * all application-set variables, function pointers and data. + * + * If partial is non-zero, keep data in the input buffer and client ID. + * (Use when a DTLS client reconnects from the same port.) */ -int mbedtls_ssl_session_reset( mbedtls_ssl_context *ssl ) +static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) { int ret; @@ -5278,7 +5287,8 @@ int mbedtls_ssl_session_reset( mbedtls_ssl_context *ssl ) ssl->in_msg = ssl->in_buf + 13; ssl->in_msgtype = 0; ssl->in_msglen = 0; - ssl->in_left = 0; + if( partial == 0 ) + ssl->in_left = 0; #if defined(MBEDTLS_SSL_PROTO_DTLS) ssl->next_record_offset = 0; ssl->in_epoch = 0; @@ -5304,7 +5314,8 @@ int mbedtls_ssl_session_reset( mbedtls_ssl_context *ssl ) ssl->transform_out = NULL; memset( ssl->out_buf, 0, MBEDTLS_SSL_BUFFER_LEN ); - memset( ssl->in_buf, 0, MBEDTLS_SSL_BUFFER_LEN ); + if( partial == 0 ) + memset( ssl->in_buf, 0, MBEDTLS_SSL_BUFFER_LEN ); #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) if( mbedtls_ssl_hw_record_reset != NULL ) @@ -5337,9 +5348,12 @@ int mbedtls_ssl_session_reset( mbedtls_ssl_context *ssl ) #endif #if defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY) && defined(MBEDTLS_SSL_SRV_C) - mbedtls_free( ssl->cli_id ); - ssl->cli_id = NULL; - ssl->cli_id_len = 0; + if( partial == 0 ) + { + mbedtls_free( ssl->cli_id ); + ssl->cli_id = NULL; + ssl->cli_id_len = 0; + } #endif if( ( ret = ssl_handshake_init( ssl ) ) != 0 ) @@ -5348,6 +5362,15 @@ int mbedtls_ssl_session_reset( mbedtls_ssl_context *ssl ) return( 0 ); } +/* + * Reset an initialized and used SSL context for re-use while retaining + * all application-set variables, function pointers and data. + */ +int mbedtls_ssl_session_reset( mbedtls_ssl_context *ssl ) +{ + return( ssl_session_reset_int( ssl, 0 ) ); +} + /* * SSL set accessors */ diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index f169291eb..7fc9fa05f 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -1838,6 +1838,12 @@ reset: } #endif + if( ret == MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) + { + mbedtls_printf( " ! Client initiated reconnection from same port\n" ); + goto handshake; + } + #ifdef MBEDTLS_ERROR_C if( ret != 0 ) { @@ -1903,6 +1909,7 @@ reset: /* * 4. Handshake */ +handshake: mbedtls_printf( " . Performing the SSL/TLS handshake..." ); fflush( stdout ); From d745a1a9b761cc12b00700b2bac7620616b2f202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 12:40:43 +0200 Subject: [PATCH 07/23] Add tests for hard reconnect --- tests/ssl-opt.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 77db588db..ba6e578c1 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2867,6 +2867,28 @@ run_test "DTLS cookie: enabled, nbio" \ -s "hello verification requested" \ -S "SSL - The requested feature is not available" +# Tests for client reconnecting from the same port with DTLS + +run_test "DTLS client reconnect from same port: reference" \ + "$P_SRV dtls=1 exchanges=2" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2" \ + 0 \ + -C "resend" \ + -S "Client initiated reconnection from same port" + +run_test "DTLS client reconnect from same port: reconnect" \ + "$P_SRV dtls=1 exchanges=2" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 reconnect_hard=1" \ + 0 \ + -C "resend" \ + -s "Client initiated reconnection from same port" + +run_test "DTLS client reconnect from same port: reconnect, nbio" \ + "$P_SRV dtls=1 exchanges=2 nbio=2" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 reconnect_hard=1" \ + 0 \ + -s "Client initiated reconnection from same port" + # Tests for various cases of client authentication with DTLS # (focused on handshake flows and message parsing) From 14c2574a9da15dbdc8add4824528e967169b5d69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 15:12:45 +0200 Subject: [PATCH 08/23] Update Changelog --- ChangeLog | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ChangeLog b/ChangeLog index d3636f00a..b4b1f61a7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,16 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.1.1 released 2015-09-?? + +Changes + * When a client initiates a reconnect from the same port as a live + connection, if cookie verification is available + (MBEDTLS_SSL_DTLS_HELLO_VERIFY defined in config.h, and usable cookie + callbacks set with mbedtls_ssl_conf_dtls_cookies()), this will be + detected and mbedtls_ssl_read() will return + MBEDTLS_ERR_SSL_CLIENT_RECONNECT - it is then possible to start a new + handshake with the same context. (See RFC 6347 section 4.2.8.) + = mbed TLS 2.1.0 released 2015-09-04 Features From 3a2a4485d486462331a2528a5774781db9f866c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 15:36:09 +0200 Subject: [PATCH 09/23] Update documentation --- include/mbedtls/ssl.h | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 8ff453929..c04070d87 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1170,6 +1170,11 @@ typedef int mbedtls_ssl_cookie_check_t( void *ctx, * the MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED that is expected * on the first handshake attempt when this is enabled. * + * \note This is also necessary to handle client reconnection from + * the same port as described in RFC 6347 section 4.2.8 (only + * the variant with cookies is supported currently). See + * comments on \c mbedtls_ssl_read() for details. + * * \param conf SSL configuration * \param f_cookie_write Cookie write callback * \param f_cookie_check Cookie check callback @@ -2139,7 +2144,23 @@ int mbedtls_ssl_renegotiate( mbedtls_ssl_context *ssl ); * * \return the number of bytes read, or * 0 for EOF, or - * a negative error code. + * MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE, or + * MBEDTLS_ERR_SSL_CLIENT_RECONNECT (see below), or + * another negative error code. + * + * \note When this function return MBEDTLS_ERR_SSL_CLIENT_RECONNECT + * (which can only happen server-side), it means that a client + * is initiating a new connection using the same source port. + * You can either treat that as a connection close and wait + * for the client to resend a ClientHello, or directly + * continue with \c mbedtls_ssl_handshake() with the same + * context (as it has beeen reset internally). Either way, you + * should make sure this is seen by the application as a new + * connection: application state, if any, should be reset, and + * most importantly the identity of the client must be checked + * again. WARNING: not validating the identity of the client + * again, or not transmitting the new identity to the + * application layer, would allow authentication bypass! */ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ); From 222cb8db2251ae3463f5159eb55ca378bfc85307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 15:43:59 +0200 Subject: [PATCH 10/23] Tune related documentation while at it --- include/mbedtls/ssl.h | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index c04070d87..7110e1ccf 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2095,29 +2095,35 @@ int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, mbedtls_ssl_session * * \param ssl SSL context * - * \return 0 if successful, MBEDTLS_ERR_SSL_WANT_READ, - * MBEDTLS_ERR_SSL_WANT_WRITE, or a specific SSL error code. + * \return 0 if successful, or + * MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE, or + * MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED (see belowe), or + * a specific SSL error code. * - * \note If this function returns non-zero, then the ssl context + * \note If this function returns something other than 0 or + * MBEDTLS_ERR_SSL_WANT_READ/WRITE, then the ssl context * becomes unusable, and you should either free it or call * \c mbedtls_ssl_session_reset() on it before re-using it. - * If DTLS is in use, then you may choose to handle + * + * \note If DTLS is in use, then you may choose to handle * MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED specially for logging - * purposes, but you still need to reset/free the context. + * purposes, as it is an expected return value rather than an + * actual error, but you still need to reset/free the context. */ int mbedtls_ssl_handshake( mbedtls_ssl_context *ssl ); /** * \brief Perform a single step of the SSL handshake * - * Note: the state of the context (ssl->state) will be at + * \note The state of the context (ssl->state) will be at * the following state after execution of this function. * Do not call this function if state is MBEDTLS_SSL_HANDSHAKE_OVER. * * \param ssl SSL context * - * \return 0 if successful, MBEDTLS_ERR_SSL_WANT_READ, - * MBEDTLS_ERR_SSL_WANT_WRITE, or a specific SSL error code. + * \return 0 if successful, or + * MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE, or + * a specific SSL error code. */ int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl ); From 62c74bb78a5840e6db4dddd24a5854ff59928bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Sep 2015 17:50:29 +0200 Subject: [PATCH 11/23] Stop wasting resources Use a custom function that minimally parses the message an creates a reply without the overhead of a full SSL context. Also fix dependencies: needs DTLS_HELLO_VERIFY for the cookie types, and let's also depend on SRV_C as is doesn't make sense on client. --- include/mbedtls/check_config.h | 5 + include/mbedtls/config.h | 3 + library/ssl_tls.c | 246 +++++++++++++++++++++------------ 3 files changed, 169 insertions(+), 85 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index cb707e9bc..2280caba7 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -421,6 +421,11 @@ #error "MBEDTLS_SSL_DTLS_HELLO_VERIFY defined, but not all prerequisites" #endif +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && \ + ( !defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY) || !defined(MBEDTLS_SSL_SRV_C) ) +#error "MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE defined, but not all prerequisites" +#endif + #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) && \ ( !defined(MBEDTLS_SSL_TLS_C) || !defined(MBEDTLS_SSL_PROTO_DTLS) ) #error "MBEDTLS_SSL_DTLS_ANTI_REPLAY defined, but not all prerequisites" diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 4956d0476..381e82b11 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1144,6 +1144,9 @@ * new connection securely, as described in section 4.1.8 of RFC 6347. This * flag enables that support. * + * Requires: MBEDTLS_SSL_DTLS_HELLO_VERIFY + * MBEDTLS_SSL_SRV_C + * * Comment this to disable support for clients reusing the source port. */ #define MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4addeb7d9..02cfaf1b9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3250,21 +3250,136 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_DTLS_ANTI_REPLAY */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) && \ - defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && \ - defined(MBEDTLS_SSL_SRV_C) -/* Dummy timer callbacks (temporary) */ -static void ssl_dummy_set_timer(void *ctx, uint32_t int_ms, uint32_t fin_ms) { - (void) ctx; (void) int_ms; (void) fin_ms; } -static int ssl_dummy_get_timer(void *ctx) { (void) ctx; return( 0 ); } - -/* Dummy recv callback (temporary) */ -static int ssl_dummy_recv(void *ctx, unsigned char *buf, size_t len) { - (void) ctx; (void) buf; (void) len; return( 0 ); } - -/* Forward declatation */ +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) +/* Forward declaration */ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); +/* + * Without any SSL context, check if a datagram looks like a ClientHello with + * a valid cookie, and if it doesn't, generate a HelloVerifyRequest message. + * Both input and input include full DTLS headers. + * + * - if cookie is valid, return 0 + * - if ClientHello looks superficially valid but cookie is not, + * fill obuf and set olen, then + * return MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED + * - otherwise return a specific error code + */ +static int ssl_check_dtls_clihlo_cookie( + mbedtls_ssl_cookie_write_t *f_cookie_write, + mbedtls_ssl_cookie_check_t *f_cookie_check, + void *p_cookie, + const unsigned char *cli_id, size_t cli_id_len, + const unsigned char *in, size_t in_len, + unsigned char *obuf, size_t buf_len, size_t *olen ) +{ + size_t sid_len, cookie_len; + unsigned char *p; + + if( f_cookie_write == NULL || f_cookie_check == NULL ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + /* + * Structure of ClientHello with record and handshake headers, + * and expected values. We don't need to check a lot, more checks will be + * done when actually parsing the ClientHello - skipping those checks + * avoids code duplication and does not make cookie forging any easier. + * + * 0-0 ContentType type; copied, must be handshake + * 1-2 ProtocolVersion version; copied + * 3-4 uint16 epoch; copied, must be 0 + * 5-10 uint48 sequence_number; copied + * 11-12 uint16 length; (ignored) + * + * 13-13 HandshakeType msg_type; (ignored) + * 14-16 uint24 length; (ignored) + * 17-18 uint16 message_seq; copied + * 19-21 uint24 fragment_offset; copied, must be 0 + * 22-24 uint24 fragment_length; (ignored) + * + * 25-26 ProtocolVersion client_version; (ignored) + * 27-58 Random random; (ignored) + * 59-xx SessionID session_id; 1 byte len + sid_len content + * 60+ opaque cookie<0..2^8-1>; 1 byte len + content + * ... + * + * Minimum length is 61 bytes. + */ + if( in_len < 61 || + in[0] != MBEDTLS_SSL_MSG_HANDSHAKE || + in[3] != 0 || in[4] != 0 || + in[19] != 0 || in[20] != 0 || in[21] != 0 ) + { + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + + sid_len = in[59]; + if( sid_len > in_len - 61 ) + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + + cookie_len = in[60 + sid_len]; + if( cookie_len > in_len - 60 ) + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + + if( f_cookie_check( p_cookie, in + sid_len + 61, cookie_len, + cli_id, cli_id_len ) == 0 ) + { + /* Valid cookie */ + return( 0 ); + } + + /* + * If we get here, we've got an invalid cookie, let's prepare HVR. + * + * 0-0 ContentType type; copied + * 1-2 ProtocolVersion version; copied + * 3-4 uint16 epoch; copied + * 5-10 uint48 sequence_number; copied + * 11-12 uint16 length; olen - 13 + * + * 13-13 HandshakeType msg_type; hello_verify_request + * 14-16 uint24 length; olen - 25 + * 17-18 uint16 message_seq; copied + * 19-21 uint24 fragment_offset; copied + * 22-24 uint24 fragment_length; olen - 25 + * + * 25-26 ProtocolVersion server_version; 0xfe 0xff + * 27-27 opaque cookie<0..2^8-1>; cookie_len = olen - 27, cookie + * + * Minimum length is 28. + */ + if( buf_len < 28 ) + return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); + + /* Copy most fields and adapt others */ + memcpy( obuf, in, 25 ); + obuf[13] = MBEDTLS_SSL_HS_HELLO_VERIFY_REQUEST; + obuf[25] = 0xfe; + obuf[26] = 0xff; + + /* Generate and write actual cookie */ + p = obuf + 28; + if( f_cookie_write( p_cookie, + &p, obuf + buf_len, cli_id, cli_id_len ) != 0 ) + { + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + *olen = p - obuf; + + /* Go back and fill length fields */ + obuf[27] = (unsigned char)( *olen - 28 ); + + obuf[14] = obuf[22] = (unsigned char)( ( *olen - 25 ) >> 16 ); + obuf[15] = obuf[23] = (unsigned char)( ( *olen - 25 ) >> 8 ); + obuf[16] = obuf[24] = (unsigned char)( ( *olen - 25 ) ); + + obuf[11] = (unsigned char)( ( *olen - 13 ) >> 8 ); + obuf[12] = (unsigned char)( ( *olen - 13 ) ); + + return( MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED ); +} + /* * Handle possible client reconnect with the same UDP quadruplet * (RFC 6347 Section 4.2.8). @@ -3272,95 +3387,57 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); * Called by ssl_parse_record_header() in case we receive an epoch 0 record * that looks like a ClientHello. * - * - if the input looks wrong, - * return MBEDTLS_ERR_SSL_INVALID_RECORD (ignore this record) * - if the input looks like a ClientHello without cookies, * send back HelloVerifyRequest, then - * return MBEDTLS_ERR_SSL_INVALID_RECORD (ignore this record) + * return MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED * - if the input looks like a ClientHello with a valid cookie, * reset the session of the current context, and * return MBEDTLS_ERR_SSL_CLIENT_RECONNECT + * - if anything goes wrong, return a specific error code * - * Currently adopts a heavyweight strategy by allocating a secondary ssl - * context. Will be refactored into something more acceptable later. + * mbedtls_ssl_read_record() will ignore the record if anything else than + * MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned (we never return 0). */ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) { int ret; - mbedtls_ssl_context tmp_ssl; - int cookie_is_good; + size_t len; - mbedtls_ssl_init( &tmp_ssl ); + ret = ssl_check_dtls_clihlo_cookie( + ssl->conf->f_cookie_write, + ssl->conf->f_cookie_check, + ssl->conf->p_cookie, + ssl->cli_id, ssl->cli_id_len, + ssl->in_buf, ssl->in_left, + ssl->out_buf, MBEDTLS_SSL_MAX_CONTENT_LEN, &len ); - /* Prepare temporary ssl context */ - ret = mbedtls_ssl_setup( &tmp_ssl, ssl->conf ); - if( ret != 0 ) + MBEDTLS_SSL_DEBUG_RET( 2, "ssl_check_dtls_clihlo_cookie", ret ); + + if( ret == MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED ) { - MBEDTLS_SSL_DEBUG_RET( 1, "nested ssl_setup", ret ); - goto cleanup; + /* Dont check write errors as we can't do anything here. + * If the error is permanent we'll catch it later, + * if it's not, then hopefully it'll work next time. */ + (void) ssl->f_send( ssl->p_bio, ssl->out_buf, len ); + + return( MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED ); } - mbedtls_ssl_set_timer_cb( &tmp_ssl, NULL, ssl_dummy_set_timer, - ssl_dummy_get_timer ); - - ret = mbedtls_ssl_set_client_transport_id( &tmp_ssl, - ssl->cli_id, ssl->cli_id_len ); - if( ret != 0 ) + if( ret == 0 ) { - MBEDTLS_SSL_DEBUG_RET( 1, "nested set_client_id", ret ); - goto cleanup; + /* Got a valid cookie, partially reset context */ + if( ( ret = ssl_session_reset_int( ssl, 1 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "reset", ret ); + return( ret ); + } + + return( MBEDTLS_ERR_SSL_CLIENT_RECONNECT ); } - mbedtls_ssl_set_bio( &tmp_ssl, ssl->p_bio, ssl->f_send, - ssl_dummy_recv, NULL ); - - memcpy( tmp_ssl.in_buf, ssl->in_buf, ssl->in_left ); - tmp_ssl.in_left = ssl->in_left; - - tmp_ssl.state = MBEDTLS_SSL_CLIENT_HELLO; - - /* Parse packet and check if cookie is good */ - ret = mbedtls_ssl_handshake_step( &tmp_ssl ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "nested handshake_step", ret ); - goto cleanup; - } - - cookie_is_good = tmp_ssl.handshake->verify_cookie_len == 0; - MBEDTLS_SSL_DEBUG_MSG( 1, ( "good ClientHello with %s cookie", - cookie_is_good ? "good" : "bad" ) ); - - /* Send HelloVerifyRequest? */ - if( !cookie_is_good ) - { - ret = mbedtls_ssl_handshake_step( &tmp_ssl ); - if( ret != 0 ) - MBEDTLS_SSL_DEBUG_RET( 1, "nested handshake_step", ret ); - goto cleanup; - } - - /* Reset context while preserving some information */ - ret = ssl_session_reset_int( ssl, 1 ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "reset", ret ); - goto cleanup; - } - - ret = MBEDTLS_ERR_SSL_CLIENT_RECONNECT; - -cleanup: - mbedtls_ssl_free( &tmp_ssl ); - - if( ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) - ret = MBEDTLS_ERR_SSL_INVALID_RECORD; - return( ret ); } -#endif /* MBEDTLS_SSL_PROTO_DTLS && - MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && - MBEDTLS_SSL_SRV_C */ +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE */ /* * ContentType type; @@ -3456,8 +3533,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) "expected %d, received %d", ssl->in_epoch, rec_epoch ) ); -#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && \ - defined(MBEDTLS_SSL_SRV_C) +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) /* * Check for an epoch 0 ClientHello. We can't use in_msg here to * access the first byte of record content (handshake type), as we @@ -3475,7 +3551,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( ssl_handle_possible_reconnect( ssl ) ); } else -#endif /* MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ +#endif /* MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE */ return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } From 22311ae62e591bc49e69a6a85cc794da2d8c704e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Sep 2015 11:22:58 +0200 Subject: [PATCH 12/23] Improve help message of ssl_*2.c --- programs/ssl/ssl_client2.c | 2 +- programs/ssl/ssl_server2.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 758ebf34a..c090db909 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -223,7 +223,7 @@ int main( void ) " debug_level=%%d default: 0 (disabled)\n" \ " nbio=%%d default: 0 (blocking I/O)\n" \ " options: 1 (non-blocking), 2 (added delays)\n" \ - " read_timeout=%%d default: 0 (no timeout)\n" \ + " read_timeout=%%d default: 0 ms (no timeout)\n" \ " max_resend=%%d default: 0 (no resend on timeout)\n" \ "\n" \ USAGE_DTLS \ diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 7fc9fa05f..bd4d1d1b4 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -301,7 +301,7 @@ int main( void ) " debug_level=%%d default: 0 (disabled)\n" \ " nbio=%%d default: 0 (blocking I/O)\n" \ " options: 1 (non-blocking), 2 (added delays)\n" \ - " read_timeout=%%d default: 0 (no timeout)\n" \ + " read_timeout=%%d default: 0 ms (no timeout)\n" \ "\n" \ USAGE_DTLS \ USAGE_COOKIES \ From 259db910238b67623305047bb491b5fc98d67829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Sep 2015 11:37:17 +0200 Subject: [PATCH 13/23] Add test without cookies Tune existing tests while at it --- tests/ssl-opt.sh | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index ba6e578c1..6387af573 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2869,26 +2869,38 @@ run_test "DTLS cookie: enabled, nbio" \ # Tests for client reconnecting from the same port with DTLS +not_with_valgrind # spurious resend run_test "DTLS client reconnect from same port: reference" \ - "$P_SRV dtls=1 exchanges=2" \ - "$P_CLI dtls=1 exchanges=2 debug_level=2" \ + "$P_SRV dtls=1 exchanges=2 read_timeout=1000" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000" \ 0 \ -C "resend" \ + -S "The operation timed out" \ -S "Client initiated reconnection from same port" +not_with_valgrind # spurious resend run_test "DTLS client reconnect from same port: reconnect" \ - "$P_SRV dtls=1 exchanges=2" \ - "$P_CLI dtls=1 exchanges=2 debug_level=2 reconnect_hard=1" \ + "$P_SRV dtls=1 exchanges=2 read_timeout=1000" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000 reconnect_hard=1" \ 0 \ -C "resend" \ + -S "The operation timed out" \ -s "Client initiated reconnection from same port" run_test "DTLS client reconnect from same port: reconnect, nbio" \ - "$P_SRV dtls=1 exchanges=2 nbio=2" \ - "$P_CLI dtls=1 exchanges=2 debug_level=2 reconnect_hard=1" \ + "$P_SRV dtls=1 exchanges=2 read_timeout=1000 nbio=2" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000 reconnect_hard=1" \ 0 \ + -S "The operation timed out" \ -s "Client initiated reconnection from same port" +run_test "DTLS client reconnect from same port: no cookies" \ + "$P_SRV dtls=1 exchanges=2 read_timeout=1000 cookies=0" \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000 reconnect_hard=1" \ + 1 \ + -s "The operation timed out" \ + -S "Client initiated reconnection from same port" + # Tests for various cases of client authentication with DTLS # (focused on handshake flows and message parsing) From ab05d23b29239d3af908505083f7ab684b8a6d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Sep 2015 11:50:00 +0200 Subject: [PATCH 14/23] Update generated file --- library/version_features.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/version_features.c b/library/version_features.c index c2f30f21c..196b93c88 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -369,6 +369,9 @@ static const char *features[] = { #if defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY) "MBEDTLS_SSL_DTLS_HELLO_VERIFY", #endif /* MBEDTLS_SSL_DTLS_HELLO_VERIFY */ +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) + "MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE", +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE */ #if defined(MBEDTLS_SSL_DTLS_BADMAC_LIMIT) "MBEDTLS_SSL_DTLS_BADMAC_LIMIT", #endif /* MBEDTLS_SSL_DTLS_BADMAC_LIMIT */ From 2ed05a049ab0b565f5460148c0f7f01f89aed5ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Sep 2015 11:52:28 +0200 Subject: [PATCH 15/23] Fix typos --- library/ssl_tls.c | 2 +- programs/pkey/pk_sign.c | 2 +- programs/pkey/rsa_sign.c | 2 +- programs/pkey/rsa_verify.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 02cfaf1b9..b631e2609 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3551,7 +3551,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( ssl_handle_possible_reconnect( ssl ) ); } else -#endif /* MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE */ +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE */ return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } diff --git a/programs/pkey/pk_sign.c b/programs/pkey/pk_sign.c index 606fc93d7..322e8aff0 100644 --- a/programs/pkey/pk_sign.c +++ b/programs/pkey/pk_sign.c @@ -40,7 +40,7 @@ int main( void ) { mbedtls_printf("MBEDTLS_BIGNUM_C and/or MBEDTLS_ENTROPY_C and/or " - "MBEDTLS_SHA256_C and/or MBEDLTS_MD_C and/or " + "MBEDTLS_SHA256_C and/or MBEDTLS_MD_C and/or " "MBEDTLS_PK_PARSE_C and/or MBEDTLS_FS_IO and/or " "MBEDTLS_CTR_DRBG_C not defined.\n"); return( 0 ); diff --git a/programs/pkey/rsa_sign.c b/programs/pkey/rsa_sign.c index 469ad9f5d..e897c6519 100644 --- a/programs/pkey/rsa_sign.c +++ b/programs/pkey/rsa_sign.c @@ -40,7 +40,7 @@ int main( void ) { mbedtls_printf("MBEDTLS_BIGNUM_C and/or MBEDTLS_RSA_C and/or " - "MBEDLTS_MD_C and/or " + "MBEDTLS_MD_C and/or " "MBEDTLS_SHA256_C and/or MBEDTLS_FS_IO not defined.\n"); return( 0 ); } diff --git a/programs/pkey/rsa_verify.c b/programs/pkey/rsa_verify.c index 9d48a18ee..ade36dc83 100644 --- a/programs/pkey/rsa_verify.c +++ b/programs/pkey/rsa_verify.c @@ -39,7 +39,7 @@ int main( void ) { mbedtls_printf("MBEDTLS_BIGNUM_C and/or MBEDTLS_RSA_C and/or " - "MBEDLTS_MD_C and/or " + "MBEDTLS_MD_C and/or " "MBEDTLS_SHA256_C and/or MBEDTLS_FS_IO not defined.\n"); return( 0 ); } From ddfe5d20d1290d98cb00df7838d997c0e2e67086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Sep 2015 12:46:16 +0200 Subject: [PATCH 16/23] Tune dependencies Don't depend on srv.c in config.h, but add explicit checks. This is more in line with other options that only make sense server-side, and also it allows to test full config minus srv.c more easily. --- include/mbedtls/check_config.h | 2 +- include/mbedtls/config.h | 1 - library/ssl_tls.c | 8 ++++---- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 2280caba7..8dadbe1c5 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -422,7 +422,7 @@ #endif #if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && \ - ( !defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY) || !defined(MBEDTLS_SSL_SRV_C) ) + !defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY) #error "MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE defined, but not all prerequisites" #endif diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 381e82b11..9fdac600f 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1145,7 +1145,6 @@ * flag enables that support. * * Requires: MBEDTLS_SSL_DTLS_HELLO_VERIFY - * MBEDTLS_SSL_SRV_C * * Comment this to disable support for clients reusing the source port. */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b631e2609..66745d5af 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3250,7 +3250,7 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_DTLS_ANTI_REPLAY */ -#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) /* Forward declaration */ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); @@ -3437,7 +3437,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) return( ret ); } -#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE */ +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ /* * ContentType type; @@ -3533,7 +3533,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) "expected %d, received %d", ssl->in_epoch, rec_epoch ) ); -#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) /* * Check for an epoch 0 ClientHello. We can't use in_msg here to * access the first byte of record content (handshake type), as we @@ -3551,7 +3551,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( ssl_handle_possible_reconnect( ssl ) ); } else -#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE */ +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } From 4f6882a8a3d6871ef5027c260e91e1a22e5e598c Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Fri, 11 Sep 2015 17:12:46 +0100 Subject: [PATCH 17/23] Update config.h Typo in RFC x-ref comment. --- include/mbedtls/config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 9fdac600f..6e9d8f3df 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1141,7 +1141,7 @@ * * Some clients unexpectedly close the connection and try to reconnect using the * same source port. This needs special support from the server to handle the - * new connection securely, as described in section 4.1.8 of RFC 6347. This + * new connection securely, as described in section 4.2.8 of RFC 6347. This * flag enables that support. * * Requires: MBEDTLS_SSL_DTLS_HELLO_VERIFY From 1a57af16072d20f566d702c68824fe81af213fa8 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Fri, 11 Sep 2015 17:14:16 +0100 Subject: [PATCH 18/23] Update ssl.h Typo --- include/mbedtls/ssl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 7110e1ccf..512c76717 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2097,7 +2097,7 @@ int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, mbedtls_ssl_session * * \return 0 if successful, or * MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE, or - * MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED (see belowe), or + * MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED (see below), or * a specific SSL error code. * * \note If this function returns something other than 0 or From 0789aed39d545394e61e9edb0178cf46d08ded00 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Fri, 11 Sep 2015 17:15:17 +0100 Subject: [PATCH 19/23] Update ssl_tls.c Typo --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 66745d5af..fa1530be6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3257,7 +3257,7 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); /* * Without any SSL context, check if a datagram looks like a ClientHello with * a valid cookie, and if it doesn't, generate a HelloVerifyRequest message. - * Both input and input include full DTLS headers. + * Both input and output include full DTLS headers. * * - if cookie is valid, return 0 * - if ClientHello looks superficially valid but cookie is not, From 74ca8d07ada9514cec876365b35e0328da1357c7 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Fri, 11 Sep 2015 17:22:40 +0100 Subject: [PATCH 20/23] Update ssl_tls.c Clarification in comments to ssl_handle_possible_reconnect() --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fa1530be6..0e7bd4d20 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3396,7 +3396,7 @@ static int ssl_check_dtls_clihlo_cookie( * - if anything goes wrong, return a specific error code * * mbedtls_ssl_read_record() will ignore the record if anything else than - * MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned (we never return 0). + * MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned. */ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) { From d0bf6a38913b2f4762ea1ec75472dba4b4d6d596 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Fri, 11 Sep 2015 17:34:49 +0100 Subject: [PATCH 21/23] Update ssl_tls.c Clarification in comments --- library/ssl_tls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0e7bd4d20..2abd18b05 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3396,7 +3396,8 @@ static int ssl_check_dtls_clihlo_cookie( * - if anything goes wrong, return a specific error code * * mbedtls_ssl_read_record() will ignore the record if anything else than - * MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned. + * MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned, although this function + * cannot not return 0. */ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) { From 6ad23b98552ebf1f123fe52172a65700aac1610a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 15 Sep 2015 12:57:46 +0200 Subject: [PATCH 22/23] Make failing test more robust Let the client retry longer, to make sure the server will time out before the client gives up. Make it really longer to get a deterministic client exit status (make sure it has time to reconnect after the server timeout). --- tests/ssl-opt.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 6387af573..e49441301 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2896,8 +2896,8 @@ run_test "DTLS client reconnect from same port: reconnect, nbio" \ run_test "DTLS client reconnect from same port: no cookies" \ "$P_SRV dtls=1 exchanges=2 read_timeout=1000 cookies=0" \ - "$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000 reconnect_hard=1" \ - 1 \ + "$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-8000 reconnect_hard=1" \ + 0 \ -s "The operation timed out" \ -S "Client initiated reconnection from same port" From ea5370d4a2df4956b32f66007c820e4578328094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 15 Sep 2015 15:11:28 +0200 Subject: [PATCH 23/23] Don't allow reconnect during handshake Especially for resumed handshake, it's entirely possible for an epoch=0 ClientHello to be retransmitted or arrive so late that the server is already at epoch=1. There is no good way to detect whether it's that or a reconnect. However: - a late ClientHello seems more likely that client going down and then up again in the middle of a handshake - even if that's the case, we'll time out on that handshake soon enough - we don't want to break handshake flows that used to work So the safest option is to not treat that as a reconnect. --- library/ssl_tls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2abd18b05..463a6b115 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3542,6 +3542,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) * fact that the record header len is 13 instead. */ if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER && rec_epoch == 0 && ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && ssl->in_left > 13 &&