Adding HelloRetryRequest. [Tests added by davidben.] Change-Id: I0d54a4f8b8fe91b348ff22658d95340cdb48b089 Reviewed-on: https://boringssl-review.googlesource.com/8850 Reviewed-by: Steven Valdez <svaldez@google.com> Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index e20b2d5..b2d31dc 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -206,6 +206,7 @@ TLSUnique []byte // the tls-unique channel binding SCTList []byte // signed certificate timestamp list PeerSignatureAlgorithm signatureAlgorithm // algorithm used by the peer in the handshake + CurveID CurveID // the curve used in ECDHE } // ClientAuthType declares the policy the server will follow for @@ -438,10 +439,16 @@ // or CertificateVerify message should be invalid. InvalidSignature bool - // SendCurve, if non-zero, causes the ServerKeyExchange message to use - // the specified curve ID rather than the negotiated one. + // SendCurve, if non-zero, causes the server to send the specified curve + // ID in ServerKeyExchange (TLS 1.2) or ServerHello (TLS 1.3) rather + // 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 @@ -953,6 +960,16 @@ // instead. MissingKeyShare bool + // SecondClientHelloMissingKeyShare, if true, causes the second TLS 1.3 + // ClientHello to skip sending a key_share extension and use the zero + // ECDHE secret instead. + SecondClientHelloMissingKeyShare bool + + // MisinterpretHelloRetryRequestCurve, if non-zero, causes the TLS 1.3 + // client to pretend the server requested a HelloRetryRequest with the + // given curve rather than the actual one. + MisinterpretHelloRetryRequestCurve CurveID + // DuplicateKeyShares, if true, causes the TLS 1.3 client to send two // copies of each KeyShareEntry. DuplicateKeyShares bool @@ -964,6 +981,22 @@ // EncryptedExtensionsWithKeyShare, if true, causes the TLS 1.3 server to // 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 + + // SecondHelloRetryRequest, if true, causes the TLS 1.3 server to send + // two HelloRetryRequests instead of one. + SecondHelloRetryRequest bool + + // SendServerHelloVersion, if non-zero, causes the server to send the + // specified version in ServerHello rather than the true version. + SendServerHelloVersion uint16 + + // SkipHelloRetryRequest, if true, causes the TLS 1.3 server to not send + // HelloRetryRequest. + SkipHelloRetryRequest bool } func (c *Config) serverInit() {
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index 3789b28..fbd501a 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go
@@ -53,6 +53,9 @@ // peerSignatureAlgorithm contains the signature algorithm that was used // by the peer in the handshake, or zero if not applicable. peerSignatureAlgorithm signatureAlgorithm + // curveID contains the curve that was used in the handshake, or zero if + // not applicable. + curveID CurveID clientRandom, serverRandom [32]byte exporterSecret []byte @@ -1500,6 +1503,7 @@ state.TLSUnique = c.firstFinished[:] state.SCTList = c.sctList state.PeerSignatureAlgorithm = c.peerSignatureAlgorithm + state.CurveID = c.curveID } return state
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 291fb75..50f3fa8 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -140,7 +140,7 @@ } if c.config.Bugs.MissingKeyShare { - hello.keyShares = nil + hello.hasKeyShares = false } } @@ -337,6 +337,9 @@ var secondHelloBytes []byte if haveHelloRetryRequest { var hrrCurveFound bool + if c.config.Bugs.MisinterpretHelloRetryRequestCurve != 0 { + helloRetryRequest.selectedGroup = c.config.Bugs.MisinterpretHelloRetryRequestCurve + } group := helloRetryRequest.selectedGroup for _, curveID := range hello.supportedCurves { if group == curveID { @@ -362,6 +365,10 @@ keyExchange: publicKey, }) + if c.config.Bugs.SecondClientHelloMissingKeyShare { + hello.hasKeyShares = false + } + hello.hasEarlyData = false hello.earlyDataContext = nil hello.raw = nil @@ -553,7 +560,7 @@ // Resolve ECDHE and compute the handshake secret. var ecdheSecret []byte - if hs.suite.flags&suiteECDHE != 0 && !c.config.Bugs.MissingKeyShare { + if hs.suite.flags&suiteECDHE != 0 && !c.config.Bugs.MissingKeyShare && !c.config.Bugs.SecondClientHelloMissingKeyShare { if !hs.serverHello.hasKeyShare { c.sendAlert(alertMissingExtension) return errors.New("tls: server omitted the key share extension") @@ -564,6 +571,7 @@ c.sendAlert(alertHandshakeFailure) return errors.New("tls: server selected an unsupported group") } + c.curveID = hs.serverHello.keyShare.group var err error ecdheSecret, err = curve.finish(hs.serverHello.keyShare.keyExchange) @@ -821,6 +829,9 @@ c.sendAlert(alertUnexpectedMessage) return err } + if ecdhe, ok := keyAgreement.(*ecdheKeyAgreement); ok { + c.curveID = ecdhe.curveID + } c.peerSignatureAlgorithm = keyAgreement.peerSignatureAlgorithm()
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 300ab50..f8b5dee 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go
@@ -266,6 +266,10 @@ vers: c.vers, } + if config.Bugs.SendServerHelloVersion != 0 { + hs.hello.vers = config.Bugs.SendServerHelloVersion + } + hs.hello.random = make([]byte, 32) if _, err := io.ReadFull(config.rand(), hs.hello.random); err != nil { c.sendAlert(alertInternalError) @@ -352,13 +356,25 @@ } } - if selectedKeyShare == nil { + sendHelloRetryRequest := selectedKeyShare == nil + if config.Bugs.UnnecessaryHelloRetryRequest { + sendHelloRetryRequest = true + } + if config.Bugs.SkipHelloRetryRequest { + sendHelloRetryRequest = false + } + if sendHelloRetryRequest { + firstTime := true + ResendHelloRetryRequest: // Send HelloRetryRequest. helloRetryRequestMsg := helloRetryRequestMsg{ vers: c.vers, 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()) @@ -392,26 +408,47 @@ return errors.New("tls: new ClientHello does not match") } + if firstTime && config.Bugs.SecondHelloRetryRequest { + firstTime = false + goto ResendHelloRetryRequest + } + selectedKeyShare = &newKeyShares[len(newKeyShares)-1] } // 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. - curve, ok := curveForCurveID(selectedKeyShare.group) + curve, ok := curveForCurveID(selectedCurve) if !ok { panic("tls: server failed to look up curve ID") } + c.curveID = selectedCurve + + var peerKey []byte + if config.Bugs.SkipHelloRetryRequest { + // If skipping HelloRetryRequest, use a random key to + // avoid crashing. + curve2, _ := curveForCurveID(selectedCurve) + var err error + peerKey, err = curve2.offer(config.rand()) + if err != nil { + return err + } + } else { + peerKey = selectedKeyShare.keyExchange + } + var publicKey []byte var err error - publicKey, ecdheSecret, err = curve.accept(config.rand(), selectedKeyShare.keyExchange) + publicKey, ecdheSecret, err = curve.accept(config.rand(), peerKey) if err != nil { c.sendAlert(alertHandshakeFailure) return err } hs.hello.hasKeyShare = true - curveID := selectedKeyShare.group + curveID := selectedCurve if c.config.Bugs.SendCurve != 0 { curveID = config.Bugs.SendCurve } @@ -648,6 +685,10 @@ compressionMethod: compressionNone, } + if config.Bugs.SendServerHelloVersion != 0 { + hs.hello.vers = config.Bugs.SendServerHelloVersion + } + hs.hello.random = make([]byte, 32) _, err = io.ReadFull(config.rand(), hs.hello.random) if err != nil { @@ -1009,6 +1050,9 @@ c.sendAlert(alertHandshakeFailure) return err } + if ecdhe, ok := keyAgreement.(*ecdheKeyAgreement); ok { + c.curveID = ecdhe.curveID + } if skx != nil && !config.Bugs.SkipServerKeyExchange { hs.writeServerHash(skx.marshal()) c.writeRecord(recordTypeHandshake, skx.marshal())
diff --git a/ssl/test/runner/key_agreement.go b/ssl/test/runner/key_agreement.go index e4a349f..ebfb93d 100644 --- a/ssl/test/runner/key_agreement.go +++ b/ssl/test/runner/key_agreement.go
@@ -484,13 +484,14 @@ return verifyMessage(ka.version, cert.PublicKey, config, sigAlg, msg, sig) } -// ecdheRSAKeyAgreement implements a TLS key agreement where the server +// ecdheKeyAgreement implements a TLS key agreement where the server // generates a ephemeral EC public/private key pair and signs it. The // pre-master secret is then calculated using ECDH. The signature may // either be ECDSA or RSA. type ecdheKeyAgreement struct { auth keyAgreementAuthentication curve ecdhCurve + curveID CurveID peerKey []byte } @@ -516,6 +517,7 @@ if ka.curve, ok = curveForCurveID(curveid); !ok { return nil, errors.New("tls: preferredCurves includes unsupported curve") } + ka.curveID = curveid publicKey, err := ka.curve.offer(config.rand()) if err != nil { @@ -554,6 +556,7 @@ return errors.New("tls: server selected unsupported curve") } curveid := CurveID(skx.key[1])<<8 | CurveID(skx.key[2]) + ka.curveID = curveid var ok bool if ka.curve, ok = curveForCurveID(curveid); !ok {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 92a2b6a..4997836 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -254,6 +254,9 @@ // expectedPeerSignatureAlgorithm, if not zero, is the signature // algorithm that the peer should have used in the handshake. expectedPeerSignatureAlgorithm signatureAlgorithm + // expectedCurveID, if not zero, is the curve that the handshake should + // have used. + expectedCurveID CurveID // messageLen is the length, in bytes, of the test message that will be // sent. messageLen int @@ -518,6 +521,10 @@ return fmt.Errorf("expected peer to use signature algorithm %04x, but got %04x", expected, connState.PeerSignatureAlgorithm) } + if expected := test.expectedCurveID; expected != 0 && expected != connState.CurveID { + return fmt.Errorf("expected peer to use curve %04x, but got %04x", expected, connState.CurveID) + } + if test.exportKeyingMaterial > 0 { actual := make([]byte, test.exportKeyingMaterial) if _, err := io.ReadFull(tlsConn, actual); err != nil { @@ -6304,6 +6311,8 @@ {"X25519", CurveX25519}, } +const bogusCurve = 0x1234 + func addCurveTests() { for _, curve := range testCurves { testCases = append(testCases, testCase{ @@ -6313,7 +6322,8 @@ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, CurvePreferences: []CurveID{curve.id}, }, - flags: []string{"-enable-all-curves"}, + flags: []string{"-enable-all-curves"}, + expectedCurveID: curve.id, }) testCases = append(testCases, testCase{ name: "CurveTest-Client-" + curve.name + "-TLS13", @@ -6322,7 +6332,8 @@ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, CurvePreferences: []CurveID{curve.id}, }, - flags: []string{"-enable-all-curves"}, + flags: []string{"-enable-all-curves"}, + expectedCurveID: curve.id, }) testCases = append(testCases, testCase{ testType: serverTest, @@ -6332,7 +6343,8 @@ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, CurvePreferences: []CurveID{curve.id}, }, - flags: []string{"-enable-all-curves"}, + flags: []string{"-enable-all-curves"}, + expectedCurveID: curve.id, }) testCases = append(testCases, testCase{ testType: serverTest, @@ -6342,12 +6354,12 @@ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, CurvePreferences: []CurveID{curve.id}, }, - flags: []string{"-enable-all-curves"}, + flags: []string{"-enable-all-curves"}, + expectedCurveID: curve.id, }) } // The server must be tolerant to bogus curves. - const bogusCurve = 0x1234 testCases = append(testCases, testCase{ testType: serverTest, name: "UnknownCurve", @@ -7359,7 +7371,8 @@ }) testCases = append(testCases, testCase{ - name: "MissingKeyShare-Server", + testType: serverTest, + name: "MissingKeyShare-Server", config: Config{ MaxVersion: VersionTLS13, Bugs: ProtocolBugs{ @@ -7432,6 +7445,158 @@ shouldFail: true, expectedLocalError: "remote error: unsupported extension", }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SendHelloRetryRequest", + config: Config{ + MaxVersion: VersionTLS13, + // Require a HelloRetryRequest for every curve. + DefaultCurves: []CurveID{}, + }, + expectedCurveID: CurveX25519, + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SendHelloRetryRequest-2", + config: Config{ + MaxVersion: VersionTLS13, + DefaultCurves: []CurveID{CurveP384}, + }, + // Although the ClientHello did not predict our preferred curve, + // we always select it whether it is predicted or not. + expectedCurveID: CurveX25519, + }) + + testCases = append(testCases, testCase{ + name: "UnknownCurve-HelloRetryRequest", + config: Config{ + MaxVersion: VersionTLS13, + // P-384 requires HelloRetryRequest in BoringSSL. + CurvePreferences: []CurveID{CurveP384}, + Bugs: ProtocolBugs{ + SendHelloRetryRequestCurve: bogusCurve, + }, + }, + shouldFail: true, + expectedError: ":WRONG_CURVE:", + }) + + testCases = append(testCases, testCase{ + name: "DisabledCurve-HelloRetryRequest", + config: Config{ + MaxVersion: VersionTLS13, + CurvePreferences: []CurveID{CurveP256}, + Bugs: ProtocolBugs{ + IgnorePeerCurvePreferences: true, + }, + }, + flags: []string{"-p384-only"}, + shouldFail: true, + expectedError: ":WRONG_CURVE:", + }) + + testCases = append(testCases, testCase{ + name: "UnnecessaryHelloRetryRequest", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + UnnecessaryHelloRetryRequest: true, + }, + }, + shouldFail: true, + expectedError: ":WRONG_CURVE:", + }) + + testCases = append(testCases, testCase{ + name: "SecondHelloRetryRequest", + config: Config{ + MaxVersion: VersionTLS13, + // P-384 requires HelloRetryRequest in BoringSSL. + CurvePreferences: []CurveID{CurveP384}, + Bugs: ProtocolBugs{ + SecondHelloRetryRequest: true, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_MESSAGE:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SecondClientHelloMissingKeyShare", + config: Config{ + MaxVersion: VersionTLS13, + DefaultCurves: []CurveID{}, + Bugs: ProtocolBugs{ + SecondClientHelloMissingKeyShare: true, + }, + }, + shouldFail: true, + expectedError: ":MISSING_KEY_SHARE:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SecondClientHelloWrongCurve", + config: Config{ + MaxVersion: VersionTLS13, + DefaultCurves: []CurveID{}, + Bugs: ProtocolBugs{ + MisinterpretHelloRetryRequestCurve: CurveP521, + }, + }, + shouldFail: true, + expectedError: ":WRONG_CURVE:", + }) + + testCases = append(testCases, testCase{ + name: "HelloRetryRequestVersionMismatch", + config: Config{ + MaxVersion: VersionTLS13, + // P-384 requires HelloRetryRequest in BoringSSL. + CurvePreferences: []CurveID{CurveP384}, + Bugs: ProtocolBugs{ + SendServerHelloVersion: 0x0305, + }, + }, + shouldFail: true, + expectedError: ":WRONG_VERSION_NUMBER:", + }) + + testCases = append(testCases, testCase{ + name: "HelloRetryRequestCurveMismatch", + config: Config{ + MaxVersion: VersionTLS13, + // P-384 requires HelloRetryRequest in BoringSSL. + CurvePreferences: []CurveID{CurveP384}, + Bugs: ProtocolBugs{ + // Send P-384 (correct) in the HelloRetryRequest. + SendHelloRetryRequestCurve: CurveP384, + // But send P-256 in the ServerHello. + SendCurve: CurveP256, + }, + }, + shouldFail: true, + expectedError: ":WRONG_CURVE:", + }) + + // Test the server selecting a curve that requires a HelloRetryRequest + // without sending it. + testCases = append(testCases, testCase{ + name: "SkipHelloRetryRequest", + config: Config{ + MaxVersion: VersionTLS13, + // P-384 requires HelloRetryRequest in BoringSSL. + CurvePreferences: []CurveID{CurveP384}, + Bugs: ProtocolBugs{ + SkipHelloRetryRequest: true, + }, + }, + shouldFail: true, + expectedError: ":WRONG_CURVE:", + }) } func worker(statusChan chan statusMsg, c chan *testCase, shimPath string, wg *sync.WaitGroup) {