diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index 8afc289a..52ff2123 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -548,9 +548,8 @@ end: return ret; } -static int ssl3_write_client_cipher_list(SSL *ssl, CBB *out, - uint16_t min_version, - uint16_t max_version) { +int ssl_write_client_cipher_list(SSL *ssl, CBB *out, uint16_t min_version, + uint16_t max_version) { /* Prepare disabled cipher masks. */ ssl_set_client_disabled(ssl); @@ -605,6 +604,45 @@ static int ssl3_write_client_cipher_list(SSL *ssl, CBB *out, return CBB_flush(out); } +int ssl_add_client_hello_body(SSL *ssl, CBB *body) { + uint16_t min_version, max_version; + if (!ssl_get_version_range(ssl, &min_version, &max_version)) { + return 0; + } + + /* Renegotiations do not participate in session resumption. */ + int has_session = ssl->session != NULL && + !ssl->s3->initial_handshake_complete; + + CBB child; + if (!CBB_add_u16(body, ssl->client_version) || + !CBB_add_bytes(body, ssl->s3->client_random, SSL3_RANDOM_SIZE) || + !CBB_add_u8_length_prefixed(body, &child) || + (has_session && + !CBB_add_bytes(&child, ssl->session->session_id, + ssl->session->session_id_length))) { + return 0; + } + + if (SSL_IS_DTLS(ssl)) { + if (!CBB_add_u8_length_prefixed(body, &child) || + !CBB_add_bytes(&child, ssl->d1->cookie, ssl->d1->cookie_len)) { + return 0; + } + } + + size_t header_len = + SSL_IS_DTLS(ssl) ? DTLS1_HM_HEADER_LENGTH : SSL3_HM_HEADER_LENGTH; + if (!ssl_write_client_cipher_list(ssl, body, min_version, max_version) || + !CBB_add_u8(body, 1 /* one compression method */) || + !CBB_add_u8(body, 0 /* null compression */) || + !ssl_add_clienthello_tlsext(ssl, body, header_len + CBB_len(body))) { + return 0; + } + + return 1; +} + static int ssl3_send_client_hello(SSL *ssl) { if (ssl->state == SSL3_ST_CW_CLNT_HELLO_B) { return ssl->method->write_message(ssl); @@ -654,34 +692,9 @@ static int ssl3_send_client_hello(SSL *ssl) { goto err; } - /* Renegotiations do not participate in session resumption. */ - int has_session = ssl->session != NULL && - !ssl->s3->initial_handshake_complete; - - CBB body, child; + CBB body; if (!ssl->method->init_message(ssl, &cbb, &body, SSL3_MT_CLIENT_HELLO) || - !CBB_add_u16(&body, ssl->client_version) || - !CBB_add_bytes(&body, ssl->s3->client_random, SSL3_RANDOM_SIZE) || - !CBB_add_u8_length_prefixed(&body, &child) || - (has_session && - !CBB_add_bytes(&child, ssl->session->session_id, - ssl->session->session_id_length))) { - goto err; - } - - if (SSL_IS_DTLS(ssl)) { - if (!CBB_add_u8_length_prefixed(&body, &child) || - !CBB_add_bytes(&child, ssl->d1->cookie, ssl->d1->cookie_len)) { - goto err; - } - } - - size_t header_len = - SSL_IS_DTLS(ssl) ? DTLS1_HM_HEADER_LENGTH : SSL3_HM_HEADER_LENGTH; - if (!ssl3_write_client_cipher_list(ssl, &body, min_version, max_version) || - !CBB_add_u8(&body, 1 /* one compression method */) || - !CBB_add_u8(&body, 0 /* null compression */) || - !ssl_add_clienthello_tlsext(ssl, &body, header_len + CBB_len(&body)) || + !ssl_add_client_hello_body(ssl, &body) || !ssl->method->finish_message(ssl, &cbb)) { goto err; } diff --git a/ssl/internal.h b/ssl/internal.h index eb8b8795..04017916 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -873,6 +873,11 @@ struct ssl_handshake_st { SSL_ECDH_CTX *groups; size_t groups_len; + /* retry_group is the group ID selected by the server in HelloRetryRequest. */ + uint16_t retry_group; + /* key_share_bytes is the value of the previously sent KeyShare extension. */ + uint8_t *key_share_bytes; + size_t key_share_bytes_len; uint8_t *public_key; size_t public_key_len; @@ -882,6 +887,8 @@ struct ssl_handshake_st { SSL_HANDSHAKE *ssl_handshake_new(enum ssl_hs_wait_t (*do_handshake)(SSL *ssl)); +void ssl_handshake_clear_groups(SSL_HANDSHAKE *hs); + /* ssl_handshake_free releases all memory associated with |hs|. */ void ssl_handshake_free(SSL_HANDSHAKE *hs); @@ -910,11 +917,14 @@ int tls13_prepare_finished(SSL *ssl); int ext_key_share_parse_serverhello(SSL *ssl, uint8_t **out_secret, size_t *out_secret_len, uint8_t *out_alert, CBS *contents); -int ext_key_share_parse_clienthello(SSL *ssl, uint8_t **out_secret, +int ext_key_share_parse_clienthello(SSL *ssl, + int *out_found, uint8_t **out_secret, size_t *out_secret_len, uint8_t *out_alert, CBS *contents); int ext_key_share_add_serverhello(SSL *ssl, CBB *out); +int ssl_add_client_hello_body(SSL *ssl, CBB *body); + /* SSLKEYLOGFILE functions. */ @@ -1228,6 +1238,9 @@ void ssl_get_compatible_server_ciphers(SSL *ssl, uint32_t *out_mask_k, STACK_OF(SSL_CIPHER) *ssl_get_ciphers_by_id(SSL *ssl); int ssl_verify_alarm_type(long type); +int ssl_write_client_cipher_list(SSL *ssl, CBB *out, uint16_t min_version, + uint16_t max_version); + int ssl3_get_finished(SSL *ssl); int ssl3_send_change_cipher_spec(SSL *ssl); void ssl3_cleanup_key_block(SSL *ssl); @@ -1331,6 +1344,13 @@ int tls1_generate_master_secret(SSL *ssl, uint8_t *out, const uint8_t *premaster char ssl_early_callback_init(struct ssl_early_callback_ctx *ctx); +/* tls1_get_grouplist sets |*out_group_ids| and |*out_group_ids_len| to the + * list of allowed group IDs. If |get_peer_groups| is non-zero, return the + * peer's group list. Otherwise, return the preferred list. */ +void tls1_get_grouplist(SSL *ssl, int get_peer_groups, + const uint16_t **out_group_ids, + size_t *out_group_ids_len); + /* tls1_check_group_id returns one if |group_id| is consistent with both our * and the peer's group preferences. Note: if called as the client, only our * preferences are checked; the peer (the server) does not send preferences. */ diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index f76d9f0a..7549240d 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -300,12 +300,9 @@ static const uint16_t kDefaultGroups[] = { #endif }; -/* tls1_get_grouplist sets |*out_group_ids| and |*out_group_ids_len| to the - * list of allowed group IDs. If |get_peer_groups| is non-zero, return the - * peer's group list. Otherwise, return the preferred list. */ -static void tls1_get_grouplist(SSL *ssl, int get_peer_groups, - const uint16_t **out_group_ids, - size_t *out_group_ids_len) { +void tls1_get_grouplist(SSL *ssl, int get_peer_groups, + const uint16_t **out_group_ids, + size_t *out_group_ids_len) { if (get_peer_groups) { /* Only clients send a supported group list, so this function is only * called on the server. */ @@ -1973,7 +1970,24 @@ static int ext_key_share_add_clienthello(SSL *ssl, CBB *out) { const uint16_t *groups; size_t groups_len; - tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len); + if (ssl->s3->hs->retry_group) { + /* Append the new key share to the old list. */ + if (!CBB_add_bytes(&kse_bytes, ssl->s3->hs->key_share_bytes, + ssl->s3->hs->key_share_bytes_len)) { + return 0; + } + OPENSSL_free(ssl->s3->hs->key_share_bytes); + ssl->s3->hs->key_share_bytes = NULL; + + groups = &ssl->s3->hs->retry_group; + groups_len = 1; + } else { + tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len); + /* Only send the top two preferred key shares. */ + if (groups_len > 2) { + groups_len = 2; + } + } ssl->s3->hs->groups = OPENSSL_malloc(groups_len * sizeof(SSL_ECDH_CTX)); if (ssl->s3->hs->groups == NULL) { @@ -1996,6 +2010,17 @@ static int ext_key_share_add_clienthello(SSL *ssl, CBB *out) { } } + if (!ssl->s3->hs->retry_group) { + /* Save the contents of the extension to repeat it in the second + * ClientHello. */ + ssl->s3->hs->key_share_bytes_len = CBB_len(&kse_bytes); + ssl->s3->hs->key_share_bytes = BUF_memdup(CBB_data(&kse_bytes), + CBB_len(&kse_bytes)); + if (ssl->s3->hs->key_share_bytes == NULL) { + return 0; + } + } + return CBB_flush(out); } @@ -2030,16 +2055,12 @@ int ext_key_share_parse_serverhello(SSL *ssl, uint8_t **out_secret, return 0; } - for (size_t i = 0; i < ssl->s3->hs->groups_len; i++) { - SSL_ECDH_CTX_cleanup(&ssl->s3->hs->groups[i]); - } - OPENSSL_free(ssl->s3->hs->groups); - ssl->s3->hs->groups = NULL; - + ssl_handshake_clear_groups(ssl->s3->hs); return 1; } -int ext_key_share_parse_clienthello(SSL *ssl, uint8_t **out_secret, +int ext_key_share_parse_clienthello(SSL *ssl, int *out_found, + uint8_t **out_secret, size_t *out_secret_len, uint8_t *out_alert, CBS *contents) { uint16_t group_id; @@ -2049,7 +2070,7 @@ int ext_key_share_parse_clienthello(SSL *ssl, uint8_t **out_secret, return 0; } - int found = 0; + *out_found = 0; while (CBS_len(&key_shares) > 0) { uint16_t id; CBS peer_key; @@ -2058,7 +2079,7 @@ int ext_key_share_parse_clienthello(SSL *ssl, uint8_t **out_secret, return 0; } - if (id != group_id || found) { + if (id != group_id || *out_found) { continue; } @@ -2078,10 +2099,10 @@ int ext_key_share_parse_clienthello(SSL *ssl, uint8_t **out_secret, } SSL_ECDH_CTX_cleanup(&group); - found = 1; + *out_found = 1; } - return found; + return 1; } int ext_key_share_add_serverhello(SSL *ssl, CBB *out) { diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index e20b2d51..b2d31dc2 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -206,6 +206,7 @@ type ConnectionState struct { 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 @@ type ProtocolBugs struct { // 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 @@ type ProtocolBugs struct { // 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 @@ type ProtocolBugs struct { // 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 3789b282..fbd501a0 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go @@ -53,6 +53,9 @@ type Conn struct { // 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 @@ func (c *Conn) ConnectionState() ConnectionState { 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 291fb752..50f3fa82 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -140,7 +140,7 @@ func (c *Conn) clientHandshake() error { } if c.config.Bugs.MissingKeyShare { - hello.keyShares = nil + hello.hasKeyShares = false } } @@ -337,6 +337,9 @@ NextCipherSuite: 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 @@ NextCipherSuite: keyExchange: publicKey, }) + if c.config.Bugs.SecondClientHelloMissingKeyShare { + hello.hasKeyShares = false + } + hello.hasEarlyData = false hello.earlyDataContext = nil hello.raw = nil @@ -553,7 +560,7 @@ func (hs *clientHandshakeState) doTLS13Handshake() error { // 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 @@ func (hs *clientHandshakeState) doTLS13Handshake() error { 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 @@ func (hs *clientHandshakeState) doFullHandshake() error { 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 300ab503..f8b5deed 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -266,6 +266,10 @@ func (hs *serverHandshakeState) doTLS13Handshake() error { 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 @@ Curves: } } - 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 @@ Curves: 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 @@ func (hs *serverHandshakeState) processClientHello() (isResume bool, err error) 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 @@ func (hs *serverHandshakeState) doFullHandshake() error { 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 e4a349fd..ebfb93dc 100644 --- a/ssl/test/runner/key_agreement.go +++ b/ssl/test/runner/key_agreement.go @@ -484,13 +484,14 @@ func (ka *signedKeyAgreement) verifyParameters(config *Config, clientHello *clie 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 @@ NextCandidate: 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 @@ func (ka *ecdheKeyAgreement) processServerKeyExchange(config *Config, clientHell 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 92a2b6ab..4997836f 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -254,6 +254,9 @@ type testCase struct { // 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 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool) er 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 @@ var testCurves = []struct { {"X25519", CurveX25519}, } +const bogusCurve = 0x1234 + func addCurveTests() { for _, curve := range testCurves { testCases = append(testCases, testCase{ @@ -6313,7 +6322,8 @@ func addCurveTests() { 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 @@ func addCurveTests() { 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 @@ func addCurveTests() { 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 @@ func addCurveTests() { 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 @@ func addTLS13HandshakeTests() { }) testCases = append(testCases, testCase{ - name: "MissingKeyShare-Server", + testType: serverTest, + name: "MissingKeyShare-Server", config: Config{ MaxVersion: VersionTLS13, Bugs: ProtocolBugs{ @@ -7432,6 +7445,158 @@ func addTLS13HandshakeTests() { 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) { diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c index 023dc0da..3cebdd39 100644 --- a/ssl/tls13_both.c +++ b/ssl/tls13_both.c @@ -40,6 +40,19 @@ SSL_HANDSHAKE *ssl_handshake_new(enum ssl_hs_wait_t (*do_handshake)(SSL *ssl)) { return hs; } +void ssl_handshake_clear_groups(SSL_HANDSHAKE *hs) { + if (hs->groups == NULL) { + return; + } + + for (size_t i = 0; i < hs->groups_len; i++) { + SSL_ECDH_CTX_cleanup(&hs->groups[i]); + } + OPENSSL_free(hs->groups); + hs->groups = NULL; + hs->groups_len = 0; +} + void ssl_handshake_free(SSL_HANDSHAKE *hs) { if (hs == NULL) { return; @@ -47,12 +60,8 @@ void ssl_handshake_free(SSL_HANDSHAKE *hs) { OPENSSL_cleanse(hs->secret, sizeof(hs->secret)); OPENSSL_cleanse(hs->traffic_secret_0, sizeof(hs->traffic_secret_0)); - if (hs->groups != NULL) { - for (size_t i = 0; i < hs->groups_len; i++) { - SSL_ECDH_CTX_cleanup(&hs->groups[i]); - } - OPENSSL_free(hs->groups); - } + ssl_handshake_clear_groups(hs); + OPENSSL_free(hs->key_share_bytes); OPENSSL_free(hs->public_key); OPENSSL_free(hs->cert_context); OPENSSL_free(hs); diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c index 1b5573ba..ead5b82e 100644 --- a/ssl/tls13_client.c +++ b/ssl/tls13_client.c @@ -28,7 +28,10 @@ enum client_hs_state_t { - state_process_server_hello = 0, + state_process_hello_retry_request = 0, + state_send_second_client_hello, + state_flush_second_client_hello, + state_process_server_hello, state_process_encrypted_extensions, state_process_certificate_request, state_process_server_certificate, @@ -43,6 +46,85 @@ enum client_hs_state_t { state_done, }; +static enum ssl_hs_wait_t do_process_hello_retry_request(SSL *ssl, + SSL_HANDSHAKE *hs) { + if (ssl->s3->tmp.message_type != SSL3_MT_HELLO_RETRY_REQUEST) { + hs->state = state_process_server_hello; + return ssl_hs_ok; + } + + CBS cbs, extensions; + uint16_t server_wire_version, cipher_suite, group_id; + 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) || + CBS_len(&cbs) != 0) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + return ssl_hs_error; + } + + /* TODO(svaldez): Don't do early_data on HelloRetryRequest. */ + + const uint16_t *groups; + size_t groups_len; + tls1_get_grouplist(ssl, 0 /* local groups */, &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; + } + + for (size_t i = 0; i < ssl->s3->hs->groups_len; i++) { + /* Check that the HelloRetryRequest does not request a key share that was + * provided in the initial ClientHello. + * + * TODO(svaldez): Don't enforce this check when the HelloRetryRequest is due + * to a cookie. */ + if (SSL_ECDH_CTX_get_id(&ssl->s3->hs->groups[i]) == 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_handshake_clear_groups(ssl->s3->hs); + ssl->s3->hs->retry_group = group_id; + + hs->state = state_send_second_client_hello; + return ssl_hs_ok; +} + +static enum ssl_hs_wait_t do_send_second_client_hello(SSL *ssl, + SSL_HANDSHAKE *hs) { + CBB cbb, body; + if (!ssl->method->init_message(ssl, &cbb, &body, SSL3_MT_CLIENT_HELLO) || + !ssl_add_client_hello_body(ssl, &body) || + !ssl->method->finish_message(ssl, &cbb)) { + CBB_cleanup(&cbb); + return ssl_hs_error; + } + + hs->state = state_flush_second_client_hello; + return ssl_hs_write_message; +} + +static enum ssl_hs_wait_t do_flush_second_client_hello(SSL *ssl, + SSL_HANDSHAKE *hs) { + hs->state = state_process_server_hello; + return ssl_hs_flush_and_read_message; +} + static enum ssl_hs_wait_t do_process_server_hello(SSL *ssl, SSL_HANDSHAKE *hs) { if (!tls13_check_message_type(ssl, SSL3_MT_SERVER_HELLO)) { return ssl_hs_error; @@ -58,6 +140,13 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL *ssl, SSL_HANDSHAKE *hs) { !CBS_get_u16_length_prefixed(&cbs, &extensions) || CBS_len(&cbs) != 0) { ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); + return ssl_hs_error; + } + + if (server_wire_version != ssl->version) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_VERSION_NUMBER); return ssl_hs_error; } @@ -169,6 +258,12 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL *ssl, SSL_HANDSHAKE *hs) { if (!tls13_advance_key_schedule(ssl, kZeroes, hash_len)) { return ssl_hs_error; } + } + + /* If there was no HelloRetryRequest, the version negotiation logic has + * already hashed the message. */ + if (ssl->s3->hs->retry_group != 0 && + !ssl->method->hash_current_message(ssl)) { return ssl_hs_error; } @@ -403,6 +498,15 @@ enum ssl_hs_wait_t tls13_client_handshake(SSL *ssl) { enum ssl_hs_wait_t ret = ssl_hs_error; enum client_hs_state_t state = hs->state; switch (state) { + case state_process_hello_retry_request: + ret = do_process_hello_retry_request(ssl, hs); + break; + case state_send_second_client_hello: + ret = do_send_second_client_hello(ssl, hs); + break; + case state_flush_second_client_hello: + ret = do_flush_second_client_hello(ssl, hs); + break; case state_process_server_hello: ret = do_process_server_hello(ssl, hs); break; diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c index 1a1e52a8..22392f09 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c @@ -29,6 +29,9 @@ enum server_hs_state_t { state_process_client_hello = 0, + state_send_hello_retry_request, + state_flush_hello_retry_request, + state_process_second_client_hello, state_send_server_hello, state_send_encrypted_extensions, state_send_certificate_request, @@ -43,6 +46,60 @@ enum server_hs_state_t { state_done, }; +static const uint8_t kZeroes[EVP_MAX_MD_SIZE] = {0}; + +static int resolve_psk_secret(SSL *ssl) { + SSL_HANDSHAKE *hs = ssl->s3->hs; + + if (ssl->s3->tmp.new_cipher->algorithm_auth != SSL_aPSK) { + return tls13_advance_key_schedule(ssl, kZeroes, hs->hash_len); + } + + /* TODO(davidben): Support PSK. */ + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return 0; +} + +static int resolve_ecdhe_secret(SSL *ssl, int *out_need_retry, + struct ssl_early_callback_ctx *early_ctx) { + *out_need_retry = 0; + SSL_HANDSHAKE *hs = ssl->s3->hs; + + if (ssl->s3->tmp.new_cipher->algorithm_mkey != SSL_kECDHE) { + return tls13_advance_key_schedule(ssl, kZeroes, hs->hash_len); + } + + const uint8_t *key_share_buf = NULL; + size_t key_share_len = 0; + CBS key_share; + if (!SSL_early_callback_ctx_extension_get(early_ctx, TLSEXT_TYPE_key_share, + &key_share_buf, &key_share_len)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_KEY_SHARE); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_MISSING_EXTENSION); + return ssl_hs_error; + } + + CBS_init(&key_share, key_share_buf, key_share_len); + int found_key_share; + uint8_t *dhe_secret; + size_t dhe_secret_len; + uint8_t alert; + if (!ext_key_share_parse_clienthello(ssl, &found_key_share, &dhe_secret, + &dhe_secret_len, &alert, &key_share)) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); + return 0; + } + + if (!found_key_share) { + *out_need_retry = 1; + return 0; + } + + int ok = tls13_advance_key_schedule(ssl, dhe_secret, dhe_secret_len); + OPENSSL_free(dhe_secret); + return ok; +} + static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { if (!tls13_check_message_type(ssl, SSL3_MT_CLIENT_HELLO)) { return ssl_hs_error; @@ -165,7 +222,6 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { /* The PRF hash is now known. Set up the key schedule and hash the * ClientHello. */ - static const uint8_t kZeroes[EVP_MAX_MD_SIZE] = {0}; size_t hash_len = EVP_MD_size(ssl_get_handshake_digest(ssl_get_algorithm_prf(ssl))); if (!tls13_init_key_schedule(ssl, kZeroes, hash_len)) { @@ -173,42 +229,79 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { } /* Resolve PSK and incorporate it into the secret. */ - if (cipher->algorithm_auth == SSL_aPSK) { - /* TODO(davidben): Support PSK. */ - OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); - return ssl_hs_error; - } else if (!tls13_advance_key_schedule(ssl, kZeroes, hash_len)) { + if (!resolve_psk_secret(ssl)) { return ssl_hs_error; } /* Resolve ECDHE and incorporate it into the secret. */ - if (cipher->algorithm_mkey == SSL_kECDHE) { - const uint8_t *key_share_buf = NULL; - size_t key_share_len = 0; - CBS key_share; - if (!SSL_early_callback_ctx_extension_get(&early_ctx, TLSEXT_TYPE_key_share, - &key_share_buf, &key_share_len)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_KEY_SHARE); - ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_MISSING_EXTENSION); - return ssl_hs_error; + int need_retry; + if (!resolve_ecdhe_secret(ssl, &need_retry, &early_ctx)) { + if (need_retry) { + hs->state = state_send_hello_retry_request; + return ssl_hs_ok; } + return ssl_hs_error; + } - CBS_init(&key_share, key_share_buf, key_share_len); - uint8_t *dhe_secret; - size_t dhe_secret_len; - uint8_t alert; - if (!ext_key_share_parse_clienthello(ssl, &dhe_secret, &dhe_secret_len, - &alert, &key_share)) { - ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); - return ssl_hs_error; - } + hs->state = state_send_server_hello; + return ssl_hs_ok; +} - int ok = tls13_advance_key_schedule(ssl, dhe_secret, dhe_secret_len); - OPENSSL_free(dhe_secret); - if (!ok) { - return ssl_hs_error; +static enum ssl_hs_wait_t do_send_hello_retry_request(SSL *ssl, + SSL_HANDSHAKE *hs) { + CBB cbb, body, extensions; + uint16_t group_id; + 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) || + !ssl->method->finish_message(ssl, &cbb)) { + CBB_cleanup(&cbb); + return ssl_hs_error; + } + + hs->state = state_flush_hello_retry_request; + return ssl_hs_write_message; +} + +static enum ssl_hs_wait_t do_flush_hello_retry_request(SSL *ssl, + SSL_HANDSHAKE *hs) { + hs->state = state_process_second_client_hello; + return ssl_hs_flush_and_read_message; +} + +static enum ssl_hs_wait_t do_process_second_client_hello(SSL *ssl, + SSL_HANDSHAKE *hs) { + if (!tls13_check_message_type(ssl, SSL3_MT_CLIENT_HELLO)) { + return ssl_hs_error; + } + + struct ssl_early_callback_ctx early_ctx; + + memset(&early_ctx, 0, sizeof(early_ctx)); + early_ctx.ssl = ssl; + early_ctx.client_hello = ssl->init_msg; + early_ctx.client_hello_len = ssl->init_num; + if (!ssl_early_callback_init(&early_ctx)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + return ssl_hs_error; + } + + int need_retry; + if (!resolve_ecdhe_secret(ssl, &need_retry, &early_ctx)) { + if (need_retry) { + /* Only send one HelloRetryRequest. */ + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); + OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE); } - } else if (!tls13_advance_key_schedule(ssl, kZeroes, hash_len)) { + return ssl_hs_error; + } + + if (!ssl->method->hash_current_message(ssl)) { return ssl_hs_error; } @@ -352,7 +445,6 @@ static enum ssl_hs_wait_t do_send_server_finished(SSL *ssl, SSL_HANDSHAKE *hs) { static enum ssl_hs_wait_t do_flush(SSL *ssl, SSL_HANDSHAKE *hs) { /* Update the secret to the master secret and derive traffic keys. */ - static const uint8_t kZeroes[EVP_MAX_MD_SIZE] = {0}; if (!tls13_advance_key_schedule(ssl, kZeroes, hs->hash_len) || !tls13_derive_traffic_secret_0(ssl) || !tls13_set_traffic_key(ssl, type_data, evp_aead_seal, @@ -426,6 +518,15 @@ enum ssl_hs_wait_t tls13_server_handshake(SSL *ssl) { case state_process_client_hello: ret = do_process_client_hello(ssl, hs); break; + case state_send_hello_retry_request: + ret = do_send_hello_retry_request(ssl, hs); + break; + case state_flush_hello_retry_request: + ret = do_flush_hello_retry_request(ssl, hs); + break; + case state_process_second_client_hello: + ret = do_process_second_client_hello(ssl, hs); + break; case state_send_server_hello: ret = do_send_server_hello(ssl, hs); break;