Fix TLS 1.3 NewSessionTicket processing.

08b65f4e31f7eb63bad0e903e9f191bcc699eaca introduced a memory leak and
also got enums confused. Also fix a codepath that was missing an error
code.

Thanks to OSS-Fuzz which appears to have found it in a matter of hours.

Change-Id: Ia9e926c28a01daab3e6154d363d0acda91209a22
Reviewed-on: https://boringssl-review.googlesource.com/13104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

ssl/test/runner/common.go

diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 029fd4a..96bc39a 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -960,6 +960,10 @@
 	// extension of the NewSessionTicket message.
 	SendTicketEarlyDataInfo uint32
 
+	// DuplicateTicketEarlyDataInfo causes an extra empty extension of
+	// ticket_early_data_info to be sent in NewSessionTicket.
+	DuplicateTicketEarlyDataInfo bool
+
 	// ExpectTicketEarlyDataInfo, if true, means that the client will fail upon
 	// absence of the ticket_early_data_info extension.
 	ExpectTicketEarlyDataInfo bool

ssl/test/runner/conn.go

diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index 62a46bf..ee342fa 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1767,11 +1767,12 @@
 
 	// TODO(davidben): Allow configuring these values.
 	m := &newSessionTicketMsg{
-		version:         c.vers,
-		ticketLifetime:  uint32(24 * time.Hour / time.Second),
-		earlyDataInfo:   c.config.Bugs.SendTicketEarlyDataInfo,
-		customExtension: c.config.Bugs.CustomTicketExtension,
-		ticketAgeAdd:    ticketAgeAdd,
+		version:                c.vers,
+		ticketLifetime:         uint32(24 * time.Hour / time.Second),
+		earlyDataInfo:          c.config.Bugs.SendTicketEarlyDataInfo,
+		duplicateEarlyDataInfo: c.config.Bugs.DuplicateTicketEarlyDataInfo,
+		customExtension:        c.config.Bugs.CustomTicketExtension,
+		ticketAgeAdd:           ticketAgeAdd,
 	}
 
 	state := sessionState{

ssl/test/runner/handshake_messages.go

diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index a431cb5..62309b8 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -2003,14 +2003,15 @@
 }
 
 type newSessionTicketMsg struct {
-	raw                []byte
-	version            uint16
-	ticketLifetime     uint32
-	ticketAgeAdd       uint32
-	ticket             []byte
-	earlyDataInfo      uint32
-	customExtension    string
-	hasGREASEExtension bool
+	raw                    []byte
+	version                uint16
+	ticketLifetime         uint32
+	ticketAgeAdd           uint32
+	ticket                 []byte
+	earlyDataInfo          uint32
+	customExtension        string
+	duplicateEarlyDataInfo bool
+	hasGREASEExtension     bool
 }
 
 func (m *newSessionTicketMsg) marshal() []byte {
@@ -2035,6 +2036,10 @@
 		if m.earlyDataInfo > 0 {
 			extensions.addU16(extensionTicketEarlyDataInfo)
 			extensions.addU16LengthPrefixed().addU32(m.earlyDataInfo)
+			if m.duplicateEarlyDataInfo {
+				extensions.addU16(extensionTicketEarlyDataInfo)
+				extensions.addU16LengthPrefixed().addU32(m.earlyDataInfo)
+			}
 		}
 		if len(m.customExtension) > 0 {
 			extensions.addU16(extensionCustom)

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5c3d914..ba78fce 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8394,6 +8394,21 @@
 	})
 
 	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "TLS13-DuplicateTicketEarlyDataInfo",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				SendTicketEarlyDataInfo:      16384,
+				DuplicateTicketEarlyDataInfo: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":DUPLICATE_EXTENSION:",
+		expectedLocalError: "remote error: illegal parameter",
+	})
+
+	testCases = append(testCases, testCase{
 		testType: serverTest,
 		name:     "TLS13-ExpectTicketEarlyDataInfo",
 		config: Config{

ssl/tls13_client.c

diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 1d8bc54..6f2bb21 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -638,6 +638,7 @@
 }
 
 int tls13_process_new_session_ticket(SSL *ssl) {
+  int ret = 0;
   SSL_SESSION *session =
       SSL_SESSION_dup(ssl->s3->established_session,
                       SSL_SESSION_INCLUDE_NONAUTH);
@@ -655,10 +656,9 @@
       !CBS_stow(&ticket, &session->tlsext_tick, &session->tlsext_ticklen) ||
       !CBS_get_u16_length_prefixed(&cbs, &extensions) ||
       CBS_len(&cbs) != 0) {
-    SSL_SESSION_free(session);
     ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
-    return 0;
+    goto err;
   }
 
   /* Parse out the extensions. */
@@ -674,14 +674,15 @@
                             OPENSSL_ARRAY_SIZE(ext_types),
                             1 /* ignore unknown */)) {
     ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
-    return ssl_hs_error;
+    goto err;
   }
 
   if (have_early_data_info) {
     if (!CBS_get_u32(&early_data_info, &session->ticket_max_early_data) ||
         CBS_len(&early_data_info) != 0) {
       ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
-      return ssl_hs_error;
+      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+      goto err;
     }
   }
 
@@ -691,11 +692,14 @@
   if (ssl->ctx->new_session_cb != NULL &&
       ssl->ctx->new_session_cb(ssl, session)) {
     /* |new_session_cb|'s return value signals that it took ownership. */
-    return 1;
+    session = NULL;
   }
 
+  ret = 1;
+
+err:
   SSL_SESSION_free(session);
-  return 1;
+  return ret;
 }
 
 void ssl_clear_tls13_state(SSL_HANDSHAKE *hs) {