From e60ce9c4513c432705c84b0efebf1421ee769eee Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 18 Nov 2016 23:44:09 +0000 Subject: [PATCH] Update the record layer to use TLSv1.3 style record construction Reviewed-by: Rich Salz --- include/openssl/ssl.h | 1 + ssl/record/rec_layer_s3.c | 27 ++++++++++++++++++-- ssl/record/ssl3_record.c | 32 +++++++++++++++++++++++ ssl/ssl_err.c | 1 + test/sslcorrupttest.c | 8 +++++- util/TLSProxy/Proxy.pm | 2 +- util/TLSProxy/Record.pm | 53 +++++++++++++++++++++++++++++++++------ 7 files changed, 113 insertions(+), 11 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 8769f46ba2..840eb6e97d 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2326,6 +2326,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_BAD_LENGTH 271 # define SSL_R_BAD_PACKET_LENGTH 115 # define SSL_R_BAD_PROTOCOL_VERSION_NUMBER 116 +# define SSL_R_BAD_RECORD_TYPE 443 # define SSL_R_BAD_RSA_ENCRYPT 119 # define SSL_R_BAD_SIGNATURE 123 # define SSL_R_BAD_SRP_A_LENGTH 347 diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 925a835d9b..8adb3cdd08 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -790,8 +790,17 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, unsigned int version = s->version; unsigned char *compressdata; size_t maxcomplen; + unsigned int rectype; SSL3_RECORD_set_type(&wr[j], type); + /* + * In TLSv1.3, once encrypting, we always use application data for the + * record type + */ + if (SSL_IS_TLS13(s) && s->enc_write_ctx != NULL) + rectype = SSL3_RT_APPLICATION_DATA; + else + rectype = type; /* * Some servers hang if initial client hello is larger than 256 bytes * and record version number > TLS 1.0 @@ -803,7 +812,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, maxcomplen = pipelens[j] + (ssl_allow_compression(s) ? SSL3_RT_MAX_COMPRESSED_OVERHEAD : 0); /* write the header */ - if (!WPACKET_put_bytes_u8(&pkt[j], type) + if (!WPACKET_put_bytes_u8(&pkt[j], rectype) || !WPACKET_put_bytes_u16(&pkt[j], version) || !WPACKET_start_sub_packet_u16(&pkt[j]) || (eivlen > 0 @@ -827,6 +836,9 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, /* first we compress */ if (s->compress != NULL) { + /* + * TODO(TLS1.3): Make sure we prevent compression!!! + */ if (!ssl3_do_compress(s, &wr[j]) || !WPACKET_allocate_bytes(&pkt[j], wr[j].length, NULL)) { SSLerr(SSL_F_DO_SSL3_WRITE, SSL_R_COMPRESSION_FAILURE); @@ -840,6 +852,18 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, SSL3_RECORD_reset_input(&wr[j]); } + if (SSL_IS_TLS13(s) && s->enc_write_ctx != NULL) { + if (!WPACKET_put_bytes_u8(&pkt[j], type)) { + SSLerr(SSL_F_DO_SSL3_WRITE, ERR_R_INTERNAL_ERROR); + goto err; + } + SSL3_RECORD_add_length(&wr[j], 1); + /* + * TODO(TLS1.3): Padding goes here. Do we need an API to add this? + * For now, use no padding + */ + } + /* * we should still have the output to wr->data and the input from * wr->input. Length should be wr->length. wr->data still points in the @@ -878,7 +902,6 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf, SSL3_RECORD_set_data(&wr[j], recordstart); SSL3_RECORD_reset_input(&wr[j]); SSL3_RECORD_set_length(&wr[j], len); - } if (s->method->ssl3_enc->enc(s, wr, numpipes, 1) < 1) diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 6e9dcef8d2..bf6967624c 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -279,6 +279,13 @@ int ssl3_get_record(SSL *s) } } + if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL + && rr[num_recs].type != SSL3_RT_APPLICATION_DATA) { + SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE); + al = SSL_AD_UNEXPECTED_MESSAGE; + goto f_err; + } + if (rr[num_recs].length > SSL3_BUFFER_get_len(rbuf) - SSL3_RT_HEADER_LENGTH) { al = SSL_AD_RECORD_OVERFLOW; @@ -398,6 +405,7 @@ int ssl3_get_record(SSL *s) } enc_err = s->method->ssl3_enc->enc(s, rr, num_recs, 0); + /*- * enc_err is: * 0: (in non-constant time) if the record is publically invalid. @@ -504,6 +512,30 @@ int ssl3_get_record(SSL *s) } } + if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) { + size_t end; + + if (rr[j].length == 0) { + al = SSL_AD_UNEXPECTED_MESSAGE; + SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE); + goto f_err; + } + + /* Strip trailing padding */ + for (end = rr[j].length - 1; end > 0 && rr[j].data[end] == 0; end--) + continue; + + rr[j].length = end; + rr[j].type = rr[j].data[end]; + if (rr[j].type != SSL3_RT_APPLICATION_DATA + && rr[j].type != SSL3_RT_ALERT + && rr[j].type != SSL3_RT_HANDSHAKE) { + al = SSL_AD_UNEXPECTED_MESSAGE; + SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE); + goto f_err; + } + } + if (rr[j].length > SSL3_RT_MAX_PLAIN_LENGTH) { al = SSL_AD_RECORD_OVERFLOW; SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_DATA_LENGTH_TOO_LONG); diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 49a9d44b33..5c8e9d4fd8 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -357,6 +357,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = { {ERR_REASON(SSL_R_BAD_PACKET_LENGTH), "bad packet length"}, {ERR_REASON(SSL_R_BAD_PROTOCOL_VERSION_NUMBER), "bad protocol version number"}, + {ERR_REASON(SSL_R_BAD_RECORD_TYPE), "bad record type"}, {ERR_REASON(SSL_R_BAD_RSA_ENCRYPT), "bad rsa encrypt"}, {ERR_REASON(SSL_R_BAD_SIGNATURE), "bad signature"}, {ERR_REASON(SSL_R_BAD_SRP_A_LENGTH), "bad srp a length"}, diff --git a/test/sslcorrupttest.c b/test/sslcorrupttest.c index f07cfceda7..c1f074b11d 100644 --- a/test/sslcorrupttest.c +++ b/test/sslcorrupttest.c @@ -11,6 +11,8 @@ #include "ssltestlib.h" #include "testutil.h" +static int docorrupt = 0; + static void copy_flags(BIO *bio) { int flags; @@ -38,7 +40,7 @@ static int tls_corrupt_write(BIO *bio, const char *in, int inl) BIO *next = BIO_next(bio); char *copy; - if (in[0] == SSL3_RT_APPLICATION_DATA) { + if (docorrupt) { copy = BUF_memdup(in, inl); TEST_check(copy != NULL); /* corrupt last bit of application data */ @@ -186,6 +188,8 @@ static int test_ssl_corrupt(int testidx) STACK_OF(SSL_CIPHER) *ciphers; const SSL_CIPHER *currcipher; + docorrupt = 0; + printf("Starting Test %d, %s\n", testidx, cipher_list[testidx]); if (!create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(), &sctx, @@ -242,6 +246,8 @@ static int test_ssl_corrupt(int testidx) goto end; } + docorrupt = 1; + if (SSL_write(client, junk, sizeof(junk)) < 0) { printf("Unable to SSL_write\n"); ERR_print_errors_fp(stdout); diff --git a/util/TLSProxy/Proxy.pm b/util/TLSProxy/Proxy.pm index be9f8f88a0..ccfc5c9b2f 100644 --- a/util/TLSProxy/Proxy.pm +++ b/util/TLSProxy/Proxy.pm @@ -343,7 +343,7 @@ sub process_packet if ($record->flight != $self->flight) { next; } - $packet .= $record->reconstruct_record(); + $packet .= $record->reconstruct_record($server); } $self->{flight} = $self->{flight} + 1; diff --git a/util/TLSProxy/Record.pm b/util/TLSProxy/Record.pm index 5a35925aeb..fe78185ccc 100644 --- a/util/TLSProxy/Record.pm +++ b/util/TLSProxy/Record.pm @@ -116,6 +116,12 @@ sub get_records } else { $record->decrypt(); } + $record->encrypted(1); + } + + if (TLSProxy::Proxy->is_tls13()) { + print " Inner content type: " + .$record_type{$record->content_type()}."\n"; } push @record_list, $record; @@ -188,7 +194,8 @@ sub new decrypt_len => $decrypt_len, data => $data, decrypt_data => $decrypt_data, - orig_decrypt_data => $decrypt_data + orig_decrypt_data => $decrypt_data, + encrypted => 0 }; return bless $self, $class; @@ -257,6 +264,13 @@ sub decrypt() #Throw away the MAC or TAG $data = substr($data, 0, length($data) - $mactaglen); + if (TLSProxy::Proxy->is_tls13()) { + #Get the content type + my $content_type = unpack("C", substr($data, length($data) - 1)); + $self->content_type($content_type); + $data = substr($data, 0, length($data) - 1); + } + $self->decrypt_data($data); $self->decrypt_len(length($data)); @@ -267,15 +281,29 @@ sub decrypt() sub reconstruct_record { my $self = shift; + my $server = shift; my $data; + my $tls13_enc = 0; if ($self->sslv2) { $data = pack('n', $self->len | 0x8000); } else { - $data = pack('Cnn', $self->content_type, $self->version, $self->len); + if (TLSProxy::Proxy->is_tls13() && $self->encrypted) { + $data = pack('Cnn', RT_APPLICATION_DATA, $self->version, + $self->len + 1); + $tls13_enc = 1; + } else { + $data = pack('Cnn', $self->content_type, $self->version, + $self->len); + } + } $data .= $self->data; + if ($tls13_enc) { + $data .= pack('C', $self->content_type); + } + return $data; } @@ -285,11 +313,6 @@ sub flight my $self = shift; return $self->{flight}; } -sub content_type -{ - my $self = shift; - return $self->{content_type}; -} sub sslv2 { my $self = shift; @@ -347,4 +370,20 @@ sub version } return $self->{version}; } +sub content_type +{ + my $self = shift; + if (@_) { + $self->{content_type} = shift; + } + return $self->{content_type}; +} +sub encrypted +{ + my $self = shift; + if (@_) { + $self->{encrypted} = shift; + } + return $self->{encrypted}; +} 1; -- 2.34.1