Fix delegated credential signature algorithm handling
https://boringssl-review.googlesource.com/c/34884 tried to update to the
newer DC draft, but didn't quite do so. In that update, DCs
overcomplicated the signature algorithm negotiation so that there are
two different signature algorithm lists, used in different contexts.
The existing signature_algorithms extension is used to verify the
signature *on* the DC, made by the end-entity certificate. On the server
side, we should be using that to decide whether to use the DC, and we
weren't.
The new delegated_credentials extension contains another sigalg list.
That is used to verify the signature *by* the DC, in the
CertificateVerify message. (This means DC changes the operative sigalg
list for the CertificateVerify message, which is quite a mess.) On the
server side, the above CL mixed things up. When deciding whether to use
DCs, it checked the correct list. When actually using DCs, it checked
the wrong one. As a result, any time the DC list wasn't a subset of the
main list, the connection would just break!
Fix both of these, in preparation for redoing DCs over the upcoming
SSL_CREDENTIAL mechanism.
Thankfully we don't support one direction of DC usage (authenticating in
C++ and verifying in Go), so there are fewer places to worry about
mixing this up. Given this overcomplication, I'm now much, much less
inclined to ever support DCs as a client, without an rfc9345bis to redo
this.
Bug: 249
Change-Id: Id5257e89a6c8daf1635757be473c45029492d420
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66550
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 029533f..e9c38a6 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -4119,15 +4119,16 @@
return true;
}
- Span<const uint16_t> sigalgs = kSignSignatureAlgorithms;
+ Span<const uint16_t> sigalgs, peer_sigalgs;
if (ssl_signing_with_dc(hs)) {
sigalgs = MakeConstSpan(&dc->dc_cert_verify_algorithm, 1);
- } else if (!cert->sigalgs.empty()) {
- sigalgs = cert->sigalgs;
+ peer_sigalgs = hs->peer_delegated_credential_sigalgs;
+ } else {
+ sigalgs = cert->sigalgs.empty() ? MakeConstSpan(kSignSignatureAlgorithms)
+ : cert->sigalgs;
+ peer_sigalgs = tls1_get_peer_verify_algorithms(hs);
}
- Span<const uint16_t> peer_sigalgs = tls1_get_peer_verify_algorithms(hs);
-
for (uint16_t sigalg : sigalgs) {
if (!ssl_private_key_supports_signature_algorithm(hs, sigalg)) {
continue;
diff --git a/ssl/internal.h b/ssl/internal.h
index 2dd7859..d64f6c4 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1630,9 +1630,14 @@
// raw is the delegated credential encoded as specified in RFC 9345.
UniquePtr<CRYPTO_BUFFER> raw;
- // dc_cert_verify_algorithm is the signature scheme of the DC public key.
+ // dc_cert_verify_algorithm is the signature scheme of the DC public key. This
+ // is used for the CertificateVerify message.
uint16_t dc_cert_verify_algorithm = 0;
+ // algorithm is the signature scheme of the signature over the delegated
+ // credential itself, made by the end-entity certificate's public key.
+ uint16_t algorithm = 0;
+
// pkey is the public key parsed from |public_key|.
UniquePtr<EVP_PKEY> pkey;
diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc
index 17363f3..71baeb7 100644
--- a/ssl/ssl_cert.cc
+++ b/ssl/ssl_cert.cc
@@ -118,6 +118,7 @@
#include <limits.h>
#include <string.h>
+#include <algorithm>
#include <utility>
#include <openssl/bn.h>
@@ -689,6 +690,7 @@
ret->raw = UpRef(raw);
ret->dc_cert_verify_algorithm = dc_cert_verify_algorithm;
+ ret->algorithm = algorithm;
ret->pkey = UpRef(pkey);
return ret;
}
@@ -705,12 +707,11 @@
CBS pubkey, deleg, sig;
uint32_t valid_time;
- uint16_t algorithm;
CRYPTO_BUFFER_init_CBS(dc->raw.get(), &deleg);
if (!CBS_get_u32(&deleg, &valid_time) ||
!CBS_get_u16(&deleg, &dc->dc_cert_verify_algorithm) ||
!CBS_get_u24_length_prefixed(&deleg, &pubkey) ||
- !CBS_get_u16(&deleg, &algorithm) ||
+ !CBS_get_u16(&deleg, &dc->algorithm) ||
!CBS_get_u16_length_prefixed(&deleg, &sig) ||
CBS_len(&deleg) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
@@ -719,7 +720,7 @@
}
dc->pkey.reset(EVP_parse_public_key(&pubkey));
- if (dc->pkey == nullptr) {
+ if (dc->pkey == nullptr || CBS_len(&pubkey) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
*out_alert = SSL_AD_DECODE_ERROR;
return nullptr;
@@ -747,14 +748,21 @@
return false;
}
- // Check that the DC signature algorithm is supported by the peer.
- Span<const uint16_t> peer_sigalgs = hs->peer_delegated_credential_sigalgs;
- for (uint16_t peer_sigalg : peer_sigalgs) {
- if (dc->dc_cert_verify_algorithm == peer_sigalg) {
- return true;
- }
+ // Check that the peer supports the signature over the delegated credential.
+ if (std::find(hs->peer_sigalgs.begin(), hs->peer_sigalgs.end(),
+ dc->algorithm) == hs->peer_sigalgs.end()) {
+ return false;
}
- return false;
+
+ // Check that the peer supports the CertificateVerify signature algorithm.
+ if (std::find(hs->peer_delegated_credential_sigalgs.begin(),
+ hs->peer_delegated_credential_sigalgs.end(),
+ dc->dc_cert_verify_algorithm) ==
+ hs->peer_delegated_credential_sigalgs.end()) {
+ return false;
+ }
+
+ return true;
}
bool ssl_signing_with_dc(const SSL_HANDSHAKE *hs) {
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 666b4b6..69d8a69 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -112,7 +112,7 @@
extensionPadding uint16 = 21
extensionExtendedMasterSecret uint16 = 23
extensionCompressedCertAlgs uint16 = 27
- extensionDelegatedCredentials uint16 = 34
+ extensionDelegatedCredential uint16 = 34
extensionSessionTicket uint16 = 35
extensionPreSharedKey uint16 = 41
extensionEarlyData uint16 = 42
@@ -591,6 +591,11 @@
// supported signature algorithms that are accepted.
VerifySignatureAlgorithms []signatureAlgorithm
+ // DelegatedCredentialAlgorithms, if not empty, is the set of signature
+ // algorithms allowed for the delegated credential key. If empty, delegated
+ // credentials are disabled.
+ DelegatedCredentialAlgorithms []signatureAlgorithm
+
// QUICTransportParams, if not empty, will be sent in the QUIC
// transport parameters extension.
QUICTransportParams []byte
@@ -1952,10 +1957,6 @@
// server returns delegated credentials.
FailIfDelegatedCredentials bool
- // DisableDelegatedCredentials, if true, disables client support for delegated
- // credentials.
- DisableDelegatedCredentials bool
-
// CompatModeWithQUIC, if true, enables TLS 1.3 compatibility mode
// when running over QUIC.
CompatModeWithQUIC bool
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 4619456..94e1db3 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -39,6 +39,7 @@
session *ClientSessionState
finishedBytes []byte
peerPublicKey crypto.PublicKey
+ peerIsDC bool
}
func mapClientHelloVersion(vers uint16, isDTLS bool) uint16 {
@@ -525,7 +526,7 @@
customExtension: c.config.Bugs.CustomExtension,
omitExtensions: c.config.Bugs.OmitExtensions,
emptyExtensions: c.config.Bugs.EmptyExtensions,
- delegatedCredentials: !c.config.Bugs.DisableDelegatedCredentials,
+ delegatedCredential: c.config.DelegatedCredentialAlgorithms,
}
// Translate the bugs that modify ClientHello extension order into a
@@ -1299,7 +1300,11 @@
c.peerSignatureAlgorithm = certVerifyMsg.signatureAlgorithm
input := hs.finishedHash.certificateVerifyInput(serverCertificateVerifyContextTLS13)
- err = verifyMessage(c.vers, hs.peerPublicKey, c.config, certVerifyMsg.signatureAlgorithm, input, certVerifyMsg.signature)
+ if hs.peerIsDC {
+ err = verifyMessageDC(c.vers, hs.peerPublicKey, c.config, certVerifyMsg.signatureAlgorithm, input, certVerifyMsg.signature)
+ } else {
+ err = verifyMessage(c.vers, hs.peerPublicKey, c.config, certVerifyMsg.signatureAlgorithm, input, certVerifyMsg.signature)
+ }
if err != nil {
return err
}
@@ -1836,7 +1841,7 @@
c.sendAlert(alertIllegalParameter)
return errors.New("tls: non-leaf certificate has a delegated credential")
}
- if c.config.Bugs.DisableDelegatedCredentials {
+ if len(c.config.DelegatedCredentialAlgorithms) == 0 {
c.sendAlert(alertIllegalParameter)
return errors.New("tls: server sent delegated credential without it being requested")
}
@@ -1886,20 +1891,17 @@
return errors.New("tls: failed to parse public key from delegated credential: " + err.Error())
}
- verifier, err := getSigner(c.vers, hs.peerPublicKey, c.config, dc.algorithm, true)
- if err != nil {
- c.sendAlert(alertBadCertificate)
- return errors.New("tls: failed to get verifier for delegated credential: " + err.Error())
- }
-
- if err := verifier.verifyMessage(leafPublicKey, delegatedCredentialSignedMessage(dc.signedBytes, dc.algorithm, certs[0].Raw), dc.signature); err != nil {
+ signedMsg := delegatedCredentialSignedMessage(dc.signedBytes, dc.algorithm, certs[0].Raw)
+ if err := verifyMessage(c.vers, leafPublicKey, c.config, dc.algorithm, signedMsg, dc.signature); err != nil {
c.sendAlert(alertBadCertificate)
return errors.New("tls: failed to verify delegated credential: " + err.Error())
}
- } else if c.config.Bugs.ExpectDelegatedCredentials {
- c.sendAlert(alertInternalError)
- return errors.New("tls: delegated credentials missing")
+ hs.peerIsDC = true
} else {
+ if c.config.Bugs.ExpectDelegatedCredentials {
+ c.sendAlert(alertInternalError)
+ return errors.New("tls: delegated credentials missing")
+ }
hs.peerPublicKey = leafPublicKey
}
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 26e1257..c3d9dd3 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -194,7 +194,7 @@
emptyExtensions bool
pad int
compressedCertAlgs []uint16
- delegatedCredentials bool
+ delegatedCredential []signatureAlgorithm
alpsProtocols []string
alpsProtocolsOld []string
outerExtensions []uint16
@@ -501,15 +501,15 @@
body: body.BytesOrPanic(),
})
}
- if m.delegatedCredentials {
+ if len(m.delegatedCredential) > 0 {
body := cryptobyte.NewBuilder(nil)
body.AddUint16LengthPrefixed(func(signatureSchemeList *cryptobyte.Builder) {
- for _, sigAlg := range m.signatureAlgorithms {
+ for _, sigAlg := range m.delegatedCredential {
signatureSchemeList.AddUint16(uint16(sigAlg))
}
})
extensions = append(extensions, extension{
- id: extensionDelegatedCredentials,
+ id: extensionDelegatedCredential,
body: body.BytesOrPanic(),
})
}
@@ -756,7 +756,7 @@
m.alpnProtocols = nil
m.extendedMasterSecret = false
m.customExtension = ""
- m.delegatedCredentials = false
+ m.delegatedCredential = nil
m.alpsProtocols = nil
m.alpsProtocolsOld = nil
@@ -1029,11 +1029,10 @@
return false
}
}
- case extensionDelegatedCredentials:
- if len(body) != 0 {
+ case extensionDelegatedCredential:
+ if !parseSignatureAlgorithms(&body, &m.delegatedCredential, false) || len(body) != 0 {
return false
}
- m.delegatedCredentials = true
case extensionApplicationSettings:
var protocols cryptobyte.String
if !body.ReadUint16LengthPrefixed(&protocols) || len(body) != 0 {
@@ -2029,7 +2028,7 @@
}
case extensionSignedCertificateTimestamp:
cert.sctList = []byte(body)
- case extensionDelegatedCredentials:
+ case extensionDelegatedCredential:
// https://www.rfc-editor.org/rfc/rfc9345.html#section-4
if cert.delegatedCredential != nil {
return false
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5e34dbd..4b704db 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -10347,8 +10347,7 @@
signatureRSAPKCS1WithSHA1,
},
Bugs: ProtocolBugs{
- NoSignatureAlgorithms: true,
- DisableDelegatedCredentials: true,
+ NoSignatureAlgorithms: true,
},
},
shouldFail: true,
@@ -16460,9 +16459,6 @@
config: Config{
MinVersion: VersionTLS13,
MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- DisableDelegatedCredentials: true,
- },
},
flags: []string{
"-delegated-credential", ecdsaFlagValue,
@@ -16473,8 +16469,32 @@
testType: serverTest,
name: "DelegatedCredentials-Basic",
config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP256AndSHA256},
+ Bugs: ProtocolBugs{
+ ExpectDelegatedCredentials: true,
+ },
+ },
+ flags: []string{
+ "-delegated-credential", ecdsaFlagValue,
+ "-expect-delegated-credential-used",
+ },
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "DelegatedCredentials-ExactAlgorithmMatch",
+ config: Config{
MinVersion: VersionTLS13,
MaxVersion: VersionTLS13,
+ // Test that the server doesn't mix up the two signature algorithm
+ // fields. These options are a match because the signature_algorithms
+ // extension matches against the signature on the delegated
+ // credential, while the delegated_credential extension matches
+ // against the signature made by the delegated credential.
+ VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPSSWithSHA256},
+ DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP256AndSHA256},
Bugs: ProtocolBugs{
ExpectDelegatedCredentials: true,
},
@@ -16491,13 +16511,33 @@
config: Config{
MinVersion: VersionTLS13,
MaxVersion: VersionTLS13,
+ // If the client doesn't support the signature in the delegated credential,
+ // the server should not use delegated credentials.
+ VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPSSWithSHA384},
+ DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP256AndSHA256},
Bugs: ProtocolBugs{
FailIfDelegatedCredentials: true,
},
- // If the client doesn't support the delegated credential signature
- // algorithm then the handshake should complete without using delegated
+ },
+ flags: []string{
+ "-delegated-credential", ecdsaFlagValue,
+ },
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "DelegatedCredentials-CertVerifySigAlgoMissing",
+ config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ // If the client doesn't support the delegated credential's
+ // CertificateVerify algorithm, the server should not use delegated
// credentials.
- VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPSSWithSHA256},
+ VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPSSWithSHA256},
+ DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP384AndSHA384},
+ Bugs: ProtocolBugs{
+ FailIfDelegatedCredentials: true,
+ },
},
flags: []string{
"-delegated-credential", ecdsaFlagValue,
diff --git a/ssl/test/runner/sign.go b/ssl/test/runner/sign.go
index e0c6a92..8b38139 100644
--- a/ssl/test/runner/sign.go
+++ b/ssl/test/runner/sign.go
@@ -81,6 +81,19 @@
return signer.verifyMessage(key, msg, sig)
}
+func verifyMessageDC(version uint16, key crypto.PublicKey, config *Config, sigAlg signatureAlgorithm, msg, sig []byte) error {
+ if version >= VersionTLS12 && !slices.Contains(config.DelegatedCredentialAlgorithms, sigAlg) {
+ return errors.New("tls: unsupported signature algorithm")
+ }
+
+ signer, err := getSigner(version, key, config, sigAlg, true)
+ if err != nil {
+ return err
+ }
+
+ return signer.verifyMessage(key, msg, sig)
+}
+
type rsaPKCS1Signer struct {
hash crypto.Hash
}
