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

Several issues with using this plugin #22

Open
rossPatton opened this issue Oct 24, 2017 · 3 comments
Open

Several issues with using this plugin #22

rossPatton opened this issue Oct 24, 2017 · 3 comments

Comments

@rossPatton
Copy link

  1. The security/detect-non-literal-fs-filename rule reports fs.${whatever} regardless of whether that's what i'm using or not. For example, i use jsonFile a lot, so an error with jsonFile.readFile gets reported as fs.readFile, when there is no reference to fs on that page. Sure, the error probably still applies, but it's confusing when the error output doesn't line up with the actual code

screen shot 2017-10-23 at 10 41 43 pm

  1. It's unclear, even with the linked resources, what exactly needs to be done to resolve an issue, or even what the issue is. With the above error, I was not at all sure what the problem really was or how to resolve it. For reference, that file (and just about all instances of that error on my code) are just using paths like this: ${process.cwd()}/path/to/json. I have a hard time seeing how this is exploitable?

  2. I'm getting several unsafe regex errors. Most of them were common regexes (verify zip codes, stuff like that). So as recommended, I used the OWASP link to replace them with their safe variants. Even after replacing them, the error still persists

  3. detect-object-injection. As this test is currently designed, it's completely overwhelming

I really want to use this plugin to easily catch minor security issues, but as is, i can't imagine it doing anything more than frustrate people

@evilpacket
Copy link
Contributor

1 - This is a known issue unfortunately. If you look at the source you can see that we are only looking for method calls. If somebody wants to track those references we would accept a pull request.

2 - We could probably improve the documentation here. To clarify for you it's simply pointing out that it could be user input in one of those calls (a variable vs a literal string), which could or could not be bad depending on the context.

3- We process these using a module called safe-regex which has known false positives AND false negatives. We use this rule to trigger re's that may need further in

4- Yup. this is a pretty overwhelming one. We have used it to find issues but it's most certainly a needle in a haystack type assessment when this rule is enabled.

Thanks for your feedback. We use these rules to audit code and identify issues quickly for human review. We find them useful. I'm sorry they frustrate you. We would love them to be better and unless we find significant time to invest or get pull requests to do more state tracking with the underlying ast we can't do much more than what we have.

@lirantal
Copy link
Contributor

@rossPatton about safe-regex, it may also report issues due to a potential complexity detected.

@ota-meshi
Copy link
Member

I think 1 and 2 can probably be fixed in #109.

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

No branches or pull requests

4 participants