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

Ignore injection warnings from strings in constant array? #1208

Closed
JacobEvelyn opened this Issue May 14, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@JacobEvelyn
Contributor

JacobEvelyn commented May 14, 2018

Background

Brakeman version: 4.3.0
Rails version: 5.1.6
Ruby version: 2.5.1

False Positive

We've noticed that Brakeman assumes that constants in constant arrays are user-supplied inputs for things like SQL/shell injection warnings. For example:

EXPRESSIONS = ["users.email", "concat_ws(' ', users.first_name, users.last_name)"]

def scopes(base_scope)
  EXPRESSIONS.map { |exp| base_scope.where("#{exp} ILIKE '%foo%'") }
end

def perform_commands
  EXPRESSIONS.each { |exp| `echo #{exp}` }
end

I'd assumed this would be too tricky for Brakeman to catch, but now I'm actually wondering how bad it would be to have brakeman understand some simple iterations on constant arrays like these. But obviously I must defer to you.

Thoughts?

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented May 22, 2018

Related to this, it seems we can't even use the whitelisting approach within brakeman. This still produces a warning:

EXPRESSIONS = ["users.email", "concat_ws(' ', users.first_name, users.last_name)"]

def scopes(base_scope)
  EXPRESSIONS.map do |exp|
    raise "Error" unless EXPRESSIONS.include?(exp) # Should prevent warning?
    base_scope.where("#{exp} ILIKE '%foo%'")
  end
end
@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 22, 2018

Iteration over a collection of literal values is possible but I have a vague feeling there may have been unintended consequences when trying to do so in the past. I can take another look.

To explain a little bit: what Brakeman does in these cases is replace exp with one of the literal values. This causes the warning to go away (typically). The problem is if there are other things depending on those values, then issues can get masked.

In the case of your second example there appears to be an odd interaction with the block argument. exp is getting replaced with (exp or "users.email"). This goes away if you remove the block. I'll look into it.

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented May 22, 2018

Thanks for the explanation and for digging in! I also noticed that, curiously, the warning goes away in my last example if we change the raise "Error" ... to instead be a conditional:

def scopes(base_scope)
  EXPRESSIONS.map do |exp|
    if EXPRESSIONS.include?(exp)
      base_scope.where("#{exp} ILIKE '%foo%'")
    end
  end
end
@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 22, 2018

I suspect the issue is scoping. It's easy with an if expression to manage the values inside the branches. But things like raise and return are harder to get right.

@bannable

This comment has been minimized.

bannable commented Jun 7, 2018

Thanks for the fix! I'm still able to trigger a false positive in 4.3.1 with this code though:

  def safe(foo = false)
    quids = foo ? %w(three four) : %w(five six)
    quids.each do |t|
      `echo #{t}`
    end
  end

In fact, it also happens if quids is assigned in either of these ways:

    quids = if foo
              %w(three four)
            else
              %w(five six)
            end

# or...

    if foo
      quids = %w(three four)
    else
      quids = %w(five six)
    end

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.