Skip to content

fix(node_tls): reject unsupported certificate versions - #36248

Open
nathanwhit wants to merge 2 commits into
denoland:mainfrom
nathanwhit:fix/node-tls-unsupported-cert-versions
Open

fix(node_tls): reject unsupported certificate versions#36248
nathanwhit wants to merge 2 commits into
denoland:mainfrom
nathanwhit:fix/node-tls-unsupported-cert-versions

Conversation

@nathanwhit

@nathanwhit nathanwhit commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

Verify X.509v1 certificate chains cryptographically instead of trusting
issuer/subject name matching.

  • walk the chain ourselves (webpki rejects v1 certs at parse time) and verify
    every hop's signature with the issuer's public key, using the crypto
    provider's signature algorithms
  • verify the handshake signature against the SubjectPublicKeyInfo read out of
    the certificate instead of asserting validity unconditionally
  • keep issuer/subject matching, but only to pick a Node/OpenSSL error code —
    never to accept a chain
  • add coverage for accept, reject, and rejectUnauthorized: false

Why

Two fallback paths treated an unparseable certificate as a successful
verification:

  • verify_server_cert accepted a v1 chain whenever verify_chain_structure
    found an issuer/subject path to a trusted root. Names are not signatures, so
    any v1 cert that merely named a trusted CA as its issuer was accepted.
  • filter_unsupported_cert_version turned UnsupportedCertVersion /
    BadEncoding from the TLS 1.2/1.3 signature callbacks into
    HandshakeSignatureValid::assertion(), so the peer never had to prove it held
    the private key for the certificate it presented.

Together those let anyone MITM a connection by presenting a self-issued v1
certificate naming a CA in the client's trust store.

OpenSSL (and therefore Node) does accept v1 certificates, so simply rejecting
them is not an option — several upstream fixtures are v1 and ~36 node_compat
tests depend on them working. Doing the signature checks ourselves keeps that
compatibility while making acceptance mean what it says.

Validity periods are still not checked for v1 chains (webpki can't read them out
of a v1 cert either), so an expired v1 chain verifies where OpenSSL would report
CERT_HAS_EXPIRED. That is pre-existing and called out in a comment.

Tests

New fixtures in tests/testdata/tls (documented in its README): localhost_v1
is a genuine v1 leaf signed by RootCA; localhost_v1_forged names RootCA as
its issuer but was signed by an impostor key, so it only fails if signatures are
actually verified.

  • cargo test unit_node::tls_test — v1 cert with the right CA connects and is
    authorized; the forged one fails with UNABLE_TO_VERIFY_LEAF_SIGNATURE;
    rejectUnauthorized: false connects with authorizationError set
  • cargo test -p node_compat_tests — the 36 TLS/HTTP2/HTTPS tests that the
    previous revision of this PR broke all pass again

@bartlomieju

Copy link
Copy Markdown
Member

One concern about lenient mode (rejectUnauthorized: false).

Removing filter_unsupported_cert_version from verify_tls12_signature/verify_tls13_signature looks like it regresses v1 certs when rejectUnauthorized is false:

  • verify_server_cert hits is_unsupported_cert_version_or_bad_encoding -> record_or_fail, which in lenient mode returns Ok(assertion()), so the chain check "passes".
  • rustls then always calls verify_tls12_signature/verify_tls13_signature to verify the key-exchange signature. The inner WebPkiServerVerifier re-parses the end-entity cert and returns Err(UnsupportedCertVersion) (or BadEncoding) - exactly the error the deleted filter existed to absorb.
  • With the filter gone, that error now propagates and aborts the handshake.

Net effect: a rejectUnauthorized: false connection to a v1-cert server that previously connected (proceeding with authorizationError set, matching Node/OpenSSL) now fails to connect entirely.

The strict-mode tightening in verify_server_cert is the right call, but in lenient mode the connection should proceed with the error recorded rather than hard-fail at signature verification. Two options:

  • keep a filter on the signature callbacks that returns HandshakeSignatureValid::assertion() only when !strict_verify, or
  • if aborting in lenient mode is intended, it'd be worth saying so explicitly and adding a rejectUnauthorized: false test to lock in the behavior.

The new test only exercises strict mode, so this lenient-mode path is currently uncovered either way.

Building the chain from issuer/subject names alone accepted any v1
certificate that merely named a trusted CA as its issuer, and the
handshake-signature callbacks then asserted validity unconditionally, so
the peer never had to prove it held the private key.

Instead, walk the chain ourselves (webpki rejects v1 certs at parse time)
and verify every hop's signature with the issuer's public key, using the
crypto provider's signature algorithms. For the handshake signature, read
the SubjectPublicKeyInfo out of the certificate and verify against that
rather than asserting. Name matching is now only used to pick a
Node/OpenSSL error code, never to accept a chain.

This keeps OpenSSL's acceptance of v1 certs with a genuinely signed chain
- including in `rejectUnauthorized: false` mode, where the connection
still comes up with the error reported via `authorizationError`.
@nathanwhit

Copy link
Copy Markdown
Member Author

Good catch — that was a real regression, and CI agreed: 36 node_compat tests were
failing, most of them exactly the lenient-mode path you traced.

I went with a third option rather than either of the two you listed, because
re-adding the filter under !strict_verify would put back the part of the old
code that worried me most. HandshakeSignatureValid::assertion() there means the
peer never proves it holds the private key for the cert it just presented, so in
lenient mode a copied v1 cert would still complete the handshake — and lenient
mode is exactly where getPeerCertificate() / authorizationError are supposed
to let callers make their own trust decision on a certificate that really is the
server's.

So instead of asserting, the callbacks now read the SubjectPublicKeyInfo
straight out of the certificate and verify the handshake signature against it
with the crypto provider's algorithms (what verify_tls13_signature_with_raw_key
does upstream, plus the TLS 1.2 multi-candidate loop). webpki can't parse a v1
cert, but it doesn't need to in order to check a signature against a raw key.

Net effect in lenient mode: the connection comes up, authorizationError carries
the chain error, and the handshake signature is still genuinely verified. Covered
by a new rejectUnauthorized: false test.

While chasing the rest of the CI failures I also had to walk back the strict-mode
tightening — not the intent, but the mechanism. Rejecting v1 chains outright
breaks ~20 tests that legitimately expect a v1 cert with a matching CA to be
authorized (test-tls-set-default-ca-certificates-append-https-request,
test-tls-client-verify with ca2, …), because OpenSSL accepts v1 certs. The
fix is to actually verify those chains rather than refuse them: verify_chain
now requires each hop's signature to verify under the issuer's public key, and
issuer/subject matching survives only as the error-code classifier.

That turned out to close a worse hole than the one I originally set out to fix.
On main, structural matching plus the asserting signature callback means a
self-issued v1 cert whose issuer name copies a trusted CA's subject is accepted
outright — a working MITM against any client with that CA in its store. There's
now a localhost_v1_forged fixture for precisely that (same issuer DN as
RootCA, signed by an impostor key) which main accepts and this branch
rejects.

One limitation I left alone and noted in a comment: validity periods still aren't
checked for v1 chains, so an expired v1 chain verifies where OpenSSL would say
CERT_HAS_EXPIRED. Pre-existing, and it needs a DER time parser, so it felt like
a separate change.

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