Add handshake hints for TLS 1.2 session tickets.

This runs through much the same code as the TLS 1.3 bits, though we use
a different hint field to avoid mixups between the fields. (Otherwise
the receiver may misinterpret a decryptPSK hint as the result of
decrypting the session_ticket extension, or vice versa. This could
happen if a ClientHello contains both a PSK and a session ticket.)

Bug: 504
Change-Id: I968bafe12120938e6e46e52536efd552b12c66a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53805
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>

5 files changed, 139 insertions, 28 deletions

diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 47434de..157d19f 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -3976,6 +3976,16 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
                                                       : ssl_ticket_aead_error;
   } else if (is_psk && hints && !hs->hints_requested && hints->ignore_psk) {
     result = ssl_ticket_aead_ignore_ticket;
+  } else if (!is_psk && hints && !hs->hints_requested &&
+             !hints->decrypted_ticket.empty()) {
+    if (plaintext.CopyFrom(hints->decrypted_ticket)) {
+      result = ssl_ticket_aead_success;
+      *out_renew_ticket = hints->renew_ticket;
+    } else {
+      result = ssl_ticket_aead_error;
+    }
+  } else if (!is_psk && hints && !hs->hints_requested && hints->ignore_ticket) {
+    result = ssl_ticket_aead_ignore_ticket;
   } else if (ssl->session_ctx->ticket_aead_method != NULL) {
     result = ssl_decrypt_ticket_with_method(hs, &plaintext, out_renew_ticket,
                                             ticket);
@@ -3994,12 +4004,24 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
     }
   }
 
-  if (is_psk && hints && hs->hints_requested) {
+  if (hints && hs->hints_requested) {
     if (result == ssl_ticket_aead_ignore_ticket) {
-      hints->ignore_psk = true;
-    } else if (result == ssl_ticket_aead_success &&
-               !hints->decrypted_psk.CopyFrom(plaintext)) {
-      return ssl_ticket_aead_error;
+      if (is_psk) {
+        hints->ignore_psk = true;
+      } else {
+        hints->ignore_ticket = true;
+      }
+    } else if (result == ssl_ticket_aead_success) {
+      if (is_psk) {
+        if (!hints->decrypted_psk.CopyFrom(plaintext)) {
+          return ssl_ticket_aead_error;
+        }
+      } else {
+        if (!hints->decrypted_ticket.CopyFrom(plaintext)) {
+          return ssl_ticket_aead_error;
+        }
+        hints->renew_ticket = *out_renew_ticket;
+      }
     }
   }
 
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index e414705..736ab47 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -774,8 +774,8 @@ int SSL_request_handshake_hints(SSL *ssl, const uint8_t *client_hello,
 //     signatureHint           [2] IMPLICIT SignatureHint OPTIONAL,
 //     -- At most one of decryptedPSKHint or ignorePSKHint may be present. It
 //     -- corresponds to the first entry in pre_shared_keys. TLS 1.2 session
-//     -- tickets will use a separate hint, to ensure the caller does not mix
-//     -- them up.
+//     -- tickets use a separate hint, to ensure the caller does not apply the
+//     -- hint to the wrong field.
 //     decryptedPSKHint        [3] IMPLICIT OCTET STRING OPTIONAL,
 //     ignorePSKHint           [4] IMPLICIT NULL OPTIONAL,
 //     compressCertificateHint [5] IMPLICIT CompressCertificateHint OPTIONAL,
@@ -783,8 +783,13 @@ int SSL_request_handshake_hints(SSL *ssl, const uint8_t *client_hello,
 //     -- a timestamp while the other doesn't. If the hint was generated
 //     -- assuming TLS 1.3 but we actually negotiate TLS 1.2, mixing the two
 //     -- will break this.
-//     serverRandomTLS12       [7] IMPLICIT OCTET STRING OPTIONAL,
-//     ecdheHint               [6] IMPLICIT ECDHEHint OPTIONAL
+//     serverRandomTLS12       [6] IMPLICIT OCTET STRING OPTIONAL,
+//     ecdheHint               [7] IMPLICIT ECDHEHint OPTIONAL
+//     -- At most one of decryptedTicketHint or ignoreTicketHint may be present.
+//     -- renewTicketHint requires decryptedTicketHint.
+//     decryptedTicketHint     [8] IMPLICIT OCTET STRING OPTIONAL,
+//     renewTicketHint         [9] IMPLICIT NULL OPTIONAL,
+//     ignoreTicketHint       [10] IMPLICIT NULL OPTIONAL,
 // }
 //
 // KeyShareHint ::= SEQUENCE {
@@ -823,6 +828,9 @@ static const unsigned kIgnorePSKTag = CBS_ASN1_CONTEXT_SPECIFIC | 4;
 static const unsigned kCompressCertificateTag = CBS_ASN1_CONTEXT_SPECIFIC | 5;
 static const unsigned kServerRandomTLS12Tag = CBS_ASN1_CONTEXT_SPECIFIC | 6;
 static const unsigned kECDHEHintTag = CBS_ASN1_CONSTRUCTED | 7;
+static const unsigned kDecryptedTicketTag = CBS_ASN1_CONTEXT_SPECIFIC | 8;
+static const unsigned kRenewTicketTag = CBS_ASN1_CONTEXT_SPECIFIC | 9;
+static const unsigned kIgnoreTicketTag = CBS_ASN1_CONTEXT_SPECIFIC | 10;
 
 int SSL_serialize_handshake_hints(const SSL *ssl, CBB *out) {
   const SSL_HANDSHAKE *hs = ssl->s3->hs.get();
@@ -918,9 +926,40 @@ int SSL_serialize_handshake_hints(const SSL *ssl, CBB *out) {
     }
   }
 
+
+  if (!hints->decrypted_ticket.empty()) {
+    if (!CBB_add_asn1(&seq, &child, kDecryptedTicketTag) ||
+        !CBB_add_bytes(&child, hints->decrypted_ticket.data(),
+                       hints->decrypted_ticket.size())) {
+      return 0;
+    }
+  }
+
+  if (hints->renew_ticket &&  //
+      !CBB_add_asn1(&seq, &child, kRenewTicketTag)) {
+    return 0;
+  }
+
+  if (hints->ignore_ticket &&  //
+      !CBB_add_asn1(&seq, &child, kIgnoreTicketTag)) {
+    return 0;
+  }
+
   return CBB_flush(out);
 }
 
+static bool get_optional_implicit_null(CBS *cbs, bool *out_present,
+                                       unsigned tag) {
+  CBS value;
+  int present;
+  if (!CBS_get_optional_asn1(cbs, &value, &present, tag) ||
+      (present && CBS_len(&value) != 0)) {
+    return false;
+  }
+  *out_present = present;
+  return true;
+}
+
 int SSL_set_handshake_hints(SSL *ssl, const uint8_t *hints, size_t hints_len) {
   if (SSL_is_dtls(ssl)) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
@@ -932,10 +971,10 @@ int SSL_set_handshake_hints(SSL *ssl, const uint8_t *hints, size_t hints_len) {
     return 0;
   }
 
-  CBS cbs, seq, server_random_tls13, key_share, signature_hint, ticket,
-      ignore_psk, cert_compression, server_random_tls12, ecdhe;
-  int has_server_random_tls13, has_key_share, has_signature_hint, has_ticket,
-      has_ignore_psk, has_cert_compression, has_server_random_tls12, has_ecdhe;
+  CBS cbs, seq, server_random_tls13, key_share, signature_hint, psk,
+      cert_compression, server_random_tls12, ecdhe, ticket;
+  int has_server_random_tls13, has_key_share, has_signature_hint, has_psk,
+      has_cert_compression, has_server_random_tls12, has_ecdhe, has_ticket;
   CBS_init(&cbs, hints, hints_len);
   if (!CBS_get_asn1(&cbs, &seq, CBS_ASN1_SEQUENCE) ||
       !CBS_get_optional_asn1(&seq, &server_random_tls13,
@@ -944,14 +983,19 @@ int SSL_set_handshake_hints(SSL *ssl, const uint8_t *hints, size_t hints_len) {
                              kKeyShareHintTag) ||
       !CBS_get_optional_asn1(&seq, &signature_hint, &has_signature_hint,
                              kSignatureHintTag) ||
-      !CBS_get_optional_asn1(&seq, &ticket, &has_ticket, kDecryptedPSKTag) ||
-      !CBS_get_optional_asn1(&seq, &ignore_psk, &has_ignore_psk,
-                             kIgnorePSKTag) ||
+      !CBS_get_optional_asn1(&seq, &psk, &has_psk, kDecryptedPSKTag) ||
+      !get_optional_implicit_null(&seq, &hints_obj->ignore_psk,
+                                  kIgnorePSKTag) ||
       !CBS_get_optional_asn1(&seq, &cert_compression, &has_cert_compression,
                              kCompressCertificateTag) ||
       !CBS_get_optional_asn1(&seq, &server_random_tls12,
                              &has_server_random_tls12, kServerRandomTLS12Tag) ||
-      !CBS_get_optional_asn1(&seq, &ecdhe, &has_ecdhe, kECDHEHintTag)) {
+      !CBS_get_optional_asn1(&seq, &ecdhe, &has_ecdhe, kECDHEHintTag) ||
+      !CBS_get_optional_asn1(&seq, &ticket, &has_ticket, kDecryptedTicketTag) ||
+      !get_optional_implicit_null(&seq, &hints_obj->renew_ticket,
+                                  kRenewTicketTag) ||
+      !get_optional_implicit_null(&seq, &hints_obj->ignore_ticket,
+                                  kIgnoreTicketTag)) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_COULD_NOT_PARSE_HINTS);
     return 0;
   }
@@ -993,15 +1037,12 @@ int SSL_set_handshake_hints(SSL *ssl, const uint8_t *hints, size_t hints_len) {
     hints_obj->signature_algorithm = static_cast<uint16_t>(sig_alg);
   }
 
-  if (has_ticket && !hints_obj->decrypted_psk.CopyFrom(ticket)) {
+  if (has_psk && !hints_obj->decrypted_psk.CopyFrom(psk)) {
     return 0;
   }
-
-  if (has_ignore_psk) {
-    if (CBS_len(&ignore_psk) != 0) {
-      return 0;
-    }
-    hints_obj->ignore_psk = true;
+  if (has_psk && hints_obj->ignore_psk) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_COULD_NOT_PARSE_HINTS);
+    return 0;
   }
 
   if (has_cert_compression) {
@@ -1039,6 +1080,18 @@ int SSL_set_handshake_hints(SSL *ssl, const uint8_t *hints, size_t hints_len) {
     hints_obj->ecdhe_group_id = static_cast<uint16_t>(group_id);
   }
 
+  if (has_ticket && !hints_obj->decrypted_ticket.CopyFrom(ticket)) {
+    return 0;
+  }
+  if (has_ticket && hints_obj->ignore_ticket) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_COULD_NOT_PARSE_HINTS);
+    return 0;
+  }
+  if (!has_ticket && hints_obj->renew_ticket) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_COULD_NOT_PARSE_HINTS);
+    return 0;
+  }
+
   ssl->s3->hs->hints = std::move(hints_obj);
   return 1;
 }
