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>

ssl/test/runner/common.go

@@ -860,9 +860,9 @@ type ProtocolBugs struct {
 	// 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.

ssl/test/runner/handshake_client.go

@@ -162,22 +162,24 @@ NextCipherSuite:
 			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)
 	}

ssl/test/runner/handshake_server.go

@@ -486,12 +486,7 @@ Curves:
 
 	// 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 @@ Curves:
 		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 @@ func (c *Conn) tryCipherSuite(id uint16, supportedCipherSuites []uint16, version
 			}
 			// 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
 		}

ssl/test/runner/prf.go

@@ -472,12 +472,6 @@ const (
 // 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 {

ssl/test/runner/runner.go

@@ -2445,12 +2445,18 @@ func addCipherSuiteTests() {
 					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 @@ func addCipherSuiteTests() {
 						PreSharedKey:         []byte(psk),
 						PreSharedKeyIdentity: pskIdentity,
 						Bugs: ProtocolBugs{
-							EnableAllCiphers:            shouldServerFail,
-							IgnorePeerCipherPreferences: shouldServerFail,
+							AdvertiseAllConfiguredCiphers: true,
 						},
 					},
 					certFile:      certFile,
@@ -2485,13 +2490,13 @@ func addCipherSuiteTests() {
 					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 @@ func addCipherSuiteTests() {
 		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,
+			},
 		},
 	})