Skip to content

tls: reject chains containing blocked public keys - #66871

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
UlisesGascon:fix/tls-spki-blocklist
Oct 20, 2026
Merged

nodejs-github-bot merged 2 commits into
nodejs:mainfrom
UlisesGascon:fix/tls-spki-blocklist

Conversation

@UlisesGascon

@UlisesGascon UlisesGascon commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Node.js embeds the Mozilla root store but has no equivalent of the
browsers' CRLSet/OneCRL and no default online revocation checking. When
an intermediate CA key is compromised, a chain through it still verifies
as long as it reaches a trusted root. Removing the affected roots would
over-block, since other legitimate intermediates chain to them.

This adds a small compiled-in blocklist of SubjectPublicKeyInfo SHA-256
hashes, checked from a single SSL_CTX_set_cert_verify_callback hook on
every SSL_CTX Node.js creates (SecureContext, QUIC, DTLS). A hit is
reported as X509_V_ERR_CERT_REVOKED through the normal verification
path, so it honours rejectUnauthorized and surfaces as
authorizationError on client and server sockets. The OpenSSL 4.1 CN
name-constraint handling from #65957 moves into the same hook, since
OpenSSL allows only one.

The initial entries are the eight eMudhra/emSign intermediates whose
private keys were reported compromised in October 2026. The hashes are
byte-identical to Go's fix (CVE-2026-111943) and Electron's patch.

Known limitation

Sessions resumed from before the upgrade are not re-verified. This is
standard TLS resumption behaviour and ends with the session timeout.

Points for reviewers

  • This is the first compiled-in trust-policy list in core. Scope is
    deliberately narrow; whether it should be configurable or updatable
    outside a release is open.
  • The test-only hooks live on internalBinding('crypto') behind
    --expose-internals. A test/addons C++ addon is the alternative.
  • Needs backporting to every active release line.

AI disclosure

Assisted by Claude Code and Codex; reviewed, built and tested by me.

Refs: electron/electron#55126
Refs: golang/go#82345 (technical details: golang/go@go1.27.2...go1.27.3)

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation. labels Oct 20, 2026
@UlisesGascon
UlisesGascon force-pushed the fix/tls-spki-blocklist branch from 577a4a9 to db7cfbb Compare October 20, 2026 09:30
@UlisesGascon
UlisesGascon marked this pull request as ready for review October 20, 2026 09:31
Node.js embeds the Mozilla root store but has no equivalent of the
browsers' CRLSet/OneCRL and no default online revocation checking, so a
certificate chain through a compromised intermediate still verifies as
long as it reaches a trusted root. Removing the affected roots would
over-block, since other legitimate intermediates chain to them.

Add a small compiled-in blocklist of SubjectPublicKeyInfo SHA-256 hashes
and check every verified chain against it from a single
SSL_CTX_set_cert_verify_callback hook. A hit is reported as
X509_V_ERR_CERT_REVOKED through the normal verification path, so it
honours rejectUnauthorized and surfaces as authorizationError on both
client and server sockets. The hook is installed for every SSL_CTX
Node.js creates: SecureContext, QUIC and DTLS. The OpenSSL 4.1 CN name
constraint handling previously registered from crypto_tls.cc moves into
the same hook, since OpenSSL allows only one.

The initial entries are the eight eMudhra/emSign intermediates whose
private keys were reported compromised in October 2026. The same list is
shipped by Go (CVE-2026-111943) and Electron.

Sessions resumed from before this change are not re-verified, as is
standard for TLS resumption; the exposure ends with the session timeout.

Test-only hooks on the crypto binding, reachable with
--expose-internals, allow the test to block a fixture key and exercise
the full handshake.

Refs: electron/electron#55126
Refs: golang/go#82345
Assisted-by: Claude, Codex
Signed-off-by: Ulises Gascon <ulisesgascongonzalez@gmail.com>
@UlisesGascon
UlisesGascon force-pushed the fix/tls-spki-blocklist branch from db7cfbb to 71af0a9 Compare October 20, 2026 09:36
@UlisesGascon

UlisesGascon commented Oct 20, 2026 •

Copy link
Copy Markdown
Member Author

@aduh95 I am waiting for CI to finish in case I need to include more changes, but I have a local fix for the build issue (ref) 👍

Edit: pushed (8417eb1)

Use only X509_STORE_CTX APIs that BoringSSL also provides. BoringSSL
lacks X509_STORE_CTX_set_error_depth, X509_STORE_CTX_set_current_cert
and X509_STORE_CTX_get_verify_cb, which broke the aarch64 shared
BoringSSL build. Fetch the per-certificate callback from the SSL
object instead and skip the two setters on BoringSSL, where Node.js'
own callbacks never read them.

Assisted-by: Claude
Signed-off-by: Ulises Gascon <ulisesgascongonzalez@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina
mcollina requested review from panva and pimterry October 20, 2026 12:26

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 20, 2026
@panva panva added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 20, 2026
@aduh95 aduh95 removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 20, 2026
@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.10638% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.50%. Comparing base (531d28f) to head (8417eb1).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_context.cc 85.10% 3 Missing and 11 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #66871    +/-   ##
========================================
  Coverage   90.49%   90.50%            
========================================
  Files         797      797            
  Lines      278221   278315    +94     
  Branches    53462    53484    +22     
