Implement draft 16 HelloRetryRequest and cookie.
We'll never send cookies, but we'll echo them on request. Implement it
in runner as well and test.
BUG=98
Change-Id: Idd3799f1eaccd52ac42f5e2e5ae07c209318c270
Reviewed-on: https://boringssl-review.googlesource.com/11565
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h
index f615073..815831e 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -923,10 +923,16 @@
/* ecdh_ctx is the current ECDH instance. */
SSL_ECDH_CTX ecdh_ctx;
+ unsigned received_hello_retry_request:1;
+
/* retry_group is the group ID selected by the server in HelloRetryRequest in
* TLS 1.3. */
uint16_t retry_group;
+ /* cookie is the value of the cookie received from the server, if any. */
+ uint8_t *cookie;
+ size_t cookie_len;
+
/* key_share_bytes is the value of the previously sent KeyShare extension by
* the client in TLS 1.3. */
uint8_t *key_share_bytes;
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 2d5fc80..1e7e4e1 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -153,12 +153,13 @@
OPENSSL_cleanse(hs->server_traffic_secret_0,
sizeof(hs->server_traffic_secret_0));
SSL_ECDH_CTX_cleanup(&hs->ecdh_ctx);
- OPENSSL_free(hs->peer_key);
- OPENSSL_free(hs->server_params);
+ OPENSSL_free(hs->cookie);
OPENSSL_free(hs->key_share_bytes);
OPENSSL_free(hs->public_key);
OPENSSL_free(hs->peer_sigalgs);
OPENSSL_free(hs->peer_supported_group_list);
+ OPENSSL_free(hs->peer_key);
+ OPENSSL_free(hs->server_params);
OPENSSL_free(hs->peer_psk_identity_hint);
sk_X509_NAME_pop_free(hs->ca_names, X509_NAME_free);
OPENSSL_free(hs->certificate_types);
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 7031145..c9f5bbf 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2030,8 +2030,8 @@
}
uint16_t group_id;
- if (ssl->s3->hs->retry_group) {
- /* Append the new key share to the old list. */
+ if (ssl->s3->hs->received_hello_retry_request) {
+ /* Replay the old key shares. */
if (!CBB_add_bytes(&kse_bytes, ssl->s3->hs->key_share_bytes,
ssl->s3->hs->key_share_bytes_len)) {
return 0;
@@ -2041,6 +2041,12 @@
ssl->s3->hs->key_share_bytes_len = 0;
group_id = ssl->s3->hs->retry_group;
+
+ /* We received a HelloRetryRequest without a new curve, so there is no new
+ * share to append. Leave |ecdh_ctx| as-is. */
+ if (group_id == 0) {
+ return CBB_flush(out);
+ }
} else {
/* Add a fake group. See draft-davidben-tls-grease-01. */
if (ssl->ctx->grease_enabled &&
@@ -2072,7 +2078,7 @@
return 0;
}
- if (!ssl->s3->hs->retry_group) {
+ if (!ssl->s3->hs->received_hello_retry_request) {
/* Save the contents of the extension to repeat it in the second
* ClientHello. */
ssl->s3->hs->key_share_bytes_len = CBB_len(&kse_bytes);
@@ -2258,6 +2264,32 @@
}
+/* Cookie
+ *
+ * https://tools.ietf.org/html/draft-ietf-tls-tls13-16#section-4.2.2 */
+
+static int ext_cookie_add_clienthello(SSL *ssl, CBB *out) {
+ if (ssl->s3->hs->cookie == NULL) {
+ return 1;
+ }
+
+ CBB contents, cookie;
+ if (!CBB_add_u16(out, TLSEXT_TYPE_cookie) ||
+ !CBB_add_u16_length_prefixed(out, &contents) ||
+ !CBB_add_u16_length_prefixed(&contents, &cookie) ||
+ !CBB_add_bytes(&cookie, ssl->s3->hs->cookie, ssl->s3->hs->cookie_len) ||
+ !CBB_flush(out)) {
+ return 0;
+ }
+
+ /* The cookie is no longer needed in memory. */
+ OPENSSL_free(ssl->s3->hs->cookie);
+ ssl->s3->hs->cookie = NULL;
+ ssl->s3->hs->cookie_len = 0;
+ return 1;
+}
+
+
/* Negotiated Groups
*
* https://tools.ietf.org/html/rfc4492#section-5.1.2
@@ -2472,6 +2504,14 @@
ignore_parse_clienthello,
dont_add_serverhello,
},
+ {
+ TLSEXT_TYPE_cookie,
+ NULL,
+ ext_cookie_add_clienthello,
+ forbid_parse_serverhello,
+ ignore_parse_clienthello,
+ dont_add_serverhello,
+ },
/* The final extension must be non-empty. WebSphere Application Server 7.0 is
* intolerant to the last extension being zero-length. See
* https://crbug.com/363583. */
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index c03cedb..7878fc7 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -469,11 +469,6 @@
// than the negotiated one.
SendCurve CurveID
- // SendHelloRetryRequestCurve, if non-zero, causes the server to send
- // the specified curve in HelloRetryRequest rather than the negotiated
- // one.
- SendHelloRetryRequestCurve CurveID
-
// InvalidECDHPoint, if true, causes the ECC points in
// ServerKeyExchange or ClientKeyExchange messages to be invalid.
InvalidECDHPoint bool
@@ -906,6 +901,10 @@
// extension what will be added to NewSessionTicket in TLS 1.3.
CustomTicketExtension string
+ // CustomTicketExtension, if not empty, contains the contents of an
+ // extension what will be added to HelloRetryRequest in TLS 1.3.
+ CustomHelloRetryRequestExtension string
+
// NoCloseNotify, if true, causes the close_notify alert to be skipped
// on connection shutdown.
NoCloseNotify bool
@@ -1092,14 +1091,26 @@
// include the KeyShare extension in the EncryptedExtensions block.
EncryptedExtensionsWithKeyShare bool
- // UnnecessaryHelloRetryRequest, if true, causes the TLS 1.3 server to
- // send a HelloRetryRequest regardless of whether it needs to.
- UnnecessaryHelloRetryRequest bool
+ // AlwaysSendHelloRetryRequest, if true, causes a HelloRetryRequest to
+ // be sent by the server, even if empty.
+ AlwaysSendHelloRetryRequest bool
// SecondHelloRetryRequest, if true, causes the TLS 1.3 server to send
// two HelloRetryRequests instead of one.
SecondHelloRetryRequest bool
+ // SendHelloRetryRequestCurve, if non-zero, causes the server to send
+ // the specified curve in a HelloRetryRequest.
+ SendHelloRetryRequestCurve CurveID
+
+ // SendHelloRetryRequestCookie, if not nil, contains a cookie to be
+ // sent by the server in HelloRetryRequest.
+ SendHelloRetryRequestCookie []byte
+
+ // DuplicateHelloRetryRequestExtensions, if true, causes all
+ // HelloRetryRequest extensions to be sent twice.
+ DuplicateHelloRetryRequestExtensions bool
+
// SendServerHelloVersion, if non-zero, causes the server to send the
// specified value in ServerHello version field.
SendServerHelloVersion uint16
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 291a3b4..4b16f28 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -389,34 +389,41 @@
helloRetryRequest, haveHelloRetryRequest := msg.(*helloRetryRequestMsg)
var secondHelloBytes []byte
if haveHelloRetryRequest {
- var hrrCurveFound bool
+ if len(helloRetryRequest.cookie) > 0 {
+ hello.tls13Cookie = helloRetryRequest.cookie
+ }
+
if c.config.Bugs.MisinterpretHelloRetryRequestCurve != 0 {
+ helloRetryRequest.hasSelectedGroup = true
helloRetryRequest.selectedGroup = c.config.Bugs.MisinterpretHelloRetryRequestCurve
}
- group := helloRetryRequest.selectedGroup
- for _, curveID := range hello.supportedCurves {
- if group == curveID {
- hrrCurveFound = true
- break
+ if helloRetryRequest.hasSelectedGroup {
+ var hrrCurveFound bool
+ group := helloRetryRequest.selectedGroup
+ for _, curveID := range hello.supportedCurves {
+ if group == curveID {
+ hrrCurveFound = true
+ break
+ }
}
+ if !hrrCurveFound || keyShares[group] != nil {
+ c.sendAlert(alertHandshakeFailure)
+ return errors.New("tls: received invalid HelloRetryRequest")
+ }
+ curve, ok := curveForCurveID(group)
+ if !ok {
+ return errors.New("tls: Unable to get curve requested in HelloRetryRequest")
+ }
+ publicKey, err := curve.offer(c.config.rand())
+ if err != nil {
+ return err
+ }
+ keyShares[group] = curve
+ hello.keyShares = append(hello.keyShares, keyShareEntry{
+ group: group,
+ keyExchange: publicKey,
+ })
}
- if !hrrCurveFound || keyShares[group] != nil {
- c.sendAlert(alertHandshakeFailure)
- return errors.New("tls: received invalid HelloRetryRequest")
- }
- curve, ok := curveForCurveID(group)
- if !ok {
- return errors.New("tls: Unable to get curve requested in HelloRetryRequest")
- }
- publicKey, err := curve.offer(c.config.rand())
- if err != nil {
- return err
- }
- keyShares[group] = curve
- hello.keyShares = append(hello.keyShares, keyShareEntry{
- group: group,
- keyExchange: publicKey,
- })
if c.config.Bugs.SecondClientHelloMissingKeyShare {
hello.hasKeyShares = false
@@ -468,7 +475,7 @@
return fmt.Errorf("tls: server selected an unsupported cipher suite")
}
- if haveHelloRetryRequest && (helloRetryRequest.cipherSuite != serverHello.cipherSuite || helloRetryRequest.selectedGroup != serverHello.keyShare.group) {
+ if haveHelloRetryRequest && helloRetryRequest.hasSelectedGroup && helloRetryRequest.selectedGroup != serverHello.keyShare.group {
c.sendAlert(alertHandshakeFailure)
return errors.New("tls: ServerHello parameters did not match HelloRetryRequest")
}
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index b92735e..285587e 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -131,13 +131,11 @@
}
type clientHelloMsg struct {
- raw []byte
- isDTLS bool
- vers uint16
- random []byte
- sessionId []byte
- // TODO(davidben): Add support for TLS 1.3 cookies which are larger and
- // use an extension.
+ raw []byte
+ isDTLS bool
+ vers uint16
+ random []byte
+ sessionId []byte
cookie []byte
cipherSuites []uint16
compressionMethods []uint8
@@ -152,6 +150,7 @@
pskIdentities []pskIdentity
hasEarlyData bool
earlyDataContext []byte
+ tls13Cookie []byte
ticketSupported bool
sessionTicket []uint8
signatureAlgorithms []signatureAlgorithm
@@ -194,6 +193,7 @@
eqPSKIdentityLists(m.pskIdentities, m1.pskIdentities) &&
m.hasEarlyData == m1.hasEarlyData &&
bytes.Equal(m.earlyDataContext, m1.earlyDataContext) &&
+ bytes.Equal(m.tls13Cookie, m1.tls13Cookie) &&
m.ticketSupported == m1.ticketSupported &&
bytes.Equal(m.sessionTicket, m1.sessionTicket) &&
eqSignatureAlgorithms(m.signatureAlgorithms, m1.signatureAlgorithms) &&
@@ -334,6 +334,11 @@
context := earlyDataIndication.addU8LengthPrefixed()
context.addBytes(m.earlyDataContext)
}
+ if len(m.tls13Cookie) > 0 {
+ extensions.addU16(extensionCookie)
+ body := extensions.addU16LengthPrefixed()
+ body.addU16LengthPrefixed().addBytes(m.tls13Cookie)
+ }
if m.ticketSupported {
// http://tools.ietf.org/html/rfc5077#section-3.2
extensions.addU16(extensionSessionTicket)
@@ -666,6 +671,15 @@
}
m.hasEarlyData = true
m.earlyDataContext = data[1:length]
+ case extensionCookie:
+ if length < 2 {
+ return false
+ }
+ l := int(data[0])<<8 | int(data[1])
+ if l != length-2 || l == 0 {
+ return false
+ }
+ m.tls13Cookie = data[2 : 2+l]
case extensionSignatureAlgorithms:
// https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1
if length < 2 || length&1 != 0 {
@@ -1270,10 +1284,13 @@
}
type helloRetryRequestMsg struct {
- raw []byte
- vers uint16
- cipherSuite uint16
- selectedGroup CurveID
+ raw []byte
+ vers uint16
+ hasSelectedGroup bool
+ selectedGroup CurveID
+ cookie []byte
+ customExtension string
+ duplicateExtensions bool
}
func (m *helloRetryRequestMsg) marshal() []byte {
@@ -1285,10 +1302,29 @@
retryRequestMsg.addU8(typeHelloRetryRequest)
retryRequest := retryRequestMsg.addU24LengthPrefixed()
retryRequest.addU16(m.vers)
- retryRequest.addU16(m.cipherSuite)
- retryRequest.addU16(uint16(m.selectedGroup))
- // Extensions field. We have none to send.
- retryRequest.addU16(0)
+ extensions := retryRequest.addU16LengthPrefixed()
+
+ count := 1
+ if m.duplicateExtensions {
+ count = 2
+ }
+
+ for i := 0; i < count; i++ {
+ if m.hasSelectedGroup {
+ extensions.addU16(extensionKeyShare)
+ extensions.addU16(2) // length
+ extensions.addU16(uint16(m.selectedGroup))
+ }
+ if len(m.cookie) > 0 {
+ extensions.addU16(extensionCookie)
+ body := extensions.addU16LengthPrefixed()
+ body.addU16LengthPrefixed().addBytes(m.cookie)
+ }
+ if len(m.customExtension) > 0 {
+ extensions.addU16(extensionCustom)
+ extensions.addU16LengthPrefixed().addBytes([]byte(m.customExtension))
+ }
+ }
m.raw = retryRequestMsg.finish()
return m.raw
@@ -1296,17 +1332,48 @@
func (m *helloRetryRequestMsg) unmarshal(data []byte) bool {
m.raw = data
- if len(data) < 12 {
+ if len(data) < 8 {
return false
}
m.vers = uint16(data[4])<<8 | uint16(data[5])
- m.cipherSuite = uint16(data[6])<<8 | uint16(data[7])
- m.selectedGroup = CurveID(data[8])<<8 | CurveID(data[9])
- extLen := int(data[10])<<8 | int(data[11])
- data = data[12:]
- if len(data) != extLen {
+ extLen := int(data[6])<<8 | int(data[7])
+ data = data[8:]
+ if len(data) != extLen || len(data) == 0 {
return false
}
+ for len(data) > 0 {
+ if len(data) < 4 {
+ return false
+ }
+ extension := uint16(data[0])<<8 | uint16(data[1])
+ length := int(data[2])<<8 | int(data[3])
+ data = data[4:]
+ if len(data) < length {
+ return false
+ }
+
+ switch extension {
+ case extensionKeyShare:
+ if length != 2 {
+ return false
+ }
+ m.hasSelectedGroup = true
+ m.selectedGroup = CurveID(data[0])<<8 | CurveID(data[1])
+ case extensionCookie:
+ if length < 2 {
+ return false
+ }
+ cookieLen := int(data[0])<<8 | int(data[1])
+ if 2+cookieLen != length {
+ return false
+ }
+ m.cookie = data[2 : 2+cookieLen]
+ default:
+ // Unknown extensions are illegal from the server.
+ return false
+ }
+ data = data[length:]
+ }
return true
}
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 7b26cb6..eea6bf5 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -543,11 +543,32 @@
hs.hello.hasKeyShare = false
}
- // Resolve ECDHE and compute the handshake secret.
- var ecdheSecret []byte
+ firstHelloRetryRequest := true
+
+ResendHelloRetryRequest:
+ var sendHelloRetryRequest bool
+ helloRetryRequest := &helloRetryRequestMsg{
+ vers: versionToWire(c.vers, c.isDTLS),
+ duplicateExtensions: config.Bugs.DuplicateHelloRetryRequestExtensions,
+ }
+
+ if config.Bugs.AlwaysSendHelloRetryRequest {
+ sendHelloRetryRequest = true
+ }
+
+ if config.Bugs.SendHelloRetryRequestCookie != nil {
+ sendHelloRetryRequest = true
+ helloRetryRequest.cookie = config.Bugs.SendHelloRetryRequestCookie
+ }
+
+ if len(config.Bugs.CustomHelloRetryRequestExtension) > 0 {
+ sendHelloRetryRequest = true
+ helloRetryRequest.customExtension = config.Bugs.CustomHelloRetryRequestExtension
+ }
+
+ var selectedKeyShare *keyShareEntry
if hs.hello.hasKeyShare {
// Look for the key share corresponding to our selected curve.
- var selectedKeyShare *keyShareEntry
for i := range hs.clientHello.keyShares {
if hs.clientHello.keyShares[i].group == selectedCurve {
selectedKeyShare = &hs.clientHello.keyShares[i]
@@ -559,67 +580,80 @@
return errors.New("tls: expected missing key share")
}
- sendHelloRetryRequest := selectedKeyShare == nil
- if config.Bugs.UnnecessaryHelloRetryRequest {
+ if selectedKeyShare == nil {
+ helloRetryRequest.hasSelectedGroup = true
+ helloRetryRequest.selectedGroup = selectedCurve
sendHelloRetryRequest = true
}
- if config.Bugs.SkipHelloRetryRequest {
- sendHelloRetryRequest = false
+ }
+
+ if config.Bugs.SendHelloRetryRequestCurve != 0 {
+ helloRetryRequest.hasSelectedGroup = true
+ helloRetryRequest.selectedGroup = config.Bugs.SendHelloRetryRequestCurve
+ sendHelloRetryRequest = true
+ }
+
+ if config.Bugs.SkipHelloRetryRequest {
+ sendHelloRetryRequest = false
+ }
+
+ if sendHelloRetryRequest {
+ hs.writeServerHash(helloRetryRequest.marshal())
+ c.writeRecord(recordTypeHandshake, helloRetryRequest.marshal())
+ c.flushHandshake()
+
+ // Read new ClientHello.
+ newMsg, err := c.readHandshake()
+ if err != nil {
+ return err
}
- if sendHelloRetryRequest {
- firstTime := true
- ResendHelloRetryRequest:
- // Send HelloRetryRequest.
- helloRetryRequestMsg := helloRetryRequestMsg{
- vers: versionToWire(c.vers, c.isDTLS),
- cipherSuite: hs.hello.cipherSuite,
- selectedGroup: selectedCurve,
- }
- if config.Bugs.SendHelloRetryRequestCurve != 0 {
- helloRetryRequestMsg.selectedGroup = config.Bugs.SendHelloRetryRequestCurve
- }
- hs.writeServerHash(helloRetryRequestMsg.marshal())
- c.writeRecord(recordTypeHandshake, helloRetryRequestMsg.marshal())
- c.flushHandshake()
+ newClientHello, ok := newMsg.(*clientHelloMsg)
+ if !ok {
+ c.sendAlert(alertUnexpectedMessage)
+ return unexpectedMessageError(newClientHello, newMsg)
+ }
+ hs.writeClientHash(newClientHello.marshal())
- // Read new ClientHello.
- newMsg, err := c.readHandshake()
- if err != nil {
- return err
- }
- newClientHello, ok := newMsg.(*clientHelloMsg)
- if !ok {
- c.sendAlert(alertUnexpectedMessage)
- return unexpectedMessageError(newClientHello, newMsg)
- }
- hs.writeClientHash(newClientHello.marshal())
+ // Check that the new ClientHello matches the old ClientHello,
+ // except for relevant modifications.
+ //
+ // TODO(davidben): Make this check more precise.
+ oldClientHelloCopy := *hs.clientHello
+ oldClientHelloCopy.raw = nil
+ oldClientHelloCopy.hasEarlyData = false
+ oldClientHelloCopy.earlyDataContext = nil
+ newClientHelloCopy := *newClientHello
+ newClientHelloCopy.raw = nil
- // Check that the new ClientHello matches the old ClientHello, except for
- // the addition of the new KeyShareEntry at the end of the list, and
- // removing the EarlyDataIndication extension (if present).
- newKeyShares := newClientHello.keyShares
- if len(newKeyShares) == 0 || newKeyShares[len(newKeyShares)-1].group != selectedCurve {
+ if helloRetryRequest.hasSelectedGroup {
+ newKeyShares := newClientHelloCopy.keyShares
+ if len(newKeyShares) == 0 || newKeyShares[len(newKeyShares)-1].group != helloRetryRequest.selectedGroup {
return errors.New("tls: KeyShare from HelloRetryRequest not present in new ClientHello")
}
- oldClientHelloCopy := *hs.clientHello
- oldClientHelloCopy.raw = nil
- oldClientHelloCopy.hasEarlyData = false
- oldClientHelloCopy.earlyDataContext = nil
- newClientHelloCopy := *newClientHello
- newClientHelloCopy.raw = nil
- newClientHelloCopy.keyShares = newKeyShares[:len(newKeyShares)-1]
- if !oldClientHelloCopy.equal(&newClientHelloCopy) {
- return errors.New("tls: new ClientHello does not match")
- }
-
- if firstTime && config.Bugs.SecondHelloRetryRequest {
- firstTime = false
- goto ResendHelloRetryRequest
- }
-
selectedKeyShare = &newKeyShares[len(newKeyShares)-1]
+ newClientHelloCopy.keyShares = newKeyShares[:len(newKeyShares)-1]
}
+ if len(helloRetryRequest.cookie) > 0 {
+ if !bytes.Equal(newClientHelloCopy.tls13Cookie, helloRetryRequest.cookie) {
+ return errors.New("tls: cookie from HelloRetryRequest not present in new ClientHello")
+ }
+ newClientHelloCopy.tls13Cookie = nil
+ }
+
+ if !oldClientHelloCopy.equal(&newClientHelloCopy) {
+ return errors.New("tls: new ClientHello does not match")
+ }
+
+ if firstHelloRetryRequest && config.Bugs.SecondHelloRetryRequest {
+ firstHelloRetryRequest = false
+ goto ResendHelloRetryRequest
+ }
+ }
+
+ // Resolve ECDHE and compute the handshake secret.
+ var ecdheSecret []byte
+ if hs.hello.hasKeyShare {
// Once a curve has been selected and a key share identified,
// the server needs to generate a public value and send it in
// the ServerHello.
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 2256346..77f9a0d 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -3381,10 +3381,8 @@
config: Config{
MaxVersion: VersionTLS13,
MinVersion: VersionTLS13,
- // P-384 requires a HelloRetryRequest against
- // BoringSSL's default configuration. Assert
- // that we do indeed test this with
- // ExpectMissingKeyShare.
+ // P-384 requires a HelloRetryRequest against BoringSSL's default
+ // configuration. Assert this with ExpectMissingKeyShare.
CurvePreferences: []CurveID{CurveP384},
Bugs: ProtocolBugs{
ExpectMissingKeyShare: true,
@@ -8505,9 +8503,10 @@
testCases = append(testCases, testCase{
name: "UnnecessaryHelloRetryRequest",
config: Config{
- MaxVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ CurvePreferences: []CurveID{CurveX25519},
Bugs: ProtocolBugs{
- UnnecessaryHelloRetryRequest: true,
+ SendHelloRetryRequestCurve: CurveX25519,
},
},
shouldFail: true,
@@ -8529,6 +8528,97 @@
})
testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-Empty",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ AlwaysSendHelloRetryRequest: true,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":DECODE_ERROR:",
+ })
+
+ testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-DuplicateCurve",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ // P-384 requires a HelloRetryRequest against BoringSSL's default
+ // configuration. Assert this ExpectMissingKeyShare.
+ CurvePreferences: []CurveID{CurveP384},
+ Bugs: ProtocolBugs{
+ ExpectMissingKeyShare: true,
+ DuplicateHelloRetryRequestExtensions: true,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":DUPLICATE_EXTENSION:",
+ expectedLocalError: "remote error: illegal parameter",
+ })
+
+ testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-Cookie",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ SendHelloRetryRequestCookie: []byte("cookie"),
+ },
+ },
+ })
+
+ testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-DuplicateCookie",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ SendHelloRetryRequestCookie: []byte("cookie"),
+ DuplicateHelloRetryRequestExtensions: true,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":DUPLICATE_EXTENSION:",
+ expectedLocalError: "remote error: illegal parameter",
+ })
+
+ testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-EmptyCookie",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ SendHelloRetryRequestCookie: []byte{},
+ },
+ },
+ shouldFail: true,
+ expectedError: ":DECODE_ERROR:",
+ })
+
+ testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-Cookie-Curve",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ // P-384 requires HelloRetryRequest in BoringSSL.
+ CurvePreferences: []CurveID{CurveP384},
+ Bugs: ProtocolBugs{
+ SendHelloRetryRequestCookie: []byte("cookie"),
+ ExpectMissingKeyShare: true,
+ },
+ },
+ })
+
+ testCases = append(testCases, testCase{
+ name: "HelloRetryRequest-Unknown",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ CustomHelloRetryRequestExtension: "extension",
+ },
+ },
+ shouldFail: true,
+ expectedError: ":UNEXPECTED_EXTENSION:",
+ expectedLocalError: "remote error: unsupported extension",
+ })
+
+ testCases = append(testCases, testCase{
testType: serverTest,
name: "SecondClientHelloMissingKeyShare",
config: Config{
diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 69d2a35..87ccdc4 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -54,48 +54,110 @@
}
CBS cbs, extensions;
- uint16_t server_wire_version, cipher_suite, group_id;
+ uint16_t server_wire_version;
CBS_init(&cbs, ssl->init_msg, ssl->init_num);
if (!CBS_get_u16(&cbs, &server_wire_version) ||
- !CBS_get_u16(&cbs, &cipher_suite) ||
- !CBS_get_u16(&cbs, &group_id) ||
- /* We do not currently parse any HelloRetryRequest extensions. */
!CBS_get_u16_length_prefixed(&cbs, &extensions) ||
+ /* HelloRetryRequest may not be empty. */
+ CBS_len(&extensions) == 0 ||
CBS_len(&cbs) != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
return ssl_hs_error;
}
- /* TODO(svaldez): Don't do early_data on HelloRetryRequest. */
+ while (CBS_len(&extensions) != 0) {
+ uint16_t type;
+ CBS extension;
+ if (!CBS_get_u16(&extensions, &type) ||
+ !CBS_get_u16_length_prefixed(&extensions, &extension)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ return ssl_hs_error;
+ }
- const uint16_t *groups;
- size_t groups_len;
- tls1_get_grouplist(ssl, &groups, &groups_len);
- int found = 0;
- for (size_t i = 0; i < groups_len; i++) {
- if (groups[i] == group_id) {
- found = 1;
- break;
+ switch (type) {
+ case TLSEXT_TYPE_cookie: {
+ if (hs->cookie != NULL) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+
+ /* Cookies may be requested whether or not advertised, so no need to
+ * check. */
+
+ CBS cookie;
+ if (!CBS_get_u16_length_prefixed(&extension, &cookie) ||
+ CBS_len(&cookie) == 0 ||
+ CBS_len(&extension) != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ return ssl_hs_error;
+ }
+
+ if (!CBS_stow(&cookie, &hs->cookie, &hs->cookie_len)) {
+ return ssl_hs_error;
+ }
+ break;
+ }
+
+ case TLSEXT_TYPE_key_share: {
+ if (hs->retry_group != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+
+ /* key_share is always advertised, so no need to check. */
+
+ uint16_t group_id;
+ if (!CBS_get_u16(&extension, &group_id) ||
+ CBS_len(&extension) != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ return ssl_hs_error;
+ }
+
+ /* The group must be supported. */
+ const uint16_t *groups;
+ size_t groups_len;
+ tls1_get_grouplist(ssl, &groups, &groups_len);
+ int found = 0;
+ for (size_t i = 0; i < groups_len; i++) {
+ if (groups[i] == group_id) {
+ found = 1;
+ break;
+ }
+ }
+
+ if (!found) {
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
+ return ssl_hs_error;
+ }
+
+ /* Check that the HelloRetryRequest does not request the key share that
+ * was provided in the initial ClientHello. */
+ if (SSL_ECDH_CTX_get_id(&hs->ecdh_ctx) == group_id) {
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
+ return ssl_hs_error;
+ }
+
+ SSL_ECDH_CTX_cleanup(&hs->ecdh_ctx);
+ hs->retry_group = group_id;
+ break;
+ }
+
+ default:
+ OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION);
+ return ssl_hs_error;
}
}
- if (!found) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
- return ssl_hs_error;
- }
-
- /* Check that the HelloRetryRequest does not request the key share that was
- * provided in the initial ClientHello. */
- if (SSL_ECDH_CTX_get_id(&ssl->s3->hs->ecdh_ctx) == group_id) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
- return ssl_hs_error;
- }
-
- SSL_ECDH_CTX_cleanup(&ssl->s3->hs->ecdh_ctx);
- ssl->s3->hs->retry_group = group_id;
-
+ hs->received_hello_retry_request = 1;
hs->state = state_send_second_client_hello;
return ssl_hs_ok;
}
@@ -183,7 +245,7 @@
case TLSEXT_TYPE_key_share:
if (have_key_share) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
key_share = extension;
@@ -192,7 +254,7 @@
case TLSEXT_TYPE_pre_shared_key:
if (have_pre_shared_key) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
pre_shared_key = extension;
@@ -201,7 +263,7 @@
case TLSEXT_TYPE_signature_algorithms:
if (have_sigalgs) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
sigalgs = extension;
@@ -318,7 +380,7 @@
/* If there was no HelloRetryRequest, the version negotiation logic has
* already hashed the message. */
- if (ssl->s3->hs->retry_group != 0 &&
+ if (hs->received_hello_retry_request &&
!ssl->method->hash_current_message(ssl)) {
return ssl_hs_error;
}
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 532b708..cf3c9f7 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -254,10 +254,11 @@
if (!ssl->method->init_message(ssl, &cbb, &body,
SSL3_MT_HELLO_RETRY_REQUEST) ||
!CBB_add_u16(&body, ssl->version) ||
- !CBB_add_u16(&body, ssl_cipher_get_value(ssl->s3->tmp.new_cipher)) ||
!tls1_get_shared_group(ssl, &group_id) ||
- !CBB_add_u16(&body, group_id) ||
!CBB_add_u16_length_prefixed(&body, &extensions) ||
+ !CBB_add_u16(&extensions, TLSEXT_TYPE_key_share) ||
+ !CBB_add_u16(&extensions, 2 /* length */) ||
+ !CBB_add_u16(&extensions, group_id) ||
!ssl->method->finish_message(ssl, &cbb)) {
CBB_cleanup(&cbb);
return ssl_hs_error;
