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

Incorrect SQL injection detection. #680

Closed
schneidmaster opened this Issue Jul 9, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@schneidmaster

schneidmaster commented Jul 9, 2015

I am having issues with incorrect SQL injection detection within a scope. I reduced it to this trivial example. The Project model has a parent_id attribute and the following scope:

scope :not_child_of, lambda { |project|
  if project.id
    where('parent_id != ?', project.id)
  else
    Project.all
  end
}

For some reason Brakeman is convinced that the Project.all line is a SQL injection vulnerability. Even more bizarrely, the warning disappears if I change Project.all to just all. Am I missing something that makes that line unsafe or is it a bug?

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Jul 9, 2015

Owner

Hi Zach,

I am seeing the same thing, and it is really weird. I'll check it out.

Owner

presidentbeef commented Jul 9, 2015

Hi Zach,

I am seeing the same thing, and it is really weird. I'll check it out.

@unmanbearpig

This comment has been minimized.

Show comment
Hide comment
@unmanbearpig

unmanbearpig Jul 21, 2015

Hi!
I've taken a look at this and I've found something I don't understand, which may be the problem.

It's in find_dangerous_value method in check_sql.rb

    when :call
      unless IGNORE_METHODS_IN_SQL.include? exp.method
        if has_immediate_user_input? exp or has_immediate_model? exp
          exp

I'm don't really understand, why are we checking for has_immediate_model? here?
Judging by the name of the method we're checking if exp is a call on a model class, correct? If so, then I don't get how can it cause SQL injection.

The problem disappears when I delete or has_immediate_model? exp , and the tests still pass.

By the way, I've managed to reduce the example a bit further:

scope :not_a_sql_injection, lambda {
    if true
      Project.hello
    end
  }

It doesn't matter which class method we're calling as long as it's on the model class and inside an if.

unmanbearpig commented Jul 21, 2015

Hi!
I've taken a look at this and I've found something I don't understand, which may be the problem.

It's in find_dangerous_value method in check_sql.rb

    when :call
      unless IGNORE_METHODS_IN_SQL.include? exp.method
        if has_immediate_user_input? exp or has_immediate_model? exp
          exp

I'm don't really understand, why are we checking for has_immediate_model? here?
Judging by the name of the method we're checking if exp is a call on a model class, correct? If so, then I don't get how can it cause SQL injection.

The problem disappears when I delete or has_immediate_model? exp , and the tests still pass.

By the way, I've managed to reduce the example a bit further:

scope :not_a_sql_injection, lambda {
    if true
      Project.hello
    end
  }

It doesn't matter which class method we're calling as long as it's on the model class and inside an if.

@rudy-on-rails

This comment has been minimized.

Show comment
Hide comment
@rudy-on-rails

rudy-on-rails Jan 3, 2017

+1

Having the same issue here. Slightly different implementation here:

scope :featured, -> (group) do
  if ProductCollection.featured.any?
    ProductCollection.featured.first.products.available_for(group)
  else
    ProductCollection.none
  end
end

It is def related with the condition. Once you get rid of it, it works fine.

rudy-on-rails commented Jan 3, 2017

+1

Having the same issue here. Slightly different implementation here:

scope :featured, -> (group) do
  if ProductCollection.featured.any?
    ProductCollection.featured.first.products.available_for(group)
  else
    ProductCollection.none
  end
end

It is def related with the condition. Once you get rid of it, it works fine.

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Jan 28, 2017

Owner

This is resolved with #985 and did ultimately use @unmanbearpig's suggestion.

Owner

presidentbeef commented Jan 28, 2017

This is resolved with #985 and did ultimately use @unmanbearpig's suggestion.

Repository owner locked and limited conversation to collaborators May 18, 2017

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