Repository navigation
Conversation
ulises-jeremias
left a comment
There was a problem hiding this comment.
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 lintis already broadly failing in the repository. Comparing onlylib/rsa.jsandtests/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:1171has one extra indentation space, andtests/unit/rsa.js:866has 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
left a comment
There was a problem hiding this comment.
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.
|
I rechecked the current head |
Fixed all newly introduced lints @ulises-jeremias |
|
Independent validation of the current head
I also ran |
|
Additional validation of head
The crafted signature remains accepted on upstream Additional runtime check: |
|
@ulises-jeremias @siebertm need approval for the workflow to trigger , than MR can be merged |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
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:
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:
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. |
|
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. |
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.