Skip to content
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

False positive: Dangerous JS Functions #7732

Open
ericpappas1 opened this issue Feb 9, 2023 · 3 comments
Open

False positive: Dangerous JS Functions #7732

ericpappas1 opened this issue Feb 9, 2023 · 3 comments

Comments

@ericpappas1
Copy link

When scanning my application I saw this Dangerous JS Functions alert (id 10110) show up. After investigating I was able to pinpoint that it is being generated from a piece of code in an angular-generated file in the node_modules folder.

The scan believes we are using the bypassSecurityTrustHtml function and reports it as a 'Dangerous JS Function'. After further investigation, our application is not using this function but the scan is finding the function definition in the node_modules folder. I was able to find this function definition in this file node_modules/@angular/platform-browser/fesm2020/platform-browser.mjs

Screen Shot 2023-02-09 at 9 30 39 AM

I tried removing this function definition and no longer using the @angular/platform-browser package. When I removed this the scan found another instance where it is finding the word 'eval' in the comment of this angular-generated file and reported it as a 'Dangerous JS Function'. This comment is stemming from this file node_modules/@angular/core/fesm2020/core.mjs

Screen Shot 2023-02-09 at 9 25 27 AM

If I were to remove the comment in the angular file and then rerun the scan I would see the scan pick up another instance of a comment with the word 'eval' and report it as a 'Dangerous JS Function'. It seems that this might be a false positive and the scan is incorrectly reporting this vulnerability.

@kingthorin
Copy link
Member

Be aware the highlighting is first match. So what's being highlighted might not be what the rule triggered on. (We know this is a weakness and have an existing issue covering it.)

@psiinon
Copy link
Member

psiinon commented Feb 9, 2023

I think the problem here is the modern practice of bundling.
A mass of JS from complex frameworks is merged into one JS file.
That includes lots of code from those frameworks which may need to do tricky things.
Bundling should remove a lot of JS that isnt used, but it wont remove all of it.

I'm not sure theres a good solution here.
When frameworks had their own files we had the option to allowlist them.
Thats not the case here.
But ignoring all JS files for this rule also doesnt make sense.

Right now I think the only realistic option is to treat these as False Positives on a case by case basis, ie by the user.

Unless anyone else has any better suggestions?

@kingthorin
Copy link
Member

Right now I think the only realistic option is to treat these as False Positives on a case by case basis, ie by the user.

That seems like the most reasonable plan to me.

https://www.zaproxy.org/docs/desktop/addons/alert-filters/

Also once we have ANTLR integrated we might have a better way to exclude JS comments from some passive rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants