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>

8 files changed, 119 insertions, 50 deletions

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 @@ bool tls1_choose_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t *out) {
     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 @@ struct DC {
   // 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 @@ UniquePtr<DC> DC::Dup() {
 
   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 @@ UniquePtr<DC> DC::Parse(CRYPTO_BUFFER *in, uint8_t *out_alert) {
 
   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 @@ UniquePtr<DC> DC::Parse(CRYPTO_BUFFER *in, uint8_t *out_alert) {
   }
 
   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 @@ static bool ssl_can_serve_dc(const SSL_HANDSHAKE *hs) {
     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 @@ const (
 	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 @@ type Config struct {
 	// 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 @@ type ProtocolBugs struct {
 	// 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 @@ type clientHandshakeState struct {
 	session        *ClientSessionState
 	finishedBytes  []byte
 	peerPublicKey  crypto.PublicKey
+	peerIsDC       bool
 }
 
 func mapClientHelloVersion(vers uint16, isDTLS bool) uint16 {
@@ -525,7 +526,7 @@ func (hs *clientHandshakeState) createClientHello(innerHello *clientHelloMsg, ec
 		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 @@ func (hs *clientHandshakeState) doTLS13Handshake(msg any) error {
 
 		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 @@ func (hs *clientHandshakeState) verifyCertificates(certMsg *certificateMsg) erro
 				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 @@ func (hs *clientHandshakeState) verifyCertificates(certMsg *certificateMsg) erro
 			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 @@ type clientHelloMsg struct {
 	emptyExtensions                          bool
 	pad                                      int
 	compressedCertAlgs                       []uint16
-	delegatedCredentials                     bool
+	delegatedCredential                      []signatureAlgorithm
 	alpsProtocols                            []string
 	alpsProtocolsOld                         []string
 	outerExtensions                          []uint16
@@ -501,15 +501,15 @@ func (m *clientHelloMsg) marshalBody(hello *cryptobyte.Builder, typ clientHelloT
 			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 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool {
 	m.alpnProtocols = nil
 	m.extendedMasterSecret = false
 	m.customExtension = ""
-	m.delegatedCredentials = false
+	m.delegatedCredential = nil
 	m.alpsProtocols = nil
 	m.alpsProtocolsOld = nil
 
@@ -1029,11 +1029,10 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool {
 					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 @@ func (m *certificateMsg) unmarshal(data []byte) bool {
 					}
 				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 @@ func addSignatureAlgorithmTests() {
 				signatureRSAPKCS1WithSHA1,
 			},
 			Bugs: ProtocolBugs{
-				NoSignatureAlgorithms:       true,
-				DisableDelegatedCredentials: true,
+				NoSignatureAlgorithms: true,
 			},
 		},
 		shouldFail:    true,
@@ -16460,21 +16459,42 @@ func addDelegatedCredentialTests() {
 		config: Config{
 			MinVersion: VersionTLS13,
 			MaxVersion: VersionTLS13,
+		},
+		flags: []string{
+			"-delegated-credential", ecdsaFlagValue,
+		},
+	})
+
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "DelegatedCredentials-Basic",
+		config: Config{
+			MinVersion:                    VersionTLS13,
+			MaxVersion:                    VersionTLS13,
+			DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP256AndSHA256},
 			Bugs: ProtocolBugs{
-				DisableDelegatedCredentials: true,
+				ExpectDelegatedCredentials: true,
 			},
 		},
 		flags: []string{
 			"-delegated-credential", ecdsaFlagValue,
+			"-expect-delegated-credential-used",
 		},
 	})
 
 	testCases = append(testCases, testCase{
 		testType: serverTest,
-		name:     "DelegatedCredentials-Basic",
+		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 @@ func addDelegatedCredentialTests() {
 		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 @@ func verifyMessage(version uint16, key crypto.PublicKey, config *Config, sigAlg
 	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
 }