Skip to content

Commit

Permalink
Rails sanitize_sql is misleading. It is identity function on strings.
Browse files Browse the repository at this point in the history
No sanitization is performed by sanitize_sql when passed just a string.

This is dangerous.
  • Loading branch information
bcaller committed Dec 18, 2023
1 parent 8264aea commit 10b0d57
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 0 deletions.
34 changes: 34 additions & 0 deletions assets/semgrep_rules/services/activerecord-sanitize-sql-noop.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# ruleid: activerecord-sanitize-sql-noop
ActiveRecord::Base.sanitize_sql("#{channel}_channel_id")

# ok: activerecord-sanitize-sql-noop
sanitize_sql_for_conditions(["name=? and group_id=?", "foo'bar", 4])
# => "name='foo''bar' and group_id=4"

# ok: activerecord-sanitize-sql-noop
sanitize_sql_for_conditions(["name=:name and group_id=:group_id", name: "foo'bar", group_id: 4])
# => "name='foo''bar' and group_id='4'"

# ok: activerecord-sanitize-sql-noop
sanitize_sql_for_conditions(["name='%s' and group_id='%s'", "foo'bar", 4])
# => "name='foo''bar' and group_id='4'"

# ruleid: activerecord-sanitize-sql-noop
sanitize_sql_for_conditions("#{user_generated}")

# ruleid: activerecord-sanitize-sql-noop
sanitize_sql_for_conditions(some_variable)
# possibly a false-positive in the case that some_variable is the correct kind of hash

# ok: activerecord-sanitize-sql-noop
sanitize_sql_for_order([Arel.sql("field(id, ?)"), [1,3,2]])
# => "field(id, 1,3,2)"

# ruleid: activerecord-sanitize-sql-noop
sanitize_sql_for_order("id ASC")
# => "id ASC"
# Yes, directly from the Rails documentation, and not dangerous as constant, but a no-op so bad

# ruleid: activerecord-sanitize-sql-noop
ActiveRecord::Base.sanitize_sql_for_order("#{order} ASC")
# Like, you're just asking for errors to happen if you aren't whitelisting order anyway.
27 changes: 27 additions & 0 deletions assets/semgrep_rules/services/activerecord-sanitize-sql-noop.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
rules:
- id: activerecord-sanitize-sql-noop
patterns:
- pattern-either:
- pattern: ActiveRecord::Base.$FUNC($STR)
- pattern: $FUNC($STR)
- metavariable-regex:
metavariable: $STR
regex: "^[^[]"
- metavariable-regex:
metavariable: $FUNC
regex: "^sanitize_sql(_for_(order|conditions))?$"
message: |
When $FUNC is called with a string argument rather than an array/hash, it returns the string as-is without sanitization.
The method name is dangerously misleading.
The method's intended use is to safely insert variables into a string containing '?' or ':param', producing a valid SQL fragment for use where parameterized queries will not work.
This method will NOT sanitize just a SQL string.
User input here is likely a SQL injection vulnerability.
languages:
- ruby
severity: INFO
metadata:
author: Ben Caller
references:
- https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html
confidence: LOW
source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.yaml

0 comments on commit 10b0d57

Please sign in to comment.