[DTLS 1.3] Use HelloRetryRequest in place of HelloVerifyRequest.

This change has DTLS 1.3 clients reject connections from servers that
negotiate 1.3 after sending HVR. It also enables HRR tests in DTLS 1.3.

Bug: 42290594
Change-Id: I7bd5ab0e968e6b4c301a5697b1166703c28d9ebc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71348
Commit-Queue: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>

ssl/handshake.cc

@@ -149,7 +149,8 @@ SSL_HANDSHAKE::SSL_HANDSHAKE(SSL *ssl_arg)
       cert_compression_negotiated(false),
       apply_jdk11_workaround(false),
       can_release_private_key(false),
-      channel_id_negotiated(false) {
+      channel_id_negotiated(false),
+      received_hello_verify_request(false) {
   assert(ssl);
 
   // Draw entropy for all GREASE values at once. This avoids calling

ssl/handshake_client.cc

@@ -648,12 +648,6 @@ static enum ssl_hs_wait_t do_read_hello_verify_request(SSL_HANDSHAKE *hs) {
     return ssl_hs_ok;
   }
 
-  // TODO(crbug.com/boringssl/715): At the point when we read an HVR, we don't
-  // know whether the connection is DTLS 1.2 (or earlier) or DTLS 1.3 - that's
-  // determined when we read the supported_versions in the ServerHello. If we
-  // receive HVR and then the ServerHello selects DTLS 1.3, that is an error and
-  // we should close the connection.
-
   CBS hello_verify_request = msg.body, cookie;
   uint16_t server_version;
   if (!CBS_get_u16(&hello_verify_request, &server_version) ||
@@ -668,6 +662,7 @@ static enum ssl_hs_wait_t do_read_hello_verify_request(SSL_HANDSHAKE *hs) {
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
     return ssl_hs_error;
   }
+  hs->received_hello_verify_request = true;
 
   ssl->method->next_message(ssl);
 
@@ -738,15 +733,6 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) {
     return ssl_hs_error;
   }
 
-  // TODO(crbug.com/boringssl/715): Check that if the server picked DTLS 1.3,
-  // that it didn't also previously send an HVR, as that is not allowed by RFC
-  // 9147. (DTLS 1.25 still uses HVR instead of HRR.) Also add a runner test to
-  // test that we handle that case properly.
-  //
-  // See
-  // https://boringssl-review.googlesource.com/c/boringssl/+/68027/3/ssl/handshake_client.cc
-  // for an example of what this check might look like.
-
   assert(ssl->s3->have_version == ssl->s3->initial_handshake_complete);
   if (!ssl->s3->have_version) {
     ssl->version = server_version;
@@ -760,6 +746,13 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) {
     return ssl_hs_error;
   }
 
+  if (hs->received_hello_verify_request &&
+      ssl_protocol_version(ssl) > TLS1_2_VERSION) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_MESSAGE);
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_PROTOCOL_VERSION);
+    return ssl_hs_error;
+  }
+
   if (ssl_protocol_version(ssl) >= TLS1_3_VERSION) {
     hs->state = state_tls13;
     return ssl_hs_ok;

ssl/internal.h

@@ -2014,7 +2014,8 @@ struct SSL_HANDSHAKE {
 
   // dtls_cookie is the value of the cookie in DTLS HelloVerifyRequest. If
   // empty, either none was received or HelloVerifyRequest contained an empty
-  // cookie.
+  // cookie. Check the received_hello_verify_request field to distinguish an
+  // empty cookie from no HelloVerifyRequest message being received.
   Array<uint8_t> dtls_cookie;
 
   // ech_client_outer contains the outer ECH extension to send in the
@@ -2222,6 +2223,10 @@ struct SSL_HANDSHAKE {
   // handshake.
   bool channel_id_negotiated : 1;
 
+  // received_hello_verify_request is true if we received a HelloVerifyRequest
+  // message from the server.
+  bool received_hello_verify_request : 1;
+
   // client_version is the value sent or received in the ClientHello version.
   uint16_t client_version = 0;
 
@@ -3077,7 +3082,7 @@ struct DTLSEpochState {
   static constexpr bool kAllowUniquePtr = true;
 
   UniquePtr<SSLAEADContext> aead_write_ctx;
-  uint64_t write_sequence;
+  uint64_t write_sequence = 0;
 };
 
 struct DTLS1_STATE {

ssl/test/runner/common.go

@@ -713,6 +713,11 @@ type ProtocolBugs struct {
 	// HelloVerifyRequest message.
 	SkipHelloVerifyRequest bool
 
+	// ForceHelloVerifyRequest causes a DTLS server to send a
+	// HelloVerifyRequest message in DTLS 1.3 or other cases where it
+	// otherwise wouldn't.
+	ForceHelloVerifyRequest bool
+
 	// HelloVerifyRequestCookieLength, if non-zero, is the length of the cookie
 	// to request in HelloVerifyRequest.
 	HelloVerifyRequestCookieLength int

ssl/test/runner/conn.go

@@ -277,12 +277,17 @@ func (hc *halfConn) useTrafficSecret(version uint16, suite *cipherSuite, secret
 	}
 }
 
-// resetCipher changes the cipher state back to no encryption to be able
+// resetCipher resets the cipher state back to no encryption to be able
 // to send an unencrypted ClientHello in response to HelloRetryRequest
 // after 0-RTT data was rejected.
 func (hc *halfConn) resetCipher() {
+	// In all cases, the cipher is set to nil so that second ClientHello
+	// will be sent with no encryption (instead of with early data keys).
 	hc.cipher = nil
-	hc.incEpoch()
+	// TODO(crbug.com/42290594): When handling 0-RTT rejections in DTLS, we
+	// need to reset the epoch to 0 and reset the sequence number to where
+	// it was prior to sending early data (this is different than resetting
+	// it to 0).
 }
 
 // incSeq increments the sequence number.

ssl/test/runner/handshake_messages.go

@@ -1792,6 +1792,7 @@ func (m *clientEncryptedExtensionsMsg) unmarshal(data []byte) bool {
 }
 
 type helloRetryRequestMsg struct {
+	isDTLS                bool
 	raw                   []byte
 	vers                  uint16
 	sessionID             []byte
@@ -1814,7 +1815,11 @@ func (m *helloRetryRequestMsg) marshal() []byte {
 	retryRequestMsg := cryptobyte.NewBuilder(nil)
 	retryRequestMsg.AddUint8(typeServerHello)
 	retryRequestMsg.AddUint24LengthPrefixed(func(retryRequest *cryptobyte.Builder) {
-		retryRequest.AddUint16(VersionTLS12)
+		legacyVersion := uint16(VersionTLS12)
+		if m.isDTLS {
+			legacyVersion = VersionDTLS12
+		}
+		retryRequest.AddUint16(legacyVersion)
 		retryRequest.AddBytes(tls13HelloRetryRequest)
 		addUint8LengthPrefixedBytes(retryRequest, m.sessionID)
 		retryRequest.AddUint16(m.cipherSuite)

ssl/test/runner/handshake_server.go

@@ -150,6 +150,20 @@ func (c *Conn) serverHandshake() error {
 	return nil
 }
 
+func (c *Conn) shouldSendHelloVerifyRequest() bool {
+	if !c.isDTLS {
+		return false
+	}
+	if c.config.Bugs.ForceHelloVerifyRequest {
+		return true
+	}
+	if c.config.Bugs.SkipHelloVerifyRequest {
+		return false
+	}
+	// Don't send HVR for DTLS 1.3; do send it for DTLS <= 1.2.
+	return c.vers < VersionTLS13
+}
+
 // readClientHello reads a ClientHello message from the client and determines
 // the protocol version.
 func (hs *serverHandshakeState) readClientHello() error {
@@ -233,7 +247,64 @@ func (hs *serverHandshakeState) readClientHello() error {
 		}
 	}
 
-	if c.isDTLS && !config.Bugs.SkipHelloVerifyRequest {
+	c.clientVersion = hs.clientHello.vers
+
+	// Use the versions extension if supplied, otherwise use the legacy ClientHello version.
+	if len(hs.clientHello.supportedVersions) == 0 {
+		if c.isDTLS {
+			if hs.clientHello.vers <= VersionDTLS12 {
+				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionDTLS12)
+			}
+			if hs.clientHello.vers <= VersionDTLS10 {
+				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionDTLS10)
+			}
+		} else {
+			if hs.clientHello.vers >= VersionTLS12 {
+				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionTLS12)
+			}
+			if hs.clientHello.vers >= VersionTLS11 {
+				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionTLS11)
+			}
+			if hs.clientHello.vers >= VersionTLS10 {
+				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionTLS10)
+			}
+			if hs.clientHello.vers >= VersionSSL30 {
+				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionSSL30)
+			}
+		}
+	} else if config.Bugs.ExpectGREASE && !containsGREASE(hs.clientHello.supportedVersions) {
+		return errors.New("tls: no GREASE version value found")
+	}
+
+	if !c.haveVers {
+		if config.Bugs.NegotiateVersion != 0 {
+			c.wireVersion = config.Bugs.NegotiateVersion
+		} else {
+			var found bool
+			for _, vers := range hs.clientHello.supportedVersions {
+				if _, ok := config.isSupportedVersion(vers, c.isDTLS); ok {
+					c.wireVersion = vers
+					found = true
+					break
+				}
+			}
+			if !found {
+				c.sendAlert(alertProtocolVersion)
+				return errors.New("tls: client did not offer any supported protocol versions")
+			}
+		}
+	} else if config.Bugs.NegotiateVersionOnRenego != 0 {
+		c.wireVersion = config.Bugs.NegotiateVersionOnRenego
+	}
+
+	c.vers, ok = wireToVersion(c.wireVersion, c.isDTLS)
+	if !ok {
+		panic("Could not map wire version")
+	}
+
+	clientProtocol, ok := wireToVersion(c.clientVersion, c.isDTLS)
+
+	if c.shouldSendHelloVerifyRequest() {
 		// Per RFC 6347, the version field in HelloVerifyRequest SHOULD
 		// be always DTLS 1.0
 		cookieLen := c.config.Bugs.HelloVerifyRequestCookieLength
@@ -274,6 +345,21 @@ func (hs *serverHandshakeState) readClientHello() error {
 		}
 		hs.clientHello = newClientHello
 	}
+	// The version for the connection is selected before sending
+	// HelloVerifyRequest, but haveVers isn't set until after we send it and
+	// receive the second ClientHello. This is intentional because once the
+	// version for the Conn has been negotiated, we enforce that the version
+	// in the record layer matches the negotiated version.
+	//
+	// At the time the server has selected a version and decided to send a
+	// HelloVerifyRequest to the client, only the server knows the selected
+	// version. Even though there is a version field in HelloVerifyRequest,
+	// it is only used to indicate packet formatting and is not part of
+	// version negotiation. Thus, when the client sends its second
+	// ClientHello (in response to HelloVerifyRequest), it does not know the
+	// version selected for this connection. Therefore, this server can't
+	// enforce that the client used the correct version in the record layer.
+	c.haveVers = true
 
 	if config.Bugs.RequireSameRenegoClientVersion && c.clientVersion != 0 {
 		if c.clientVersion != hs.clientHello.vers {
@@ -301,64 +387,6 @@ func (hs *serverHandshakeState) readClientHello() error {
 		}
 	}
 
-	c.clientVersion = hs.clientHello.vers
-
-	// Use the versions extension if supplied, otherwise use the legacy ClientHello version.
-	if len(hs.clientHello.supportedVersions) == 0 {
-		if c.isDTLS {
-			if hs.clientHello.vers <= VersionDTLS12 {
-				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionDTLS12)
-			}
-			if hs.clientHello.vers <= VersionDTLS10 {
-				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionDTLS10)
-			}
-		} else {
-			if hs.clientHello.vers >= VersionTLS12 {
-				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionTLS12)
-			}
-			if hs.clientHello.vers >= VersionTLS11 {
-				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionTLS11)
-			}
-			if hs.clientHello.vers >= VersionTLS10 {
-				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionTLS10)
-			}
-			if hs.clientHello.vers >= VersionSSL30 {
-				hs.clientHello.supportedVersions = append(hs.clientHello.supportedVersions, VersionSSL30)
-			}
-		}
-	} else if config.Bugs.ExpectGREASE && !containsGREASE(hs.clientHello.supportedVersions) {
-		return errors.New("tls: no GREASE version value found")
-	}
-
-	if !c.haveVers {
-		if config.Bugs.NegotiateVersion != 0 {
-			c.wireVersion = config.Bugs.NegotiateVersion
-		} else {
-			var found bool
-			for _, vers := range hs.clientHello.supportedVersions {
-				if _, ok := config.isSupportedVersion(vers, c.isDTLS); ok {
-					c.wireVersion = vers
-					found = true
-					break
-				}
-			}
-			if !found {
-				c.sendAlert(alertProtocolVersion)
-				return errors.New("tls: client did not offer any supported protocol versions")
-			}
-		}
-	} else if config.Bugs.NegotiateVersionOnRenego != 0 {
-		c.wireVersion = config.Bugs.NegotiateVersionOnRenego
-	}
-
-	c.vers, ok = wireToVersion(c.wireVersion, c.isDTLS)
-	if !ok {
-		panic("Could not map wire version")
-	}
-	c.haveVers = true
-
-	clientProtocol, ok := wireToVersion(c.clientVersion, c.isDTLS)
-
 	// Reject < 1.2 ClientHellos with signature_algorithms.
 	if ok && clientProtocol < VersionTLS12 && len(hs.clientHello.signatureAlgorithms) > 0 {
 		return fmt.Errorf("tls: client included signature_algorithms before TLS 1.2")
@@ -700,6 +728,7 @@ ResendHelloRetryRequest:
 		cipherSuite = config.Bugs.SendHelloRetryRequestCipherSuite
 	}
 	helloRetryRequest := &helloRetryRequestMsg{
+		isDTLS:              c.isDTLS,
 		vers:                c.wireVersion,
 		sessionID:           hs.clientHello.sessionID,
 		cipherSuite:         cipherSuite,