mishoo / UglifyJS

JavaScript parser / mangler / compressor / beautifier toolkit
http://lisperator.net/uglifyjs/
Other
13.16k stars 1.25k forks source link

[CVE-2022-37598]/ Prototype pollution found in ast.js #5699

Closed secdevlpr26 closed 2 years ago

secdevlpr26 commented 2 years ago

Prototype pollution vulnerability in function DEFNODE in ast.js in mishoo UglifyJS 3.13.2 via the name variable in ast.js.

The prototype pollution vulnerability can be mitigated with several best practices described here: https://learn.snyk.io/lessons/prototype-pollution/javascript/

robbytx commented 2 years ago

@Supraja9726 can you please explain the mechanism by which prototype pollution may occur?

My initial assessment (based on literally five minutes of reading the code) is to agree with @alexlamsl that this is NOT a vulnerability because the methods argument is always statically determined by the method calls that exist in ast.js and therefore always under the complete control of the authors of this library.

A prototype pollution vulnerability would require some mechanism by which an attacker could inject problematic property names onto the prototype, which would require the methods argument to be dynamically determined, and I'm just not seeing it.

robbytx commented 2 years ago

FWIW - the reporter has filed many similar prototype pollution issues in other projects over the past several weeks, each associated with a CVE number, but many of which have turned out to be false positives. jdgregson has identified many of them in the comments on https://github.com/xmldom/xmldom/issues/436, and I agree with his assessment that reporting security vulnerabilities based on unverified claims from a static analysis tool is frustrating to maintainers and likely harmful to the open source community. What's more, this individual appears to have filed CVE numbers for each of the reported issues before filing the GitHub issues (since the CVE-# appears in the original issue title), which is actively harmful and irresponsible without a good faith effort to create a working exploit.

@Supraja9726 this behavior should be stopped, and assuming you are acting in good faith, you should personally take responsibility for updating each false positive CVE to repair some of the damage you have caused open source maintainers and users alike. In the future, you should only report issues that either (a) you have verified are likely exploitable (through deeper code analysis or a legitimate exploit) or (b) you are unsure about and are therefore posing as a question, not a statement of fact. You should also defer the creation of a CVE until sufficient verification has been performed to necessitate awareness in the wider open source community.

secdevlpr26 commented 2 years ago

Hello,

The code has been flagged as a potentially vulnerable code, and the CVE has the code's sink and path details.

All the reports are based on the research work of my colleague (you can find her paper's link and her contact details below) and I am reporting them here as per her analysis and records.

https://dl.acm.org/doi/pdf/10.1145/3488932.3497769 - This is the published paper with the Github link to her static analysis tool. Thanks

robbytx commented 2 years ago

Hi @Supraja9726, after reading the linked paper at https://users.encs.concordia.ca/~mmannan/publications/JS-vulnerability-aisaccs2022.pdf, I think the core problem is directly stated in the first sentence under "Limitations and Future Work":

Although we verified several of our findings as part of our case studies, we want to note that, when a function is flagged by our framework as vulnerable, this does not always imply that the project containing this function is vulnerable as well.

This is precisely the case here -- the static analysis tool identified a potentially vulnerable function due to the existence of patterns like object[key] = value, but no further analysis was performed to confirm whether the possible key in question was statically determined or possibly attacker-controlled. In fact, by spending literally five minutes doing a quick review of all usages of the flagged function, I was able to determine that no usages of this function were in fact vulnerable, and therefore this is a false positive reported by the static analysis tool.

Certainly I do not dispute the following additional sentence later in the same paragraph of the paper:

On the other hand, it is still important to identify such functions, as they might be reused in an unsafe place, either in the same project or in a different one. Also, the code outside of the function might change, possibly eliminating the applied protection measures.

However, I would accept this statement only on the condition that such identification is used to guide the search for vulnerable code. Flagging code as outlined in the paper (i.e. by using SemGrep to search vulnerable patterns) is insufficient proof for the existence of a true vulnerability, much less one warranting preemptive CVE assignment.

In summary, your colleague's research (while potentially helpful) is being used in a manner inappropriate to its capability, and a large proportion of the vulnerabilities you have reported are in fact false positives, and I would guess something similar is true for the "9,858 unique detected functions for prototype pollution pattern" reported in the paper's results.

I hope that you and your colleague are able to learn from this experience and use it to inform future research that can improve our collective ability to identify security vulnerabilities. At this point I think you owe it to all of us to re-verify all GitHub issues you've reported based on the results of this tool, and to retract the CVE for any issues including this one where neither you nor the maintainers (if they are willing to help you) are able to sufficiently demonstrate are true vulnerabilities that are potentially exploitable via some potentially untrusted input.

robbytx commented 2 years ago

In case it's not immediately clear, there are significant negative impacts that result from reporting vulnerabilities that turn out to be false positives:

  1. You waste the maintainer(s) time reviewing the vulnerability and either developing unnecessary protections or refuting your claims.
  2. If a CVE number is assigned, you waste energy in systems across the globe that are scanning codebases and systems for the existence of the (falsely) vulnerable software.
  3. If a patch is released (or the CVE is not retracted), you waste human time (like mine) attempting to update our codebases and systems to eliminate versions of (falsely) vulnerable software.
  4. You sow confusion and potentially damage the reputation of valuable (and still secure) open source libraries, as evidenced by some of the comments in #5721, when the "vulnerability" goes unpatched.
  5. You damage your own reputation in the open source community, since all other issues you've reported start to be treated with a healthy dose of skepticism for obvious reasons.

This is why I stress that you must be certain that the project is vulnerable before claiming it as such, and if you're simply not sure, then spend some time attempting to verify it (and get a second opinion when doing so) before you file it as an issue, much less before you request a CVE number.

michaeljauk commented 1 year ago

@robbytx And idea how long it takes to retract this CVE?

cold-wisteria commented 1 year ago

In my capacity as a external party, I have reported to CVE from this form the conclusion of the analyst ( @robbytx ) and the vendor ( Collaborator of this project @alexlamsl ) that CVE-2022-37598 is a false positive. As a result, CVE stafff appear to have taken action to add a NOTE to the Description of CVE-2022-37598. https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-37598 https://nvd.nist.gov/vuln/detail/CVE-2022-37598

RustyButtons commented 1 year ago

Hi all, The BlackDuck Security advisory that is mentioned in this issue (#5781 ) has been deleted. After looking at the files, it does not appear that this should be being flagged as a vulnerability.

As for the CVE, it is still currently under the "Disputed" status, hopefully the CVE will also be deleted soon and this issue can be closed.

javixeneize commented 1 year ago

although it is closed, i agree with @robbytx view. @secdevlpr26 you are just creating the wrong reputation for your user, just running automated scanners and opening cves without any level of analysis. In the future, anything that you would report will be lightly considered, specially if after what everybody is reporting, you still dont pull back the cve

DanielRuf commented 1 week ago

you still dont pull back the cve hopefully the CVE will also be deleted soon

CVEs are normally not deleted, just rejected or disputed.

https://security.stackexchange.com/questions/203494/can-a-cve-be-removed-from-nvd-database

See also the disputed and rejected status description at https://www.cve.org/ResourcesSupport/FAQs and https://nvd.nist.gov/vuln/vulnerability-status