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 - "unless test" is treated differently than "if !test" #1225

Closed
bannable opened this Issue Jun 1, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@bannable
Copy link

bannable commented Jun 1, 2018

Background

Brakeman version: 4.3.0
Rails version: 3.2.22.8
Ruby version: 2.3.7p456

Issue

I noticed that brakeman is returning a false positive depending on how a conditional whitelist is constructed.

False Positive

Full warning from Brakeman:

Confidence: Medium
Category: Command Injection
Check: Execute
Code: `echo #{foo}`
File: lib/hello.rb
Line: 16

Confidence: Medium
Category: Command Injection
Check: Execute
Code: `echo #{foo}`
File: lib/hello.rb
Line: 6

Relevant code:

module HelloWorld
  ALLOWED_FOOS = [:bar, :gaz]
  FROZEN_FOOS = [:qux, :quz].freeze
  def safe_one(foo)
    return if !ALLOWED_FOOS.include?(foo)
    `echo #{foo}`
  end

  def safe_two(foo)
    return unless ALLOWED_FOOS.include?(foo)
    `echo #{foo}`
  end

  def safe_three(foo)
    return if !FROZEN_FOOS.include?(foo)
    `echo #{foo}`
  end

  def safe_four(foo)
    return unless FROZEN_FOOS.include?(foo)
    `echo #{foo}`
  end
end

At first I thought this was the same as #1211, but then I noticed that .freeze doesn't change the behavior as suggested in that issue. It seems like brakeman is failing to recognize the negative conditional test of the form if !test, but it does correctly recognize unless test.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Jun 1, 2018

True, because if !test and unless test are not exactly the same semantically and therefore look very different 😞

presidentbeef added a commit that referenced this issue Sep 6, 2018

Repository owner locked and limited conversation to collaborators Oct 16, 2018

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