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

SQL injection false negative for connections on complex objects #1678

Open
EQuincerot opened this issue Feb 9, 2022 · 2 comments
Open

SQL injection false negative for connections on complex objects #1678

EQuincerot opened this issue Feb 9, 2022 · 2 comments

Comments

@EQuincerot
Copy link

Background

Brakeman version: 5.2.1
Rails version: 6 and 7
Ruby version: 3.0.3p157

def not_detected_injection_risk(query)
  base_record = [ActiveRecord::Base].find {true}
  base_record.connection.exec_query("SELECT * where x = #{query}")
end

def detected_injection_risk(query)
  base_record = [ActiveRecord::Base].first
  base_record.connection.exec_query("SELECT * where x = #{query}")
end

Issue

The same SQL injection risk exists in both functions. The first one is not detected whereas the second one is correctly detected.

@presidentbeef
Copy link
Owner

This is really unusual code.

There are two bits to this:

  1. Brakeman looks for connection calls on several potential targets, including ActiveRecord::Base.
  2. Brakeman can pick the first item out of an array, but there's no way it's going to support calls to e.g. find with an arbitrary block.

We could assume that any connection call is going to be on an ActiveRecord model. That would introduce false positives, but maybe not very many.

But before we go down that road, it would be helpful to know if this was based on real code or not.

@EQuincerot
Copy link
Author

Yes this is the kind of code we use in production. As we are using multiple database, we have for example this use case:

   DatabaseHelper.all_databases # returns Db1Record, Db2Record (abstract model classes with rules to connect to db1 or db2)
   DatabaseHelper.all_databases.find { |db| db.connection.tables.include?(table) }.connection.execute_query("VACUUM ANALYZE #{table_name}")

Rely on connection would work for us, I don't think that will cause false positives in our situation.

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

2 participants