Check the second ClientHello's PSK binder on resumption.

We perform all our negotiation based on the first ClientHello (for
consistency with what |select_certificate_cb| observed), which is in the
transcript, so we can ignore most of the second one.

However, we ought to check the second PSK binder. That covers the client
key share, which we do consume. In particular, we'll want to check if it
we ever send half-RTT data on these connections (we do not currently do
this). It is also a tricky computation, so we enforce the peer handled
it correctly.

Tested that both Chrome and Firefox continue to interop with this check,
when configuring uncommon curve preferences that trigger HRR. (Normally
neither browser sees HRRs against BoringSSL servers.)

Update-Note: This does enforce some client behavior that we hadn't been
    enforcing previously. However, it only figures into TLS 1.3 (not many
    implementations yet), and only clients which hit HelloRetryRequest
    (rare), so this should be low risk.
Change-Id: I42126585ec0685d009542094192e674cbd22520d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37124
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>

crypto/err/ssl.errordata

@@ -72,6 +72,7 @@ SSL,284,HANDSHAKE_NOT_COMPLETE
 SSL,155,HTTPS_PROXY_REQUEST
 SSL,156,HTTP_REQUEST
 SSL,157,INAPPROPRIATE_FALLBACK
+SSL,303,INCONSISTENT_CLIENT_HELLO
 SSL,259,INVALID_ALPN_PROTOCOL
 SSL,158,INVALID_COMMAND
 SSL,256,INVALID_COMPRESSION_LIST

include/openssl/ssl.h

@@ -5035,6 +5035,7 @@ BSSL_NAMESPACE_END
 #define SSL_R_TOO_MUCH_READ_EARLY_DATA 300
 #define SSL_R_INVALID_DELEGATED_CREDENTIAL 301
 #define SSL_R_KEY_USAGE_BIT_INCORRECT 302
+#define SSL_R_INCONSISTENT_CLIENT_HELLO 303
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020

ssl/handshake_client.cc

