Skip to content

CVE-2026-8539 fixed the root cause of given CVE - #1158

Closed
vasuUN wants to merge 3 commits into
digitalbazaar:mainfrom
vasuUN:CVE-fix
Closed

vasuUN wants to merge 3 commits into
digitalbazaar:mainfrom
vasuUN:CVE-fix

Conversation

@vasuUN

@vasuUN vasuUN commented Oct 7, 2026

Copy link
Copy Markdown

Cause: CVE-2026-85393 allows RSA signature forgery because verification ignores extra children inside the nested DigestAlgorithm sequence.
Fix applied: rsa.js:1174 now requires exactly an OID plus optional NULL.
Shared ASN.1 parsing and valid-signature behavior remain unchanged.

Regression: Added malformed-signature rejection coverage in rsa.js:866.

Validation: RSA suite passed: 101 passing, 4 pending.
Build succeeded;
browser bundles were regenerated.
The core minified bundle rejected the malformed signature.
The “all” bundle runtime check was blocked by legacy jQuery initialization.
Lint remains blocked by existing violations.

@ulises-jeremias ulises-jeremias left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I validated the current head 475b820 locally on Node 22.22.0:

  • npx mocha tests/unit/rsa.js: 101 passing, 4 pending.
  • npm test: 829 passing, 4 pending.
  • npm run lint is already broadly failing in the repository. Comparing only lib/rsa.js and tests/unit/rsa.js, the base has 94 errors / 9 warnings and this head has 96 errors / 9 warnings. The two added errors are in this diff: lib/rsa.js:1171 has one extra indentation space, and tests/unit/rsa.js:866 has an extra blank line.

The targeted fix and full suite pass; please clean up those two new lint errors so the change does not increase the existing lint debt. GitHub currently reports no checks for this PR, and npm still has no node-forge@1.4.1 release to consume.

@vasuUN
vasuUN requested a review from ulises-jeremias October 9, 2026 05:16
@vasuUN

vasuUN commented Oct 9, 2026

Copy link
Copy Markdown
Author

I validated the current head 475b820 locally on Node 22.22.0:

* `npx mocha tests/unit/rsa.js`: 101 passing, 4 pending.

* `npm test`: 829 passing, 4 pending.

* `npm run lint` is already broadly failing in the repository. Comparing only `lib/rsa.js` and `tests/unit/rsa.js`, the base has 94 errors / 9 warnings and this head has 96 errors / 9 warnings. The two added errors are in this diff: `lib/rsa.js:1171` has one extra indentation space, and `tests/unit/rsa.js:866` has an extra blank line.

The targeted fix and full suite pass; please clean up those two new lint errors so the change does not increase the existing lint debt. GitHub currently reports no checks for this PR, and npm still has no node-forge@1.4.1 release to consume.

Addressed lint issue
@ulises-jeremias

@ulises-jeremias ulises-jeremias left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for addressing the extra blank line. I reran the current head f0399e0: npm test still passes (829 passing, 4 pending), and lint errors in the two touched files drop from 96 to 95. One new error remains: ESLint reports lib/rsa.js:1171 expects 10 indentation spaces but currently finds 16. Please align the comment with the surrounding block, then rerun the touched-file lint and upstream CI.

@ulises-jeremias

Copy link
Copy Markdown

I rechecked the current head f0399e0: the full npm test suite passes (829 passing, 4 pending), including the new malformed DigestAlgorithm regression. Targeted ESLint still reports one new error in this diff at lib/rsa.js:1171: expected 10 spaces, found 16. This appears to be indentation on the updated // validate DigestInfo structure and nested element counts comment. Please align it with the surrounding block and run the project lint/CI; the current package release is still 1.4.0.

@vasuUN

vasuUN commented Oct 9, 2026

Copy link
Copy Markdown
Author
  • lib/rsa.js and tests/unit/rsa.js

Fixed all newly introduced lints @ulises-jeremias

@vasuUN
vasuUN requested a review from ulises-jeremias October 9, 2026 07:11
@ulises-jeremias

Copy link
Copy Markdown

Independent validation of the current head f192c67:

  • The crafted malformed RSA signature from the new regression test is accepted by upstream main (7232404) and rejected by this PR with the expected invalid DigestInfo error.
  • npm test passes: 829 passing, 4 pending.
  • No GitHub status checks are currently reported for this PR.

