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

Command injection false positive with nested system call #1399

Closed
toupeira opened this issue Sep 19, 2019 · 3 comments · Fixed by #1408

Comments

@toupeira
Copy link

@toupeira toupeira commented Sep 19, 2019

Background

Brakeman version: 4.6.1
Rails version: 5.2.3
Ruby version: 2.6.3

False Positive

Full warning from Brakeman:

Confidence: Medium
Category: Command Injection
Check: Execute
Message: Possible command injection
Code: system(*["foo", "bar", "#{value}"])
File: lib/foo.rb
Line: 5

Relevant code:

system(*%W(foo bar #{value}))

module Foo
  def test1
    system(*%W(foo bar #{value}))
  end

  def test2
    system('foo', 'bar', value)
  end
end

Note: I had to place this file in e.g. lib/foo.rb rather than the toplevel directory for Brakeman to pick it up, even though I passed --only-files foo.rb.

Why might this be a false positive?

The first system call outside of Foo is the same as in test1, but not reported as a warning.

Using a normal literal instead of %W() as in test2 doesn't get reported as a warning either, but this seems unnecessary. I originally thought %W() can result in variables containing whitespace to be interpreted as two arguments, but Ruby is smart enough to protect against that:

%W(#{'foo bar'}) === ['foo bar'] # -> true
@presidentbeef

This comment has been minimized.

Copy link
Owner

@presidentbeef presidentbeef commented Sep 20, 2019

Hi @toupeira thank you for reporting!

It seems that the string interpolation in test1 is being wrongly treated as dangerous.

@presidentbeef

This comment has been minimized.

Copy link
Owner

@presidentbeef presidentbeef commented Oct 15, 2019

I saw this fixed some issues in GitLab(-ci/hq) and thought "Huh, looks like someone does use this code pattern" and then I realized 🤣

@toupeira

This comment has been minimized.

Copy link
Author

@toupeira toupeira commented Oct 17, 2019

I saw this fixed some issues in GitLab(-ci/hq) and thought "Huh, looks like someone does use this code pattern" and then I realized 🤣

Haha yeah... which reminds me, I still need to finish watching those security training videos with you and Jim 😁

Thanks for the fix! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.