Skip to content

crypto: fix X509 curve name crash for non-NIST curves - #66694

Open
lazerg wants to merge 3 commits into
nodejs:mainfrom
lazerg:fix/issue-66681-x509-curve-name
Open

lazerg wants to merge 3 commits into
nodejs:mainfrom
lazerg:fix/issue-66681-x509-curve-name

Conversation

@lazerg

@lazerg lazerg commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Fixes: #66681

GetCurveName() put the result of EC_curve_nid2nist() straight into a std::string_view. That function returns NULL for curves with no NIST name, like brainpool, so toLegacyObject() and getPeerCertificate() segfault on those certificates. This started with v23.7.0.

The name is now kept as a const char* and nistCurve is undefined when it is NULL. The test uses a brainpoolP256r1 certificate.

Signed-off-by: lazerg <lazerg2@gmail.com>
Assisted-by: Claude Code
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@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. labels Oct 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Caution

AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context.

@codecov

codecov Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.47%. Comparing base (a7a96c2) to head (7a58cc4).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_x509.cc 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66694      +/-   ##
==========================================
- Coverage   90.47%   90.47%   -0.01%     
==========================================
  Files         791      791              
  Lines      277088   277246     +158     
  Branches    53286    53315      +29     
==========================================
+ Hits       250703   250833     +130     
- Misses      16805    16813       +8     
- Partials     9580     9600      +20     
Files with missing lines Coverage Δ
src/crypto/crypto_x509.cc 73.03% <75.00%> (+0.16%) ⬆️

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

@panva panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 13, 2026
@github-actions github-actions Bot 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 13, 2026
@nodejs-github-bot

nodejs-github-bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

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

test-crypto-x509

---
duration_ms: 167.715
exitcode: 1
severity: fail
stack: |-
  node:internal/assert/utils:146
    throw error;
    ^

  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected

  + undefined
  - 'brainpoolP256r1'

      at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-crypto-x509.js:534:12)
      at Module._compile (node:internal/modules/cjs/loader:1968:14)
      at Object..js (node:internal/modules/cjs/loader:2108:10)
      at Module.load (node:internal/modules/cjs/loader:1690:32)
      at Module._load (node:internal/modules/cjs/loader:1480:12)
      at wrapModuleLoad (node:internal/modules/cjs/loader:261:19)
      at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:173:5)
      at node:internal/main/run_main_module:33:47 {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: undefined,
    expected: 'brainpoolP256r1',
    operator: 'strictEqual',
    diff: 'simple'
  }

  Node.js v27.0.0-pre
...

@panva panva removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 13, 2026

@pimterry pimterry 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.

Test needs a mechanical fix to get FIPS passing, but otherwise all LGTM

Comment thread test/parallel/test-crypto-x509.js Outdated
Signed-off-by: lazerg <lazerg2@gmail.com>
Assisted-by: Claude Code
@panva panva added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Oct 13, 2026
@pimterry pimterry 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 13, 2026
@github-actions github-actions Bot 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 13, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment thread test/parallel/test-crypto-x509.js Outdated
const assert = require('assert');
const fixtures = require('../common/fixtures');
const { hasOpenSSL, isBoringSSL } = require('../common/crypto');
const { hasFIPS, hasOpenSSL, isBoringSSL } = require('../common/crypto');

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.

The suggestion below makes this import unnecessary.

Suggested change
const { hasFIPS, hasOpenSSL, isBoringSSL } = require('../common/crypto');
const { hasOpenSSL, isBoringSSL } = require('../common/crypto');

Comment thread test/parallel/test-crypto-x509.js Outdated
Comment on lines +518 to +539
{
// EC certificate on a curve without a NIST name (brainpoolP256r1).
const certPem = `-----BEGIN CERTIFICATE-----
MIIBfzCCASagAwIBAgIUcZKohGvML9PeBwQtylddAdUU3nkwCgYIKoZIzj0EAwIw
FDESMBAGA1UEAwwJYnJhaW5wb29sMCAXDTI2MTAxMjE0NDczMVoYDzIxMjYwOTE4
MTQ0NzMxWjAUMRIwEAYDVQQDDAlicmFpbnBvb2wwWjAUBgcqhkjOPQIBBgkrJAMD
AggBAQcDQgAEKnP14jKQEUyWxWhXrs+ql7OlMy0EW3mnYTvwkF8VyYsPB9dBUJOW
8IHqIik0kjQVSVhtLH0j+hveyZvGd0aQq6NTMFEwHQYDVR0OBBYEFNx4uQEsAiNZ
zcrKuAZ9/9d1eIeDMB8GA1UdIwQYMBaAFNx4uQEsAiNZzcrKuAZ9/9d1eIeDMA8G
A1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDRwAwRAIgHc4TOjOLOPXrvWXRQ44L
jEVmV959vS0DtFIIsrGT3moCIBBRZWdLdd0YjFHUQshMZFM9BN2INpjFNUUxqPAC
3qPo
-----END CERTIFICATE-----`;

if (!isBoringSSL) {
const legacyObject = new X509Certificate(certPem).toLegacyObject();
if (!hasFIPS()) {
assert.strictEqual(legacyObject.asn1Curve, 'brainpoolP256r1');
}
assert.strictEqual(legacyObject.nistCurve, undefined);
}
}

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.

