diff --git a/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.rb b/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.rb new file mode 100644 index 00000000..a1177523 --- /dev/null +++ b/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.rb @@ -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. \ No newline at end of file diff --git a/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.yaml b/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.yaml new file mode 100644 index 00000000..fb332b9f --- /dev/null +++ b/assets/semgrep_rules/services/activerecord-sanitize-sql-noop.yaml @@ -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 \ No newline at end of file