Explicitly handle empty NewSessionTickets on the client.

RFC 5077 explicitly allows the server to change its mind and send no
ticket by sending an empty NewSessionTicket. See also upstream's
21b538d616b388fa0ce64ef54da3504253895cf8.

CBS_stow handles this case somewhat, so we won't get confused about
malloc(0) as upstream did. But we'll still fill in a bogus SHA-256
session ID, cache the session, and send a ClientHello with bogus session
ID but empty ticket extension. (The session ID field changes meaning
significantly when the ticket is or isn't empty. Non-empty means "ignore
the session ID, but echo if it resuming" while empty means "I support
tickets, but am offering this session ID".

The other behavior change is that a server which changes its mind on a
resumption handshake will no longer override the client's session cache
with a ticket-less session.

(This is kind of silly. Given that we don't get completely confused due
to CBS_stow, it might not be worth bothering with the rest. Mostly it
bugged me that we send an indicator session ID with no ticket.)

Change-Id: Id6b5bde1fe51aa3e1f453a948e59bfd1e2502db6
Reviewed-on: https://boringssl-review.googlesource.com/6340
Reviewed-by: Adam Langley <alangley@gmail.com>
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 31fe268..e34af59 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1427,10 +1427,7 @@
 
 int ssl3_get_new_session_ticket(SSL *s) {
   int ok, al;
-  long n;
-  CBS new_session_ticket, ticket;
-
-  n = s->method->ssl_get_message(
+  long n = s->method->ssl_get_message(
       s, SSL3_ST_CR_SESSION_TICKET_A, SSL3_ST_CR_SESSION_TICKET_B,
       SSL3_MT_NEWSESSION_TICKET, 16384, ssl_hash_message, &ok);
 
@@ -1438,6 +1435,24 @@
     return n;
   }
 
+  CBS new_session_ticket, ticket;
+  uint32_t ticket_lifetime_hint;
+  CBS_init(&new_session_ticket, s->init_msg, n);
+  if (!CBS_get_u32(&new_session_ticket, &ticket_lifetime_hint) ||
+      !CBS_get_u16_length_prefixed(&new_session_ticket, &ticket) ||
+      CBS_len(&new_session_ticket) != 0) {
+    al = SSL_AD_DECODE_ERROR;
+    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+    goto f_err;
+  }
+
+  if (CBS_len(&ticket) == 0) {
+    /* RFC 5077 allows a server to change its mind and send no ticket after
+     * negotiating the extension. Behave as if no ticket was sent. */
+    s->tlsext_ticket_expected = 0;
+    return 1;
+  }
+
   if (s->hit) {
     /* The server is sending a new ticket for an existing session. Sessions are
      * immutable once established, so duplicate all but the ticket of the
@@ -1459,22 +1474,12 @@
     s->session = new_session;
   }
 
-  CBS_init(&new_session_ticket, s->init_msg, n);
-
-  if (!CBS_get_u32(&new_session_ticket,
-                   &s->session->tlsext_tick_lifetime_hint) ||
-      !CBS_get_u16_length_prefixed(&new_session_ticket, &ticket) ||
-      CBS_len(&new_session_ticket) != 0) {
-    al = SSL_AD_DECODE_ERROR;
-    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
-    goto f_err;
-  }
-
   if (!CBS_stow(&ticket, &s->session->tlsext_tick,
                 &s->session->tlsext_ticklen)) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
     goto err;
   }
+  s->session->tlsext_tick_lifetime_hint = ticket_lifetime_hint;
 
   /* Generate a session ID for this session based on the session ticket. We use
    * the session ID mechanism for detecting ticket resumption. This also fits in
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 99412d8..87f713f 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -863,7 +863,7 @@
         (!SSL_session_reused(ssl) || config->expect_ticket_renewal);
     if (expect_new_session != GetTestState(ssl)->got_new_session) {
       fprintf(stderr,
-              "new session was%s established, but we expected the opposite\n",
+              "new session was%s cached, but we expected the opposite\n",
               GetTestState(ssl)->got_new_session ? "" : " not");
       return false;
     }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 3e5696d..7defec1 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -773,6 +773,15 @@
 	// NegotiateALPNAndNPN, if true, causes the server to negotiate both
 	// ALPN and NPN in the same connetion.
 	NegotiateALPNAndNPN bool
+
+	// SendEmptySessionTicket, if true, causes the server to send an empty
+	// session ticket.
+	SendEmptySessionTicket bool
+
+	// FailIfSessionOffered, if true, causes the server to fail any
+	// connections where the client offers a non-empty session ID or session
+	// ticket.
+	FailIfSessionOffered bool
 }
 
 func (c *Config) serverInit() {
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index aa91723..9647715 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -358,6 +358,10 @@
 		return false, errors.New("tls: offered resumption on renegotiation")
 	}
 
+	if c.config.Bugs.FailIfSessionOffered && (len(hs.clientHello.sessionTicket) > 0 || len(hs.clientHello.sessionId) > 0) {
+		return false, errors.New("tls: client offered a session ticket or ID")
+	}
+
 	if hs.checkForResumption() {
 		return true, nil
 	}
@@ -866,10 +870,12 @@
 
 	m := new(newSessionTicketMsg)
 
-	var err error
-	m.ticket, err = c.encryptTicket(&state)
-	if err != nil {
-		return err
+	if !c.config.Bugs.SendEmptySessionTicket {
+		var err error
+		m.ticket, err = c.encryptTicket(&state)
+		if err != nil {
+			return err
+		}
 	}
 
 	hs.writeServerHash(m.marshal())
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 189854e..01813f9 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1971,6 +1971,18 @@
 			// does not fail.
 			expectMessageDropped: true,
 		},
+		{
+			name: "SendEmptySessionTicket",
+			config: Config{
+				Bugs: ProtocolBugs{
+					SendEmptySessionTicket: true,
+					FailIfSessionOffered:   true,
+				},
+			},
+			flags:                []string{"-expect-no-session"},
+			resumeSession:        true,
+			expectResumeRejected: true,
+		},
 	}
 	testCases = append(testCases, basicTests...)
 }