I also ran npm run lint. It fails on thousands of existing errors across unrelated files, including parser errors on syntax already present in the repository. The lint job is commented out in .github/workflows/main.yaml; the updated indentation in this PR's changed hunk is corrected.

@ulises-jeremias

ulises-jeremias commented Oct 9, 2026 •

Copy link
Copy Markdown

Additional validation of head f192c67 using the repository's documented jobs:

  • npm test: 829 passing, 4 pending.
  • npm run coverage-ci: passed (same 829 passing, 4 pending).
  • CHROME_BIN=/usr/bin/chromium npm run test-karma (webpack): 829 browser tests completed, 3 skipped.
  • BUNDLER=browserify CHROME_BIN=/usr/bin/chromium npm run test-karma: 828 browser tests completed, 3 skipped.

The crafted signature remains accepted on upstream main and rejected on this PR. GitHub still reports no status checks for the PR. The repository's global lint command fails on thousands of unrelated baseline errors; its lint workflow job is disabled.

Additional runtime check: fnm exec --using=v24.17.0 npm test passes as well (829 passing, 4 pending).

@vasuUN

vasuUN commented Oct 9, 2026

Copy link
Copy Markdown
Author

@ulises-jeremias @siebertm need approval for the workflow to trigger , than MR can be merged

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.29%. Comparing base (7232404) to head (f192c67).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
+ Coverage   80.25%   80.29%   +0.04%     
==========================================
  Files          42       42              
  Lines        9597     9597              
==========================================
+ Hits         7702     7706       +4     
+ Misses       1895     1891       -4     
Files with missing lines Coverage Δ
lib/rsa.js 79.23% <ø> (+1.15%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7232404...f192c67. Read the comment docs.

🚀 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.

@gabradohub

Copy link
Copy Markdown

Thank you for the work on the RSA signature verification fixes in #1152 and #1158.

We independently validated both proposed fixes on Node 24.21.0.

Validation results

We also identified an inherited "describe.only" marker in "tests/unit/jsbn.js" on #1152's base, which causes its normal "npm test" to execute only five tests. Running the remaining suites through a separate entrypoint produced 824 passing / 4 pending, in addition to the five jsbn tests.

Neither candidate has a clean lint baseline. Both exact PR bases already report 8,227 errors and 270 warnings. PR #1152 adds one warning; PR #1158 does not change the baseline counts.

Our core bundle runtime checks used a Node VM with browser globals and WebCrypto, not a real browser. The all bundle requires jQuery and was not runtime-verified.

Additional ASN.1 strictness observation

Both candidates still accept nonempty ASN.1 NULL parameter contents. We have not demonstrated an additional forgery exploit from this behavior.

Our downstream backport adds a small parameter-content check after the proposed nested-element validation:

         obj.value[0].value.length !==
  •          (('parameters' in capture) ? 2 : 1)) {
    
  •          (('parameters' in capture) ? 2 : 1) ||
    
  •        ('parameters' in capture && capture.parameters !== '')) {
    

With this additional check, all 30 downstream checks pass, alongside 829 passing / 4 pending upstream Node tests.

The executable RSA fixes appear equivalent based on our testing. We favor preserving #1152's signed regression vector when integrating the selected fix into the maintained release base.

Maintainer guidance requested

Could a maintainer advise:

  1. Which maintained release branch should receive the fix?
  2. Whether the additional ASN.1 NULL parameter-content check is appropriate for the official patch?
  3. Whether any further compatibility, browser, security or release validation is required?
  4. Whether there is an expected path toward an officially published patched npm version?

We can provide the independent probes, exact-commit results and downstream validation evidence.

Our goal is to remain on official upstream dependencies, preserve compatibility and maintain strict security auditing without introducing a fork or advisory exception.

Thank you.

@davidlehn

Copy link
Copy Markdown
Member

Thank you for the fix. CVE-2026-85393 is fixed in node-forge 1.4.1 (GHSA-2g46-22mc-6gwp). Checking element counts leaves other non-canonical encodings open (NULL parameters with content, non-minimal lengths), so the release re-encodes the DigestInfo and compares bytes instead. Closing.

@davidlehn davidlehn closed this Oct 27, 2026
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.

5 participants