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

Possible ActiveRecord false positive? #1182

Closed
JacobEvelyn opened this Issue Apr 24, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@JacobEvelyn
Contributor

JacobEvelyn commented Apr 24, 2018

Background

Brakeman version: 4.2.1
Rails version: 5.1.6
Ruby version: 2.5.0

False Positive

Sorry to bug you since I suspect it doesn't make sense to have brakeman handle this case. We've got a fairly complex query we construct in our code, which uses a subquery. To stay sane, we alias the table in the subquery and use a Ruby constant to ensure we're using the same table alias everywhere. In the end, we have code looking something like this:

SUBQUERY_TABLE_ALIAS = "my_table_alias".freeze

# This is used inside a larger query by using `inner_query.to_sql`
def inner_query
  self.class.
    select("#{SUBQUERY_TABLE_ALIAS}.*").
    from("#{table_name} AS #{SUBQUERY_TABLE_ALIAS}")
end

Brakeman warns us of possible SQL injection here, which is fair as we're injecting a string into our SQL without sanitizing (interestingly, it only warns about the SUBQUERY_TABLE_ALIAS usage, not the table_name usage):

Confidence: Weak
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: self.class.select("#{"my_table_alias".freeze}.*").from("#{table_name} AS #{"my_table_alias".freeze}")
File: app/models/concerns/sequenced.rb
Line: 41

However, since SUBQUERY_TABLE_ALIAS is a frozen constant in our code I wonder if there's a way for brakeman to ignore this.

We're happy to just add this to our config/brakeman.ignore file, but I just wanted to check that you didn't think this is something brakeman should be smarter about.

As always, I'd be happy to work on a PR if I'm able to.

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Apr 24, 2018

Hm, yes. String#freeze should be treated like a regular string, at which point this will go away.

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented Apr 24, 2018

Does it always makes sense to treat String#freeze as a safe string though? What if we're freezing user input? (Or perhaps I misunderstood your comment?)

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Apr 25, 2018

No, what I meant was the freeze call itself should be ignored and the target of the call should be examined.

In your code Brakeman would then see it as

self.class.select("my_table_alias.*").from("#{table_name} AS my_table_alias")

As far as Brakeman only warning about the first interpolation, Brakeman only warns about a given type of vulnerability once per code location (typically one per line).

presidentbeef added a commit that referenced this issue Apr 25, 2018

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented Apr 26, 2018

Ah I see, thanks for the explanation!

In terms of the warnings, Brakeman actually doesn't warn about the use of #{table_name} at all (so if I remove the .freeze then it reports no warnings). I'm assuming that's intentional, since table_name should be safe to call, but based on your comment it sounds like it might not be intentional?

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 6, 2018

Yes, table_name is considered safe and ignored.

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.