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

False positives SQL injections #1571

Open
aglushkov opened this issue Feb 17, 2021 · 4 comments
Open

False positives SQL injections #1571

aglushkov opened this issue Feb 17, 2021 · 4 comments
Milestone

Comments

@aglushkov
Copy link
Contributor

Background

Brakeman version: brakeman 5.0.0
Rails version: Rails 6.0.3.4
Ruby version: ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux]

Issue

There are 4 examples that I believe free from any possible sql injection, but brakeman show warnings

class BrakemanIssues
  # Issue with model name
  def issue1
    votable_type = User.name
    Arel.sql("votes.votable_type = '#{votable_type}'")
  end

  # Issue with variable selected from some constant list
  def issue2_constant_hash_value(role_name)
    roles = { admin: 1, moderator: 2 }.freeze
    role = roles.fetch(role_name)
    Arel.sql("role = '#{role}'")
  end

  # Issue with already escaped variable
  def issue3(query)
    query = ActiveRecord::Base.sanitize_sql_like(query) # escaped variable
    Arel.sql("name ILIKE '%#{query}%'")
  end

  # Issue with constant date
  def issue4
    date = 30.days.ago
    Arel.sql("created_at > '#{date}'")
  end
end
== Warnings ==

Confidence: High
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Arel.sql("votes.votable_type = '#{User.name}'")
File: app/queries/brakeman.rb
Line: 5

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Arel.sql("role = '#{{ :admin => 1, :moderator => 2 }.fetch(role_name)}'")
File: app/queries/brakeman.rb
Line: 12

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Arel.sql("name ILIKE '%#{ActiveRecord::Base.sanitize_sql_like(query)}%'")
File: app/queries/brakeman.rb
Line: 18

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Arel.sql("created_at > '#{30.days.ago}'")
File: app/queries/brakeman.rb
Line: 24

@aglushkov
Copy link
Contributor Author

There are really possible SQL injection is issue3, sorry.
I've used sanitize_sql to fix all this warnings, but it would be great if Brakeman don't argue on class names or constant values

@presidentbeef
Copy link
Owner

I agree - all of these are fixable.

presidentbeef added a commit that referenced this issue May 2, 2021
presidentbeef added a commit that referenced this issue May 2, 2021
presidentbeef added a commit that referenced this issue May 3, 2021
presidentbeef added a commit that referenced this issue May 3, 2021
presidentbeef added a commit that referenced this issue May 3, 2021
@natematykiewicz
Copy link

natematykiewicz commented Jul 20, 2021

@presidentbeef I just want to make sure everyone's on the same page. Example 3 here is open to SQL Injection. sanitize_sql_like simply escapes _ and %.

https://github.com/rails/rails/blob/83217025a171593547d1268651b446d3533e2019/activerecord/lib/active_record/sanitization.rb#L94-L111

@ghost
Copy link

ghost commented Sep 17, 2021

@presidentbeef you ignored almost all sanitize methods. However, you missed method sanitize_sql_for_order from the document https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html#method-i-sanitize_sql_for_order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants