Skip to content

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

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

crypto: fix X509 curve name crash for non-NIST curves#66694
lazerg wants to merge 1 commit 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.

@lazerg
crypto: fix X509 curve name crash for non-NIST curves
a271385 
Signed-off-by: lazerg <[email protected]>
Assisted-by: Claude Code
@nodejs-github-bot

nodejs-github-bot commented Oct 13, 2026

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

github-actions Bot commented Oct 13, 2026

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 •
edited
Loading

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 (a271385).
⚠️ Report is 4 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%           
=======================================
  Files         791      791           
  Lines      277088   277089    +1     
  Branches    53286    53273   -13     
=======================================
+ Hits       250703   250707    +4     
- Misses      16805    16808    +3     
+ Partials     9580     9574    -6
Files with missing lines Coverage Δ
src/crypto/crypto_x509.cc 73.03% <75.00%> (+0.16%) ⬆️

... and 28 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 •
edited by panva
Loading

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panva panva panva approved these changes

Assignees

No one assigned

Labels

agentscan:community-flagged 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.

Projects

None yet

Milestone

No milestone

3 participants

@lazerg @nodejs-github-bot @panva