========================================
+ Hits       251786   251888   +102     
+ Misses      16809    16808     -1     
+ Partials     9626     9619     -7     
Files with missing lines Coverage Δ
src/crypto/crypto_context.h 100.00% <ø> (ø)
src/crypto/crypto_tls.cc 78.44% <ø> (ø)
src/crypto/crypto_context.cc 72.52% <85.10%> (+1.03%) ⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aduh95 aduh95 added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Oct 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @aduh95. Please 👍 to approve.

}

bool IsBlockedSPKIForTesting(const SpkiHash& digest) {
Mutex::ScopedLock lock(BlockedSPKIsForTestingMutex());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This puts a mutex into every TLS handshake, to check a list which is guaranteed empty anywhere except our test suite. It's not a huge cost compared to a whole TLS handshake, but it's not free, especially if there's any actual contention (worker threads doing TLS).

Doesn't need to block this, but there might be nicer ways to test this without this list and mutex and extra internal APIs. We could just generate a fresh intermediate for our test suite for example and include its SPKI in the real revoked list. That'd be cheaper, no locking, and we could drop the internal test APIs etc that this adds. Having a revoked example we can test with is probably quite useful generally.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will work on a separate PR for this 👍

@gurgunday gurgunday left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm

trigger-dev-repo-ops Bot pushed a commit to triggerdotdev/trigger.dev that referenced this pull request Oct 20, 2026
…ermediates

The webapp Docker image no longer trusts the roots behind the eight
eMudhra intermediate CAs whose private keys were compromised
([background](https://emudhra.we-have-a-problem.com/)): emSign ECC Root
CA - G3, emSign Root CA - C1, emSign ECC Root CA - C3, and Sectigo's AAA
Certificate Services, which cross-signs the same intermediates.

The system CA bundle drops them for OpenSSL, goose and Prisma's engines.
A small preload removes them from Node's bundled roots, since no Node
release has done that yet
([nodejs/node#66871](nodejs/node#66871)). The
image loads it through `NODE_OPTIONS`, and the entrypoint keeps it when
`NODE_OPTIONS` is overridden at deploy time. Certificates added with
`NODE_EXTRA_CA_CERTS` are kept.

Mono-RevId: 28fb41176a5984be06fe3755f5fa4447e05d399f
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 20, 2026
@nodejs-github-bot
nodejs-github-bot merged commit d9a3f3d into nodejs:main Oct 20, 2026
75 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in d9a3f3d

ylli-selmani pushed a commit to ylli-selmani/Marinara-Engine-Ylli-Edits that referenced this pull request Oct 20, 2026
…mediates

The private keys of eight eMudhra intermediate CAs were stolen (Pasta-Devs#7900).
Browsers block them, but Node.js has no release with the block yet, so
the server would accept a certificate an attacker signs with one of them.

Wrap tls.checkServerIdentity at startup and refuse any chain that
contains one of the eight public keys (their Sectigo cross-signs share
them). Stopgap until the bundled Node includes nodejs/node#66871.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@golywater

Copy link
Copy Markdown

Data point for anyone landing here, plus a userland stopgap until a release containing this PR ships.

Unpatched v24 releases still accept these chains. On v24.22.0 and v24.14.0, against the test hosts from https://emudhra.we-have-a-problem.com, plain fetch() and https.get() with the bundled store return 200 for all 6 {dv,ov,ev}-{g3b,g3c}-emudhra hosts. The served chain is leaf → EM … TLS CA - G3x → emSign Root TLS CA - G3 (cross-signed) → bundled emSign ECC Root CA - G3. The 6 -aaa- hosts already fail with UNABLE_TO_GET_ISSUER_CERT_LOCALLY, because AAA Certificate Services isn't in the bundle.

Stopgap, using the same SPKI-hash approach as this PR. Wrap tls.checkServerIdentity, which Node calls after chain verification succeeds. Walk cert.issuerCertificate, stopping at a self-referencing root or undefined. For each certificate, SHA-256 new crypto.X509Certificate(c.raw).publicKey.export({ type: 'spki', format: 'der' }) and return an Error if it matches one of the 8 hex spki_sha256 values in the disclosure's impacted-intermediates.json. On both versions above, this rejected all 6 hosts through https and fetch(), and the unrelated sites we tried were unaffected.

Caveats, each observed:

  • Install it before the process makes any TLS connection. Resumed sessions skip checkServerIdentity, so a session to a compromised host that was cached before the wrapper went in was resumed afterwards and got 200. Rejected handshakes are not cached, so with the wrapper installed first, repeat requests stayed rejected. Preloading with node --import ./stopgap.mjs app.js does this.
  • Requests that pass their own checkServerIdentity, or rejectUnauthorized: false, bypass it.
  • It's a stopgap for the window until a release with this PR ships, not a replacement.

rbright added a commit to rbright/nix-pi-agent that referenced this pull request Oct 20, 2026
* ci: patch runner CA trust for eMudhra compromise

Upgrade Ubuntu ca-certificates as the first step of every job and fail
closed below 20260816, the first patched release. Set NODE_OPTIONS to
--use-openssl-ca with SSL_CERT_FILE at job level so JavaScript actions
trust the patched system store instead of the runner's bundled Node CA
snapshot, whose fix is still pending (nodejs/node#66871).

Advisory: https://emudhra.we-have-a-problem.com/

* ci: require Ubuntu-patched ca-certificates revision

Plain 20260816 is the unpatched upstream baseline. Require 20260816ubuntu0.22.04.1 or later so the gate fails closed on it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. fast-track PRs proposed for a shorter-than-standard waiting period before landing. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.