Update the record layer to use TLSv1.3 style record construction

Reviewed-by: NRich Salz <rsalz@openssl.org>

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

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)

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);

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"},

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);

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;

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;