Skip to content

fix(crypto/rustls): pin RA-TLS CertificateVerify to ECDSA P-384/SHA-384#940

Open
MichalTarnacki wants to merge 1 commit into
intel:mainfrom
MichalTarnacki:fix/certificate_verification
Open

fix(crypto/rustls): pin RA-TLS CertificateVerify to ECDSA P-384/SHA-384#940
MichalTarnacki wants to merge 1 commit into
intel:mainfrom
MichalTarnacki:fix/certificate_verification

Conversation

@MichalTarnacki

Copy link
Copy Markdown
Contributor
# Assessed Severity Bug Call Chain / Location Fix Classification Disposition Reason
1 Low TLS CertificateVerify sig-scheme not pinned -- supported_verify_schemes() returns full default list rustls::ClientConnection::process_tls_records -> ::verify_tls13_signature Override provider.signature_verification_algorithms to only ECDSA_P384_SHA384 and return [ECDSA_NISTP384_SHA384] from supported_verify_schemes(). weakness The CertificateVerify signature scheme is not pinned, but the scheme is inseparable from the certificate's key type, and that key is already forced to P-384 by the app-layer RA-TLS callback, which rejects any non-P-384 SPKI before the transcript signature matters. rustls verifies CertificateVerify against the end-entity key, so a peer declaring RSA or P-256 simply fails the key/algorithm mismatch, and there is no weaker P-384 variant to downgrade to (ECDSA_NISTP384_SHA384 is the only one). Most decisively, producing any valid CertificateVerify under any scheme requires the peer's private key: without it no forgery is possible, and with it the attacker is a legitimate peer, so pinning the scheme adds nothing to authentication. No reachable path lets the unpinned scheme grant a new capability, so the residual is purely formal algorithm-agility hygiene.

The RA-TLS Verifier accepted the full default list of signature schemes
in supported_verify_schemes() and did not check the scheme used in the
CertificateVerify message, leaving the handshake transcript signature
algorithm unpinned.

Restrict both the client- and server-side Verifier to
ECDSA_NISTP384_SHA384: return only that scheme from
supported_verify_schemes(), and reject any other scheme in
verify_tls13_signature() with PeerIncompatible before verifying the
transcript. This keeps the signature algorithm consistent with the
cipher suite and key-exchange group already pinned in crypto_provider().

Signed-off-by: Michal Tarnacki <michal.tarnacki@intel.com>
Co-authored-by: GitHub Copilot <noreply@github.com>
@sgrams

sgrams commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Where's this requirement coming from? MigTD shall support more algorithms.

@sgrams

sgrams commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

According to docs, P384 is only recommended, but should not be enforced

@sgrams sgrams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide source of this requirement

@MichalTarnacki

Copy link
Copy Markdown
Contributor Author

You are right, from the spec:

TDX Migration TD Design Guide Rev 0.9.3 (2025-08-21)

Table 7-1: MigTD cryptography algorithm

cryptography options recommendation
digital signature rsa, ecdsa ecdsa-nist_p384
key exchange ecdhe ecdhe-secp384r1
aead aes-gcm, chacha20-poly1305 aes-256-gcm

For "digital signature", the permitted options are RSA and ECDSA; P-384
is only the recommendation

However code already hard-force ECDSA P-384 / SHA-384 even though the spec
allows more options.

verify_signature() in src/migtd/src/ratls/server_client.rs:

fn verify_signature(cert: &Certificate, verified_report: &[u8]) -> CryptoResult<()> {
    let public_key = cert
        .tbs_certificate
        .subject_public_key_info
        .subject_public_key
        .as_bytes()
        .ok_or_else(|| CryptoError::ParseCertificate)?;
    let tbs = cert.tbs_certificate.to_der()?;
    let signature = cert.signature_value.as_bytes().ok_or_else(|| CryptoError::ParseCertificate)?;
    verify_public_key(verified_report, public_key)?;
    ecdsa_verify(public_key, &tbs, signature) // <-- P-384 only
}

ecdsa_verify is hard-wired to ECDSA_P384_SHA384_ASN1 in
src/crypto/src/rustls_impl/ecdsa.rs:

pub fn ecdsa_verify(public_key: &[u8], data: &[u8], signature: &[u8]) -> Result<()> {
    let pk = UnparsedPublicKey::new(&signature::ECDSA_P384_SHA384_ASN1, public_key);
    pk.verify(data, signature).map_err(|_e| Error::EcdsaVerify)
}

verify_signature_with_algorithm() in src/crypto/src/lib.rs:

// ECDSA with SHA-384: 1.2.840.10045.4.3.3
const ECDSA_WITH_SHA384: &[u32] = &[1, 2, 840, 10045, 4, 3, 3];

// Only ECDSA-P384 with SHA384 signature is supported
match oid_arcs.as_slice() {
    ECDSA_WITH_SHA384 => ecdsa::ecdsa_verify_with_algorithm(
        public_key,
        message,
        signature,
        &ecdsa::ECDSA_P384_SHA384_ASN1,
    )
    .map_err(|_| Error::SignatureVerification),
    _ => {
        // Unsupported algorithm
        Err(Error::UnsupportedAlgorithm)
    }
}

EcdsaPk::new() in src/crypto/src/rustls_impl/ecdsa.rs:

pub fn new() -> Result<Self> {
    let rand = SystemRandom::new();
    EcdsaKeyPair::generate_pkcs8(&signature::ECDSA_P384_SHA384_ASN1_SIGNING, &rand)
        .map(|pk| Self { pk })
        .map_err(|_| Error::GenerateKeyPair)
}

Requester in src/migtd/src/spdm/spdm_req.rs and
responder in src/migtd/src/spdm/spdm_rsp.rs:

let config_info = common::SpdmConfigInfo {
    // ...
    base_asym_algo: SpdmBaseAsymAlgo::TPM_ALG_ECDSA_ECC_NIST_P384,
    base_hash_algo: SpdmBaseHashAlgo::TPM_ALG_SHA_384,
    dhe_algo: SpdmDheAlgo::SECP_384_R1,
    aead_algo: SpdmAeadAlgo::AES_256_GCM,
    req_asym_algo: SpdmReqAsymAlgo::TPM_ALG_ECDSA_ECC_NIST_P384,
    // ...
};

So this change is only hardening for the existing code, adding fail fast strategy.

@MichalTarnacki

Copy link
Copy Markdown
Contributor Author

Maybe we should close this PR and open issue about adding support for other algorithms?

@MichalTarnacki
MichalTarnacki requested a review from sgrams July 9, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants