-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GHSA-27h2-hvpr-p74q] Request to reject CVE: jsonwebtoken has insecure input validation in jwt.verify function #1595
Conversation
Hi there @esarafianou and @julienwoll! A community member has suggested an improvement to your security advisory. If approved, this change will affect the global advisory listed at github.com/advisories. It will not affect the version listed in your project repository. This change will be reviewed by our highly-trained Security Curation Team. If you have thoughts or feedback, please share them in a comment here! If this PR has already been closed, you can start a new community contribution for this advisory |
I support this claim. This is not a vulnerability. If a malicious 3rdparty succeeds at creating an object with a toString function from inputs to the program, it's what made it possible that we should call a vulnerability. This report could have been filed against the JavaScript The PoC supporting this report shows how to get an RCE if you already have an RCE. It's a validation bug at best. |
I've seen similar issue on msgpack/msgpack-node#56 (CVE-2021-23410 is withdrawn)
|
* 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 <julien.wollscheid@auth0.com>
As I already said in the mentioned comment, I agree that this is complete nonsense. In a situation like this, the deserializer that created a live function from untrusted data would be vulnerable, not thebCode that just runs toString on an object. |
I support this claim. I have yet to see a sensible path to get a malicious toString function in, that doesn’t imply you already have RCE. |
I have code reviewed this yesterday and today, and am glad to see some traction for lowering this CVE/GHSA. It's actually surprising it's even called a bug. It's marked as fixed in v9, and practically it's now on line 120 in verify.js doing an instanceof check, to see that the function passed is a KeyObject (from the node crypto lib). There is no reasonable way someone without access to the codebase could pass a function to the verify() call. One would have to make a wierd serializer and deserialize user input into functions, which is not normal in JS, and JSON does not support functions. I doubt anyone working with JWTs use eval() or exec() on user input... So the real-world application is miniscule at best, doing this remote would be superbly wierd, and even with an edge-case where this would work, the vulnerability would be in passing unsafely deserialized functions to another function (verify()). If one still thinks it's unsafe to let developers pass functions into library functions, you gotta get away from javascript or something. You could always bypass the v9-fix by simply settings your function's TL;DR: I agree to this PR. |
If you allow untrusted data into a secret, that in itself should be a vulnerability as you could potentially alter the secret (I'd call that secret pollution or something). But alright, secrets can be used for exploitation scenarios. This however argues that JavaScript mechanisms for type conversion are unsafe (in this case it's an explicit conversion) and that conversion details shouldn't be left to the object which is being converted. Instead no type conversion should be done on user input and the library should fail if it receives an input that does not match or it should find a way to do type conversion without relying on the object to dictate how it should be converted. I think most people will agree that Javascript type conversion mechanisms are only unsafe if the objects are deserialized in a dangerous way. That's a big if though. |
But that would place the RCE vulnerability into the deserializer, the accused library contains no real vulnerability, and some missing type checks can potentially lead to an uncaptured exception, which is a bug, but creates no RCE and should not lead to a CVE. |
Only if the deserialization triggers code execution. It's possible to deserialize a function without doing code execution (for example through |
I am also in favor of withdrawal of the CVE. I also want point to the timeline provided by Unit42:
5 months to fix something which is claimed to be a critical issue? I dont think so. I think they just appeased Unit42 to just get them off their backs with the "sec. vuln.". |
There is now even a joke on twitter demonstrating that this could even happen in Java.
|
If it is really a vulnerability, I want to ask the discoverer how to fix it and delete the mechanism of System.out.println directly calling toString()? The first interesting thing in 2023 |
Agreed; this CVE should be revoked. The evil code (possibly contained in the secret manager) and the app code need to run in the same execution context for this to work. In other words, the CVE requires that an attacker has code execution capabilities already in that context. If the developer uses eval() to parse tainted text, that is a vulnerability in itself. |
I used https://cveform.mitre.org/ to request a rejection of the CVE. |
Me too, but they rejected the request as @github is responsible... |
Also should be mentioned that the PR linked to this CVE/blog post contains a non-trivial change related to a well-known "null" algorithm issue, which I believe should have been fixed some years ago. As of my point of view this PR should be reviewed by a third party experts before the new major version forced by such an ambiguous CVE would be adopted. |
This is what i got. So if gh does not resolve i would escalate it. But i agree to what you wrote on linkedin: The Panic by this PR stunt costs unnecessary developer time. |
Wrote an Email to security-advisories@github.com to avoid that they say we maybe need to write the requests for rejection only per email.
|
Got a response from them? |
No |
Hello everyone on behalf of GitHub’s Advisory Database Curation team 👋 My team manages the content within the Advisory Database, CVE assignment for the GitHub_M CNA, advisory and CVE feedback within this repository, and the security-advisories@github.com mailbox. I want to thank everyone for the level of effort and review provided for this advisory from the open source and security communities, and appreciate everyone’s patience while we work to address the concerns brought up across these different areas. We are continuing to actively review this and will update this pull request once we have more information. |
I'd also like to point out that
So the parameter can be a function and it most certainly will be called. So if you can argue that an attacker could somehow add a function into an object that is being used as this parameter then you can also argue that an attacker could introduce a function as the parameter itself and run malicious code. But again, neither of them should be the library's fault. |
Why has no action been taken yet? It's been more than 2 weeks that this misleading entry has been spamming audit reports. This CVE needs to be retracted already (and the reporters should be issuing a correction to their blog post, but that's another story) |
Probably the typical strategy to wait it out. After some weeks and months devs will be forced anyway by product and engineering managers to upgrade jsonwebtoken, so that superiors get their security audits clear. And then people like @taladrane will close this issue as stale. |
Hello everyone 👋, On behalf of GitHub’s Advisory Database Curation team, I want to thank everyone for the feedback provided for this advisory. We value our partnership with the open source security community and have been working closely with the maintainers at Auth0 and the original reporting researchers at Palo Alto’s Unit42 to review the public feedback regarding the validity of this vulnerability. After careful discussion, we have decided to take the following steps:
The GitHub Security Lab aims to empower others and foster collaboration in the open source security ecosystem. While the end result of this disclosure process is revoking the CVE, we believe that the collaboration across organizations is a great example of how the community can come together to support each other and positively impact open source for all. More information about best practices during coordinated disclosure of security vulnerabilities can be found here. |
Updates
Comments
The CVE is a joke, it requires the „attacker“ to create an object in the application context, not just parsed JSON but an object with an executable function.
This can only happen if the „attacker“ can modify the source code itself, if that is possible, he may execute the malicious code directly.
See @n1nj4sec reply, even
btoa
contains this so-called "vulnerability" 😱 #1595 (comment)Every known transport to a secret manager will serialize the data using
JSON.parse/JSON.strinigify
, this will render thetoString
property as a string.So even this very hypothetical case constructed in the original disclosure could only happen if the secret manager would be part of the same context (app) so the secret is not serialized and that secret manager would have to contain a vulnerability allowing users to define a function 🤯
References:
CVE: https://www.cve.org/CVERecord?id=CVE-2022-23529
"Disclosure":
https://unit42.paloaltonetworks.com/jsonwebtoken-vulnerability-cve-2022-23529/