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>
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;
}
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,
},
