Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request from GHSA-8cf7-32gw-wr33
* Check if node version supports asymmetricKeyDetails * Validate algorithms for ec key type * Rename variable * Rename function * Add early return for symmetric keys * Validate algorithm for RSA key type * Validate algorithm for RSA-PSS key type * Check key types for EdDSA algorithm * Rename function * Move validateKey function to module * Convert arrow to function notation * Validate key in verify function * Simplify if * Convert if to switch..case * Guard against empty key in validation * Remove empty line * Add lib to check modulus length * Add modulus length checks * Validate mgf1HashAlgorithm and saltLength * Check node version before using key details API * Use built-in modulus length getter * Fix Node version validations * Remove duplicate validateKey * Add periods to error messages * Fix validation in verify function * Make asymmetric key validation the latest validation step * Change key curve validation * Remove support for ES256K * Fix old test that was using wrong key types to sign tokens * Enable RSA-PSS for old Node versions * Add specific RSA-PSS validations on Node 16 LTS+ * Improve error message * Simplify key validation code * Fix typo * Improve error message * Change var to const in test * Change const to let to avoid reassigning problem * Improve error message * Test incorrect private key type * Rename invalid to unsupported * Test verifying of jwt token with unsupported key * Test invalid private key type * Change order of object parameters * Move validation test to separate file * Move all validation tests to separate file * Add prime256v1 ec key * Remove modulus length check * WIP: Add EC key validation tests * Fix node version checks * Fix error message check on test * Add successful tests for EC curve check * Remove only from describe * Remove `only` * Remove duplicate block of code * Move variable to a different scope and make it const * Convert allowed curves to object for faster lookup * Rename variable * Change variable assignment order * Remove unused object properties * Test RSA-PSS happy path and wrong length * Add missing tests * Pass validation if no algorithm has been provided * Test validation of invalid salt length * Test error when signing token with invalid key * Change var to const/let in verify tests * Test verifying token with invalid key * Improve test error messages * Add parameter to skip private key validation * Replace DSA key with a 4096 bit long key * Test allowInvalidPrivateKeys in key signing * Improve test message * Rename variable * Add key validation flag tests * Fix variable name in Readme * Change private to public dsa key in verify * Rename flag * Run EC validation tests conditionally * Fix tests in old node versions * Ignore block of code from test coverage * Separate EC validations tests into two different ones * Add comment * Wrap switch in if instead of having an early return * Remove unsupported algorithms from asymmetric key validation * Rename option to allowInvalidAsymmetricKeyTypes and improve Readme * 9.0.0 * adding migration notes to readme * adding changelog for version 9.0.0 Co-authored-by: julienwoll <[email protected]>
- Loading branch information
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIST has given the CVE for this at 9.8 severity. NIST CVE-2022-23540
Based on the comments here that seems way too high.
Is it correct that a
secretOrPublicKey
must not be specified for the algorithm to be set tonone
? Could the advisory be updated to note this? Right now it says everybody is vulnerable if they don't specify the algorithm.@julienwoll tagging you since you published the advisory.
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nickhobbs94, that was a great feedback, we updated the advisory: GHSA-qwph-4952-7xr6
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it just me or is CVE-2022-23529 / GHSA-27h2-hvpr-p74q complete nonsense?
The entire "vulnerability" is based on an object with a malicious
toString
function being passed - but the very existance of a malicioustoString
function should not happen in a sane situation.If any deserializer allows creation of arbitrary functions, it's the deserializer that has a vulnerability - not a library that calls
toString
on the deserialized object (especially when the library explicitly states thatthe argument should be of type String or Buffer, as this library does).
By this definition of a "vulnerability", even
JSON.parse
, which should obviously be safe for untrusted input, would be vulnerableAlso taking the nonsensical "PoC" code into account (running code by creating a process
exit
event handler, then exiting the process instead of just running the code),and the fact that this seems to have been submitted as "Arbitrary File Write via verify function" when it would just be a full RCE if it were real,
makes it seem like this was submitted by a complete beginner.
At the same time, the GHSA is marked as "GitHub Reviewed" so it somehow passed some sort of manual check?
Someone please enlighten me if this is nonsense or i'm just going insane.
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly unless there some sort of prototype pollution but in that case it would be a vulnerable application than the package but if we want to make sure then let the package take the reigns
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would even make
console.log
in node.js vulnerable:The CVSS score of 7.6 from Github and 9.8 is NVD is completely overblown. If you can exploit this you already have RCE, because you can already supply code.
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, in your example the string concatenation
"x" + obj
causes this, notconsole.log
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leumasme yes, its completely made up github/advisory-database#1595
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice bug, I wish there was Legit Impact on Impact Metrics for CVSS..
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leumasme You are right, and that actually makes it worse 😄
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it was ChatGPT generated
e1fa9dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is a sec. Vuln. in nodejs?
Lol.