diff --git a/ssl/internal.h b/ssl/internal.h
index 0a15ace..153c9ca 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1719,6 +1719,10 @@ struct SSL_HANDSHAKE_HINTS {
   uint16_t ecdhe_group_id = 0;
   Array<uint8_t> ecdhe_public_key;
   Array<uint8_t> ecdhe_private_key;
+
+  Array<uint8_t> decrypted_ticket;
+  bool renew_ticket = false;
+  bool ignore_ticket = false;
 };
 
 struct SSL_HANDSHAKE {
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 35086f8..3e12a0f 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -686,8 +686,7 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume,
       return false;
     }
 
-    // TODO(davidben): Make handshake hints skip TLS 1.2 ticket decryption.
-    if (SSL_version(ssl) == TLS1_3_VERSION && state->ticket_decrypt_done) {
+    if (state->ticket_decrypt_done) {
       fprintf(stderr,
               "Performed ticket decryption, but hint should have skipped it\n");
       return false;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 9291605..2b5d0e0 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -18893,7 +18893,7 @@ func addHintMismatchTests() {
 		// We run the first connection with tickets enabled, so the client is
 		// issued a ticket, then disable tickets on the second connection.
 		testCases = append(testCases, testCase{
-			name:               protocol.String() + "-HintMismatch-NoTickets1",
+			name:               protocol.String() + "-HintMismatch-NoTickets1-TLS13",
 			testType:           serverTest,
 			protocol:           protocol,
 			skipSplitHandshake: true,
@@ -18909,7 +18909,7 @@ func addHintMismatchTests() {
 			expectResumeRejected: true,
 		})
 		testCases = append(testCases, testCase{
-			name:               protocol.String() + "-HintMismatch-NoTickets2",
+			name:               protocol.String() + "-HintMismatch-NoTickets2-TLS13",
 			testType:           serverTest,
 			protocol:           protocol,
 			skipSplitHandshake: true,
@@ -18923,6 +18923,39 @@ func addHintMismatchTests() {
 			},
 			resumeSession: true,
 		})
+		if protocol != quic {
+			testCases = append(testCases, testCase{
+				name:               protocol.String() + "-HintMismatch-NoTickets1-TLS12",
+				testType:           serverTest,
+				protocol:           protocol,
+				skipSplitHandshake: true,
+				config: Config{
+					MinVersion: VersionTLS12,
+					MaxVersion: VersionTLS12,
+				},
+				flags: []string{
+					"-on-resume-allow-hint-mismatch",
+					"-on-shim-on-resume-no-ticket",
+				},
+				resumeSession:        true,
+				expectResumeRejected: true,
+			})
+			testCases = append(testCases, testCase{
+				name:               protocol.String() + "-HintMismatch-NoTickets2-TLS12",
+				testType:           serverTest,
+				protocol:           protocol,
+				skipSplitHandshake: true,
+				config: Config{
+					MinVersion: VersionTLS12,
+					MaxVersion: VersionTLS12,
+				},
+				flags: []string{
+					"-on-resume-allow-hint-mismatch",
+					"-on-handshaker-on-resume-no-ticket",
+				},
+				resumeSession: true,
+			})
+		}
 
 		// The shim and handshaker may disagree on whether to request a client
 		// certificate.