Reject NewSessionTicket messages with empty tickets in TLS 1.3 (#2367)

In TLS 1.2, the ticket field could be empty to indicate the server
changed its mind on sending a ticket, having previously committed to
sending a NewSessionTicket message a round-trip ago.

In TLS 1.3, the server does not commit to sending NewSessionTicket. It
can always just not send it. So the ticket field is required to be
non-empty.

It's important we enforce this on the client because otherwise we
produce a mixed up SSL_SESSION object that looks like an ID session
(thanks to the placeholder ID field that was added for a still unfixed
Envoy bug, see b/200292207). That, in turn, confuses some code in
assembling the next ClientHello.

The subsequent CL will tighten that up.

Update-Note: BoringSSL TLS 1.3 clients will now correctly reject
zero-length tickets, rather than letting servers get us into a slightly
funny state.

Change-Id: I1651e7887f9611ebc44ac54af89c85bf86a9feff
Reviewed-on:
https://boringssl-review.googlesource.com/c/boringssl/+/73007
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>

### Issues:
Resolves #ISSUE-NUMBER1
Addresses #ISSUE-NUMBER2

### Description of changes: 
Describe AWS-LC’s current behavior and how your code changes that
behavior. If there are no issues this pr is resolving, explain why this
change is necessary.

### Call-outs:
Point out areas that need special attention or support during the review
process. Discuss architecture or design changes.

### Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any
testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

Co-authored-by: David Benjamin <[email protected]>

Author: justsmth

ssl/test/runner/runner.go

@@ -12442,7 +12442,7 @@ func addSessionTicketTests() {
 	testCases = append(testCases, testCase{
 		// In TLS 1.2 and below, empty NewSessionTicket messages
 		// mean the server changed its mind on sending a ticket.
-		name: "SendEmptySessionTicket",
+		name: "SendEmptySessionTicket-TLS12",
 		config: Config{
 			MaxVersion: VersionTLS12,
 			Bugs: ProtocolBugs{
@@ -12452,6 +12452,21 @@ func addSessionTicketTests() {
 		flags: []string{"-expect-no-session"},
 	})
 
+	testCases = append(testCases, testCase{
+		// In TLS 1.3, empty NewSessionTicket messages are not
+		// allowed.
+		name: "SendEmptySessionTicket-TLS13",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				SendEmptySessionTicket: true,
+			},
+		},
+		shouldFail:         true,
+		expectedError:      ":DECODE_ERROR:",
+		expectedLocalError: "remote error: error decoding message",
+	})
+
 	// Test that the server ignores unknown PSK modes.
 	testCases = append(testCases, testCase{
 		testType: serverTest,

ssl/tls13_client.cc

@@ -1074,8 +1074,9 @@ UniquePtr<SSL_SESSION> tls13_create_session_with_ticket(SSL *ssl, CBS *body) {
       !CBS_get_u32(body, &session->ticket_age_add) ||
       !CBS_get_u8_length_prefixed(body, &ticket_nonce) ||
       !CBS_get_u16_length_prefixed(body, &ticket) ||
+      CBS_len(&ticket) == 0 ||  //
       !session->ticket.CopyFrom(ticket) ||
-      !CBS_get_u16_length_prefixed(body, &extensions) ||
+      !CBS_get_u16_length_prefixed(body, &extensions) ||  //
       CBS_len(body) != 0) {
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);