Don't dereference hs->credential on TLS 1.2 PSK ciphers

TLS 1.2 PSK ciphers don't use the credentials system and won't have a
credential set. The OCSP and SCT extension callbacks didn't handle this
case correctly.

Since we were already checking for ssl_cipher_uses_certificate_auth in
the OCSP one, which implies there's a credential, I opted to just fix
the order of the conditions, as well as align the SCT one with it.

I thought we had test coverage for this, but runner automatically
configures a certificate even when it doesn't need one, so we never
actually exercised this path. Refine the automatic behavior a bit.

Change-Id: Idf7f06688fc51a2f5d23fd83c23f6da7035e27a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66927
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Pete Bentley <prb@google.com>

ssl/extensions.cc

diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index a5e4ec8..20a5d30 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -1128,10 +1128,9 @@
 static bool ext_ocsp_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) {
   SSL *const ssl = hs->ssl;
   if (ssl_protocol_version(ssl) >= TLS1_3_VERSION ||
-      !hs->ocsp_stapling_requested ||
-      hs->credential->ocsp_response == nullptr ||  //
-      ssl->s3->session_reused ||
-      !ssl_cipher_uses_certificate_auth(hs->new_cipher)) {
+      !hs->ocsp_stapling_requested || ssl->s3->session_reused ||
+      !ssl_cipher_uses_certificate_auth(hs->new_cipher) ||
+      hs->credential->ocsp_response == nullptr) {
     return true;
   }
 
@@ -1346,8 +1345,10 @@
 
 static bool ext_sct_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) {
   SSL *const ssl = hs->ssl;
+  assert(hs->scts_requested);
   // The extension shouldn't be sent when resuming sessions.
   if (ssl_protocol_version(ssl) >= TLS1_3_VERSION || ssl->s3->session_reused ||
+      !ssl_cipher_uses_certificate_auth(hs->new_cipher) ||
       hs->credential->signed_cert_timestamp_list == nullptr) {
     return true;
   }

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index cd7fbca..8cc698b 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1453,7 +1453,7 @@
 
 	// Configure the default credential.
 	shimCertificate := test.shimCertificate
-	if shimCertificate == nil && len(test.shimCredentials) == 0 && test.testType == serverTest {
+	if shimCertificate == nil && len(test.shimCredentials) == 0 && test.testType == serverTest && len(test.config.PreSharedKey) == 0 {
 		shimCertificate = &rsaCertificate
 	}
 	if shimCertificate != nil {
@@ -3715,11 +3715,13 @@
 	}
 	prefix := protocol.String() + "-"
 
-	var cert Credential
-	if hasComponent(suite.name, "ECDSA") {
-		cert = ecdsaP256Certificate
-	} else {
-		cert = rsaCertificate
+	var cert *Credential
+	if isTLS13Suite(suite.name) {
+		cert = &rsaCertificate
+	} else if hasComponent(suite.name, "ECDSA") {
+		cert = &ecdsaP256Certificate
+	} else if hasComponent(suite.name, "RSA") {
+		cert = &rsaCertificate
 	}
 
 	var flags []string
@@ -3750,6 +3752,11 @@
 	serverCipherSuites := []uint16{suite.id}
 	if shouldFail {
 		expectedServerError = ":NO_SHARED_CIPHER:"
+		if ver.version >= VersionTLS13 && cert == nil {
+			// TLS 1.2 PSK ciphers won't configure a server certificate, but we
+			// require one in TLS 1.3.
+			expectedServerError = ":NO_CERTIFICATE_SET:"
+		}
 		expectedClientError = ":WRONG_CIPHER_RETURNED:"
 		// Configure the server to select ciphers as normal but
 		// select an incompatible cipher in ServerHello.
@@ -3768,14 +3775,14 @@
 			MinVersion:           ver.version,
 			MaxVersion:           ver.version,
 			CipherSuites:         []uint16{suite.id},
-			Credential:           &cert,
+			Credential:           cert,
 			PreSharedKey:         []byte(psk),
 			PreSharedKeyIdentity: pskIdentity,
 			Bugs: ProtocolBugs{
 				AdvertiseAllConfiguredCiphers: true,
 			},
 		},
-		shimCertificate:      &cert,
+		shimCertificate:      cert,
 		flags:                flags,
 		resumeSession:        true,
 		shouldFail:           shouldFail,
@@ -3791,7 +3798,7 @@
 			MinVersion:           ver.version,
 			MaxVersion:           ver.version,
 			CipherSuites:         serverCipherSuites,
-			Credential:           &cert,
+			Credential:           cert,
 			PreSharedKey:         []byte(psk),
 			PreSharedKeyIdentity: pskIdentity,
 			Bugs: ProtocolBugs{
@@ -3818,7 +3825,7 @@
 			MinVersion:           ver.version,
 			MaxVersion:           ver.version,
 			CipherSuites:         []uint16{suite.id},
-			Credential:           &cert,
+			Credential:           cert,
 			PreSharedKey:         []byte(psk),
 			PreSharedKeyIdentity: pskIdentity,
 		},
@@ -3845,7 +3852,7 @@
 				MinVersion:           ver.version,
 				MaxVersion:           ver.version,
 				CipherSuites:         []uint16{suite.id},
-				Credential:           &cert,
+				Credential:           cert,
 				PreSharedKey:         []byte(psk),
 				PreSharedKeyIdentity: pskIdentity,
 			},