@@ -326,7 +326,7 @@ bool ssl_write_client_hello(SSL_HANDSHAKE *hs) {
   // Now that the length prefixes have been computed, fill in the placeholder
   // PSK binder.
   if (hs->needs_psk_binder &&
-      !tls13_write_psk_binder(hs, msg.data(), msg.size())) {
+      !tls13_write_psk_binder(hs, MakeSpan(msg))) {
     return false;
   }
 

ssl/internal.h

@@ -596,9 +596,11 @@ class SSLTranscript {
   // is released.
   bool UpdateForHelloRetryRequest();
 
-  // CopyHashContext copies the hash context into |ctx| and returns true on
-  // success.
-  bool CopyHashContext(EVP_MD_CTX *ctx);
+  // CopyToHashContext initializes |ctx| with |digest| and the data thus far in
+  // the transcript. It returns true on success and false on failure. If the
+  // handshake buffer is still present, |digest| may be any supported digest.
+  // Otherwise, |digest| must match the transcript hash.
+  bool CopyToHashContext(EVP_MD_CTX *ctx, const EVP_MD *digest);
 
   Span<const uint8_t> buffer() {
     return MakeConstSpan(reinterpret_cast<const uint8_t *>(buffer_->data),
@@ -1309,7 +1311,7 @@ bool tls13_derive_session_psk(SSL_SESSION *session, Span<const uint8_t> nonce);
 // tls13_write_psk_binder calculates the PSK binder value and replaces the last
 // bytes of |msg| with the resulting value. It returns true on success, and
 // false on failure.
-bool tls13_write_psk_binder(SSL_HANDSHAKE *hs, uint8_t *msg, size_t len);
+bool tls13_write_psk_binder(SSL_HANDSHAKE *hs, Span<uint8_t> msg);
 
 // tls13_verify_psk_binder verifies that the handshake transcript, truncated up
 // to the binders has a valid signature using the value of |session|'s
@@ -1738,7 +1740,8 @@ bool ssl_ext_pre_shared_key_parse_serverhello(SSL_HANDSHAKE *hs,
                                               CBS *contents);
 bool ssl_ext_pre_shared_key_parse_clienthello(
     SSL_HANDSHAKE *hs, CBS *out_ticket, CBS *out_binders,
-    uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert, CBS *contents);
+    uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert,
+    const SSL_CLIENT_HELLO *client_hello, CBS *contents);
 bool ssl_ext_pre_shared_key_add_serverhello(SSL_HANDSHAKE *hs, CBB *out);
 
 // ssl_is_sct_list_valid does a shallow parse of the SCT list in |contents| and

ssl/ssl_transcript.cc

@@ -137,6 +137,7 @@
 
 #include <openssl/buf.h>
 #include <openssl/digest.h>
+#include <openssl/err.h>
 
 #include "internal.h"
 
@@ -205,8 +206,20 @@ bool SSLTranscript::UpdateForHelloRetryRequest() {
   return true;
 }
 
-bool SSLTranscript::CopyHashContext(EVP_MD_CTX *ctx) {
-  return EVP_MD_CTX_copy_ex(ctx, hash_.get());
+bool SSLTranscript::CopyToHashContext(EVP_MD_CTX *ctx, const EVP_MD *digest) {
+  const EVP_MD *transcript_digest = Digest();
+  if (transcript_digest != nullptr &&
+      EVP_MD_type(transcript_digest) == EVP_MD_type(digest)) {
+    return EVP_MD_CTX_copy_ex(ctx, hash_.get());
+  }
+
+  if (buffer_) {
+    return EVP_DigestInit_ex(ctx, digest, nullptr) &&
+           EVP_DigestUpdate(ctx, buffer_->data, buffer_->length);
+  }
+
+  OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+  return false;
 }
 
 bool SSLTranscript::Update(Span<const uint8_t> in) {

ssl/t1_lib.cc

@@ -1901,7 +1901,17 @@ bool ssl_ext_pre_shared_key_parse_serverhello(SSL_HANDSHAKE *hs,
 
 bool ssl_ext_pre_shared_key_parse_clienthello(
     SSL_HANDSHAKE *hs, CBS *out_ticket, CBS *out_binders,
-    uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert, CBS *contents) {
+    uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert,
+    const SSL_CLIENT_HELLO *client_hello, CBS *contents) {
+  // Verify that the pre_shared_key extension is the last extension in
+  // ClientHello.
+  if (CBS_data(contents) + CBS_len(contents) !=
+      client_hello->extensions + client_hello->extensions_len) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_PRE_SHARED_KEY_MUST_BE_LAST);
+    *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+    return false;
+  }
+
   // We only process the first PSK identity since we don't support pure PSK.
   CBS identities, binders;
   if (!CBS_get_u16_length_prefixed(contents, &identities) ||

ssl/test/runner/common.go

@@ -1459,6 +1459,14 @@ type ProtocolBugs struct {
 	// rejected. See draft-davidben-tls-grease-01.
 	ExpectGREASE bool
 
+	// OmitPSKsOnSecondClientHello, if true, causes the client to omit the
+	// PSK extension on the second ClientHello.
+	OmitPSKsOnSecondClientHello bool
+
+	// OnlyCorruptSecondPSKBinder, if true, causes the options below to
+	// only apply to the second PSK binder.
+	OnlyCorruptSecondPSKBinder bool
+
 	// SendShortPSKBinder, if true, causes the client to send a PSK binder
 	// that is one byte shorter than it should be.
 	SendShortPSKBinder bool

ssl/test/runner/handshake_client.go

@@ -125,7 +125,7 @@ func (c *Conn) clientHandshake() error {
 		srtpProtectionProfiles:  c.config.SRTPProtectionProfiles,
 		srtpMasterKeyIdentifier: c.config.Bugs.SRTPMasterKeyIdentifer,
 		customExtension:         c.config.Bugs.CustomExtension,
-		pskBinderFirst:          c.config.Bugs.PSKBinderFirst,
+		pskBinderFirst:          c.config.Bugs.PSKBinderFirst && !c.config.Bugs.OnlyCorruptSecondPSKBinder,
 		omitExtensions:          c.config.Bugs.OmitExtensions,
 		emptyExtensions:         c.config.Bugs.EmptyExtensions,
 		delegatedCredentials:    !c.config.Bugs.DisableDelegatedCredentials,
@@ -603,6 +603,12 @@ NextCipherSuite:
 		}
 
 		hello.hasEarlyData = c.config.Bugs.SendEarlyDataOnSecondClientHello
+		// The first ClientHello may have skipped this due to OnlyCorruptSecondPSKBinder.
+		hello.pskBinderFirst = c.config.Bugs.PSKBinderFirst
+		if c.config.Bugs.OmitPSKsOnSecondClientHello {
+			hello.pskIdentities = nil
+			hello.pskBinders = nil
+		}
 		hello.raw = nil
 
 		if len(hello.pskIdentities) > 0 {
@@ -1997,18 +2003,24 @@ func writeIntPadded(b []byte, x *big.Int) {
 }
 
 func generatePSKBinders(version uint16, hello *clientHelloMsg, pskCipherSuite *cipherSuite, psk, firstClientHello, helloRetryRequest []byte, config *Config) {
-	if config.Bugs.SendNoPSKBinder {
-		return
-	}
-
+	maybeCorruptBinder := !config.Bugs.OnlyCorruptSecondPSKBinder || len(firstClientHello) > 0
 	binderLen := pskCipherSuite.hash().Size()
-	if config.Bugs.SendShortPSKBinder {
-		binderLen--
-	}
-
 	numBinders := 1
-	if config.Bugs.SendExtraPSKBinder {
-		numBinders++
+	if maybeCorruptBinder {
+		if config.Bugs.SendNoPSKBinder {
+			// The binders may have been set from the previous
+			// ClientHello.
+			hello.pskBinders = nil
+			return
+		}
+
+		if config.Bugs.SendShortPSKBinder {
+			binderLen--
+		}
+
+		if config.Bugs.SendExtraPSKBinder {
+			numBinders++
+		}
 	}
 
 	// Fill hello.pskBinders with appropriate length arrays of zeros so the
@@ -2023,11 +2035,13 @@ func generatePSKBinders(version uint16, hello *clientHelloMsg, pskCipherSuite *c
 	binderSize := len(hello.pskBinders)*(binderLen+1) + 2
 	truncatedHello := helloBytes[:len(helloBytes)-binderSize]
 	binder := computePSKBinder(psk, version, resumptionPSKBinderLabel, pskCipherSuite, firstClientHello, helloRetryRequest, truncatedHello)
-	if config.Bugs.SendShortPSKBinder {
-		binder = binder[:binderLen]
-	}
-	if config.Bugs.SendInvalidPSKBinder {
-		binder[0] ^= 1
+	if maybeCorruptBinder {
+		if config.Bugs.SendShortPSKBinder {
+			binder = binder[:binderLen]
+		}
+		if config.Bugs.SendInvalidPSKBinder {
+			binder[0] ^= 1
+		}
 	}
 
 	for i := range hello.pskBinders {