Make EnableAllCiphers client-only and rename.
EnableAllCiphers is problematic since some (version, cipher)
combinations aren't even defined and crash. Instead, use the
SendCipherSuite bug to mask the true cipher (which is becomes arbitrary)
for failure tests. The shim should fail long before we get further.
This lets us remove a number of weird checks in the TLS 1.3 code.
This also fixes the UnknownCipher tests which weren't actually testing
anything. EnableAllCiphers is now AdvertiseAllConfiguredCiphers and
does not filter out garbage values.
Change-Id: I7102fa893146bb0d096739e768c5a7aa339e51a8
Reviewed-on: https://boringssl-review.googlesource.com/11481
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 8bb4a6b..1d16a7a 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -860,9 +860,9 @@
// be packed into records, up to the largest size record available.
PackHandshakeFlight bool
- // EnableAllCiphers, if true, causes all configured ciphers to be
- // enabled.
- EnableAllCiphers bool
+ // AdvertiseAllConfiguredCiphers, if true, causes the client to
+ // advertise all configured cipher suite values.
+ AdvertiseAllConfiguredCiphers bool
// EmptyCertificateList, if true, causes the server to send an empty
// certificate list in the Certificate message.
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 167fabf..c658e72 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -162,22 +162,24 @@
if suite.id != suiteId {
continue
}
- if !c.config.Bugs.EnableAllCiphers {
- // Don't advertise TLS 1.2-only cipher suites unless
- // we're attempting TLS 1.2.
- if maxVersion < VersionTLS12 && suite.flags&suiteTLS12 != 0 {
- continue
- }
- // Don't advertise non-DTLS cipher suites in DTLS.
- if c.isDTLS && suite.flags&suiteNoDTLS != 0 {
- continue
- }
+ // Don't advertise TLS 1.2-only cipher suites unless
+ // we're attempting TLS 1.2.
+ if maxVersion < VersionTLS12 && suite.flags&suiteTLS12 != 0 {
+ continue
+ }
+ // Don't advertise non-DTLS cipher suites in DTLS.
+ if c.isDTLS && suite.flags&suiteNoDTLS != 0 {
+ continue
}
hello.cipherSuites = append(hello.cipherSuites, suiteId)
continue NextCipherSuite
}
}
+ if c.config.Bugs.AdvertiseAllConfiguredCiphers {
+ hello.cipherSuites = possibleCipherSuites
+ }
+
if c.config.Bugs.SendRenegotiationSCSV {
hello.cipherSuites = append(hello.cipherSuites, renegotiationSCSV)
}
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index c88181e..a09dc27 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -486,12 +486,7 @@
// Resolve PSK and compute the early secret.
var psk []byte
- // The only way for hs.suite to be a PSK suite yet for there to be
- // no sessionState is if config.Bugs.EnableAllCiphers is true and
- // the test runner forced us to negotiated a PSK suite. It doesn't
- // really matter what we do here so long as we continue the
- // handshake and let the client error out.
- if hs.suite.flags&suitePSK != 0 && hs.sessionState != nil {
+ if hs.suite.flags&suitePSK != 0 {
psk = deriveResumptionPSK(hs.suite, hs.sessionState.masterSecret)
hs.finishedHash.setResumptionContext(deriveResumptionContext(hs.suite, hs.sessionState.masterSecret))
} else {
@@ -743,9 +738,7 @@
c.writeRecord(recordTypeHandshake, certVerify.marshal())
} else {
// Pick up certificates from the session instead.
- // hs.sessionState may be nil if config.Bugs.EnableAllCiphers is
- // true.
- if hs.sessionState != nil && len(hs.sessionState.certificates) > 0 {
+ if len(hs.sessionState.certificates) > 0 {
if _, err := hs.processCertsFromClient(hs.sessionState.certificates); err != nil {
return err
}
@@ -1728,25 +1721,23 @@
}
// Don't select a ciphersuite which we can't
// support for this client.
- if !c.config.Bugs.EnableAllCiphers {
- if (candidate.flags&suitePSK != 0) && !pskOk {
- continue
- }
- if (candidate.flags&suiteECDHE != 0) && !ellipticOk {
- continue
- }
- if (candidate.flags&suiteECDSA != 0) != ecdsaOk {
- continue
- }
- if version < VersionTLS12 && candidate.flags&suiteTLS12 != 0 {
- continue
- }
- if version >= VersionTLS13 && candidate.flags&suiteTLS13 == 0 {
- continue
- }
- if c.isDTLS && candidate.flags&suiteNoDTLS != 0 {
- continue
- }
+ if (candidate.flags&suitePSK != 0) && !pskOk {
+ continue
+ }
+ if (candidate.flags&suiteECDHE != 0) && !ellipticOk {
+ continue
+ }
+ if (candidate.flags&suiteECDSA != 0) != ecdsaOk {
+ continue
+ }
+ if version < VersionTLS12 && candidate.flags&suiteTLS12 != 0 {
+ continue
+ }
+ if version >= VersionTLS13 && candidate.flags&suiteTLS13 == 0 {
+ continue
+ }
+ if c.isDTLS && candidate.flags&suiteNoDTLS != 0 {
+ continue
}
return candidate
}
diff --git a/ssl/test/runner/prf.go b/ssl/test/runner/prf.go
index 33ad75a..5c7b3ab 100644
--- a/ssl/test/runner/prf.go
+++ b/ssl/test/runner/prf.go
@@ -472,12 +472,6 @@
// deriveTrafficAEAD derives traffic keys and constructs an AEAD given a traffic
// secret.
func deriveTrafficAEAD(version uint16, suite *cipherSuite, secret, phase []byte, side trafficDirection) interface{} {
- // We may have forcibly selected a non-AEAD cipher from the
- // EnableAllCiphers bug. Use the NULL cipher to avoid crashing the test.
- if suite.aead == nil {
- return nil
- }
-
label := make([]byte, 0, len(phase)+2+16)
label = append(label, phase...)
if side == clientWrite {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5630b47..17cc507 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2445,12 +2445,18 @@
shouldServerFail = true
}
+ var sendCipherSuite uint16
var expectedServerError, expectedClientError string
+ serverCipherSuites := []uint16{suite.id}
if shouldServerFail {
expectedServerError = ":NO_SHARED_CIPHER:"
}
if shouldClientFail {
expectedClientError = ":WRONG_CIPHER_RETURNED:"
+ // Configure the server to select ciphers as normal but
+ // select an incompatible cipher in ServerHello.
+ serverCipherSuites = nil
+ sendCipherSuite = suite.id
}
testCases = append(testCases, testCase{
@@ -2466,8 +2472,7 @@
PreSharedKey: []byte(psk),
PreSharedKeyIdentity: pskIdentity,
Bugs: ProtocolBugs{
- EnableAllCiphers: shouldServerFail,
- IgnorePeerCipherPreferences: shouldServerFail,
+ AdvertiseAllConfiguredCiphers: true,
},
},
certFile: certFile,
@@ -2485,13 +2490,13 @@
config: Config{
MinVersion: ver.version,
MaxVersion: ver.version,
- CipherSuites: []uint16{suite.id},
+ CipherSuites: serverCipherSuites,
Certificates: []Certificate{cert},
PreSharedKey: []byte(psk),
PreSharedKeyIdentity: pskIdentity,
Bugs: ProtocolBugs{
- EnableAllCiphers: shouldClientFail,
IgnorePeerCipherPreferences: shouldClientFail,
+ SendCipherSuite: sendCipherSuite,
},
},
flags: flags,
@@ -2631,6 +2636,20 @@
name: "UnknownCipher",
config: Config{
CipherSuites: []uint16{bogusCipher, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+ Bugs: ProtocolBugs{
+ AdvertiseAllConfiguredCiphers: true,
+ },
+ },
+ })
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "UnknownCipher-TLS13",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ CipherSuites: []uint16{bogusCipher, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+ Bugs: ProtocolBugs{
+ AdvertiseAllConfiguredCiphers: true,
+ },
},
})
