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

Command injection warning expected but not found #1179

Closed
JacobEvelyn opened this Issue Apr 20, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@JacobEvelyn
Contributor

JacobEvelyn commented Apr 20, 2018

Background

Brakeman version: 4.2.1
Rails version: 5.1
Ruby version: 2.5.1

Issue

I noticed that brakeman correctly warns me about the shell injection vulnerability in something like the following:

arguments = [method_that_returns_user_input, "--some-flag"].join(" ")

system("ruby #{arguments}")

However, I expect to see the same false-positive warning when I rearrange this code like so:

command = ["ruby", method_that_returns_user_input, "--some-flag"].join(" ")

system(command)

and yet I do not. Is there any way to modify brakeman's code to add this check? I'd be happy to help write a PR but I'd need some guidance on where to start.

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Apr 21, 2018

Hi Jacob,

Thank you for bringing this up! I can think of two ways to do this.

The simpler way would be to modify the current check to simply look for Array#join calls as arguments and then check the members of the array.

The other (possibly cooler) way would be to convert all Array#join calls to strings/string interpolation.
For example:

["ruby", method_that_returns_user_input, "--some-flag"].join(" ")

Would become

"ruby #{method_that_returns_user_input} --some-flag"

I kind of like the second option better. It may help a lot of different checks to have this behavior.

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented Apr 23, 2018

Thanks for the response, @presidentbeef! I agree that the second way you suggest would be much better and more broadly useful. Do you have suggestions for where I should get started to work on this feature? (Or, if it's easy for you to just go ahead and implement it, I'm happy with that too.)

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 9, 2018

Seems like I forgot to mention this was addressed in #1198.

Repository owner locked and limited conversation to collaborators Jul 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.