hasFIPS() checks whether FIPS mode is enabled, not whether the linked OpenSSL can decode Brainpool keys. The 111fips failure reaches the assertion with FIPS mode disabled and asn1Curve === undefined.

Can we allow either the curve name or undefined, while keeping the unconditional nistCurve assertion and the conversion call that exercises the crash? Moving the BoringSSL guard around the whole block also keeps certPem scoped with its use.

Suggested change
{
// EC certificate on a curve without a NIST name (brainpoolP256r1).
const certPem = `-----BEGIN CERTIFICATE-----
MIIBfzCCASagAwIBAgIUcZKohGvML9PeBwQtylddAdUU3nkwCgYIKoZIzj0EAwIw
FDESMBAGA1UEAwwJYnJhaW5wb29sMCAXDTI2MTAxMjE0NDczMVoYDzIxMjYwOTE4
MTQ0NzMxWjAUMRIwEAYDVQQDDAlicmFpbnBvb2wwWjAUBgcqhkjOPQIBBgkrJAMD
AggBAQcDQgAEKnP14jKQEUyWxWhXrs+ql7OlMy0EW3mnYTvwkF8VyYsPB9dBUJOW
8IHqIik0kjQVSVhtLH0j+hveyZvGd0aQq6NTMFEwHQYDVR0OBBYEFNx4uQEsAiNZ
zcrKuAZ9/9d1eIeDMB8GA1UdIwQYMBaAFNx4uQEsAiNZzcrKuAZ9/9d1eIeDMA8G
A1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDRwAwRAIgHc4TOjOLOPXrvWXRQ44L
jEVmV959vS0DtFIIsrGT3moCIBBRZWdLdd0YjFHUQshMZFM9BN2INpjFNUUxqPAC
3qPo
-----END CERTIFICATE-----`;
if (!isBoringSSL) {
const legacyObject = new X509Certificate(certPem).toLegacyObject();
if (!hasFIPS()) {
assert.strictEqual(legacyObject.asn1Curve, 'brainpoolP256r1');
}
assert.strictEqual(legacyObject.nistCurve, undefined);
}
}
if (!isBoringSSL) {
// EC certificate on a curve without a NIST name (brainpoolP256r1).
const certPem = `-----BEGIN CERTIFICATE-----
MIIBfzCCASagAwIBAgIUcZKohGvML9PeBwQtylddAdUU3nkwCgYIKoZIzj0EAwIw
FDESMBAGA1UEAwwJYnJhaW5wb29sMCAXDTI2MTAxMjE0NDczMVoYDzIxMjYwOTE4
MTQ0NzMxWjAUMRIwEAYDVQQDDAlicmFpbnBvb2wwWjAUBgcqhkjOPQIBBgkrJAMD
AggBAQcDQgAEKnP14jKQEUyWxWhXrs+ql7OlMy0EW3mnYTvwkF8VyYsPB9dBUJOW
8IHqIik0kjQVSVhtLH0j+hveyZvGd0aQq6NTMFEwHQYDVR0OBBYEFNx4uQEsAiNZ
zcrKuAZ9/9d1eIeDMB8GA1UdIwQYMBaAFNx4uQEsAiNZzcrKuAZ9/9d1eIeDMA8G
A1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDRwAwRAIgHc4TOjOLOPXrvWXRQ44L
jEVmV959vS0DtFIIsrGT3moCIBBRZWdLdd0YjFHUQshMZFM9BN2INpjFNUUxqPAC
3qPo
-----END CERTIFICATE-----`;
const legacyObject = new X509Certificate(certPem).toLegacyObject();
assert.ok([undefined, 'brainpoolP256r1'].includes(legacyObject.asn1Curve));
assert.strictEqual(legacyObject.nistCurve, undefined);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied both suggestions in 3998152.

Signed-off-by: lazerg <lazerg2@gmail.com>
Assisted-by: Claude Code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentscan:community-flagged c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

4 participants