Don't set client_version to the ServerHello version.
The client_version needs to be preserved, both for the RSA key exchange and
(when this codepath is used for TLS) for the SChannel renego workaround. Fix
the tests to enforce this so the cipher suite version tests catch this.
Change-Id: I0c42dc3ec4830f3724026b400e5066e7a7f1ee97
Reviewed-on: https://boringssl-review.googlesource.com/2551
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/test/runner/cipher_suites.go b/ssl/test/runner/cipher_suites.go
index 5a3ac80..89e75c8 100644
--- a/ssl/test/runner/cipher_suites.go
+++ b/ssl/test/runner/cipher_suites.go
@@ -289,7 +289,7 @@
}
func rsaKA(version uint16) keyAgreement {
- return rsaKeyAgreement{}
+ return &rsaKeyAgreement{}
}
func ecdheECDSAKA(version uint16) keyAgreement {
diff --git a/ssl/test/runner/key_agreement.go b/ssl/test/runner/key_agreement.go
index d59168f..116dfd8 100644
--- a/ssl/test/runner/key_agreement.go
+++ b/ssl/test/runner/key_agreement.go
@@ -24,9 +24,14 @@
// rsaKeyAgreement implements the standard TLS key agreement where the client
// encrypts the pre-master secret to the server's public key.
-type rsaKeyAgreement struct{}
+type rsaKeyAgreement struct {
+ clientVersion uint16
+}
-func (ka rsaKeyAgreement) generateServerKeyExchange(config *Config, cert *Certificate, clientHello *clientHelloMsg, hello *serverHelloMsg) (*serverKeyExchangeMsg, error) {
+func (ka *rsaKeyAgreement) generateServerKeyExchange(config *Config, cert *Certificate, clientHello *clientHelloMsg, hello *serverHelloMsg) (*serverKeyExchangeMsg, error) {
+ // Save the client version for comparison later.
+ ka.clientVersion = versionToWire(clientHello.vers, clientHello.isDTLS)
+
if config.Bugs.RSAServerKeyExchange {
// Send an empty ServerKeyExchange message.
return &serverKeyExchangeMsg{}, nil
@@ -35,7 +40,7 @@
return nil, nil
}
-func (ka rsaKeyAgreement) processClientKeyExchange(config *Config, cert *Certificate, ckx *clientKeyExchangeMsg, version uint16) ([]byte, error) {
+func (ka *rsaKeyAgreement) processClientKeyExchange(config *Config, cert *Certificate, ckx *clientKeyExchangeMsg, version uint16) ([]byte, error) {
preMasterSecret := make([]byte, 48)
_, err := io.ReadFull(config.rand(), preMasterSecret[2:])
if err != nil {
@@ -59,20 +64,21 @@
if err != nil {
return nil, err
}
- // We don't check the version number in the premaster secret. For one,
- // by checking it, we would leak information about the validity of the
- // encrypted pre-master secret. Secondly, it provides only a small
- // benefit against a downgrade attack and some implementations send the
- // wrong version anyway. See the discussion at the end of section
- // 7.4.7.1 of RFC 4346.
+ // This check should be done in constant-time, but this is a testing
+ // implementation. See the discussion at the end of section 7.4.7.1 of
+ // RFC 4346.
+ vers := uint16(preMasterSecret[0])<<8 | uint16(preMasterSecret[1])
+ if ka.clientVersion != vers {
+ return nil, errors.New("tls: invalid version in RSA premaster")
+ }
return preMasterSecret, nil
}
-func (ka rsaKeyAgreement) processServerKeyExchange(config *Config, clientHello *clientHelloMsg, serverHello *serverHelloMsg, cert *x509.Certificate, skx *serverKeyExchangeMsg) error {
+func (ka *rsaKeyAgreement) processServerKeyExchange(config *Config, clientHello *clientHelloMsg, serverHello *serverHelloMsg, cert *x509.Certificate, skx *serverKeyExchangeMsg) error {
return errors.New("tls: unexpected ServerKeyExchange")
}
-func (ka rsaKeyAgreement) generateClientKeyExchange(config *Config, clientHello *clientHelloMsg, cert *x509.Certificate) ([]byte, *clientKeyExchangeMsg, error) {
+func (ka *rsaKeyAgreement) generateClientKeyExchange(config *Config, clientHello *clientHelloMsg, cert *x509.Certificate) ([]byte, *clientKeyExchangeMsg, error) {
preMasterSecret := make([]byte, 48)
vers := clientHello.vers
if config.Bugs.RsaClientKeyExchangeVersion != 0 {
