[panva]

X509Certificate can report failed operations as missing fields or negative checks. Invalid validity times return "Bad time value" or Invalid Date, making relational date comparisons return false.

Throw when required fields cannot be converted, certificate encoding or fingerprint calculation fails, or a present AIA or EKU extension cannot be decoded. Apply this to getters and explicit toLegacyObject() calls. Report failed or unsupported private-key comparisons as exceptions.

For verify(), return true for 1 and false for 0. Also return false for a wrong key type or invalid unused bits in the signature BIT STRING. Throw for other negative results.

Use the existing conversion behavior for legacy TLS certificate objects, which are also constructed internally during handshakes. Formatting unrelated metadata must not cause an uncaught exception or reject an otherwise valid connection.

Fixes: #67285

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/security-wg

[mcollina]

lgtm

[codecov]

Codecov Report

❌ Patch coverage is 67.67241% with 75 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.70%. Comparing base (c905bed) to head (7dee17d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_x509.cc	64.59%	37 Missing and 37 partials ⚠️
lib/internal/crypto/x509.js	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #67408      +/-   ##
==========================================
- Coverage   90.71%   90.70%   -0.01%     
==========================================
  Files         802      802              
  Lines      282109   282211     +102     
  Branches    54357    54385      +28     
==========================================
+ Hits       255922   255987      +65     
- Misses      16311    16341      +30     
- Partials     9876     9883       +7     

Files with missing lines	Coverage Δ
lib/internal/crypto/x509.js	96.00% <95.65%> (-0.13%)	⬇️
src/crypto/crypto_x509.cc	71.25% <64.59%> (-0.05%)	⬇️

... and 26 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.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/79029/

[pimterry]

I think this has some problems. These methods are called during normal TLS, and this can add remotely triggerable crashes in normal handshake flows, if somebody uses a broken cert. That's because these are called within X509ToObject, which is called via getPeerCertificate, and normal TLS calls that in two common cases:

• Clients call getPeerCertificate in onConnectSecure after they connect, for every successful fresh connection, but doesn't catch errors. Bad server certs throw errors only catchable via uncaughtException, not normal TLS error handling.
• Servers doing mTLS call socket.getPeerCertificate() to read the client cert after it connects to get identity. This can now throw unexpectedly, even for successfully authorized certs, when it never did before.

Probably other cases too.

This needs something in the TLS flows alongside it, everything that can reach X509ToObject, to guard against this and properly expose failures there too. Either it should fail earlier & clearer (so if any part of the cert is unparseable then the whole connection fails, like any other invalid cert - though this might break some weird connections that do work today), or we should defer the failure somehow out of the normal simple flow (fail in the same way as the other APIs but only when you actually read the invalid field, but not when you initially parse the cert during TLS).

Interestingly, we do already have similar handling to avoid errors during SAN parsing within TLS verification:

node/src/crypto/crypto_x509.cc

Lines 291 to 297 in 925faf4

	if (malformed_name_mode ==
	MalformedSubjectAltNameMode::kReturnLegacySentinel) {
	// The legacy object is used by TLS hostname verification. Return an
	// empty DNS name so verification cannot fall back to the common name,
	// while keeping getPeerCertificate() usable for malformed certificates.
	return OneByteString(env->isolate(), kMalformedSubjectAltName);
	}

That should probably match whatever we do here too.

The main risk is AIA, since other invalid data (dates) will make the cert fail authorization anyway, while AIA is mostly ignored by everybody for simple TLS today but would now be strictly validated.

[panva]

@pimterry You're right. I reproduced the uncaught exception with a trusted certificate containing malformed AIA.

So I've separated the conversion paths. The new conversion exceptions apply to X509Certificate getters and explicit toLegacyObject() calls now while TLS getPeerCertificate() and getCertificate() use their existing field omissions and time diagnostics, so unrelated metadata does not become a new handshake rejection condition.

[pimterry]

Thanks! LGTM with that change.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/79041/