False positive SQL injection warning when using I18n.locale

[kreintjes]

Background

Brakeman version: 5.0.1 Rails version: 6.0.3.7 Ruby version: 2.7.3

Link to Rails application code: not to be disclosed

False Positive

Full warning from Brakeman:

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Country.where("lower(source_slug) = :country_id OR lower(slug_#{I18n.locale.to_s.split("-").first}) = :country_id", :country_id => params[:new_country_id])
File: app/controllers/some_controller.rb
Line: N

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Location.joins(:region => :country).select("locations.slug_#{I18n.locale.to_s.split("-").first}, locations.source_slug, COALESCE(locations.slug_#{I18n.locale.to_s.split("-").first}, locations.source_slug) as location_slug, countries.slug_#{I18n.locale.to_s.split("-").first} as country_slug, COALESCE(regions.slug_#{I18n.locale.to_s.split("-").first}, regions.source_slug) as region_slug")
File: app/controllers/some_controller2.rb
Line: N

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Country.where(:code => codes).order("name_#{I18n.locale.to_s.split("-").first}")
File: app/helpers/some_helper.rb
Line: N

Relevant code:

Country.where("lower(source_slug) = :country_id OR lower(slug_#{I18n.locale.to_s.split("-").first}) = :country_id", country_id: params[:new_country_id]).first

Location.joins(region: :country).select("locations.slug_#{locale}, locations.source_slug, COALESCE(locations.slug_#{locale}, locations.source_slug) as location_slug, countries.slug_#{locale} as country_slug, COALESCE(regions.slug_#{locale}, regions.source_slug) as region_slug").where("locations.region_id IN (?) OR locations.has_cross_border_landing_page = true", @regions.ids)

Country.where(code: codes).order("name_#{I18n.locale.to_s.split("-").first}")

Why might this be a false positive?

Brakeman warns about a potential SQL injection vulnerability, although this could not be the case since Rails checks the input passed to I18n.locale = if it matches one of the I18n.available_locales. If you pass some untrusted input, like I18n.locale = "thisisn'tsafe", then you get an error: I18n::InvalidLocale: "thisisn'tsafe" is not a valid locale. So this cannot be an actual SQL injection.

I can't seem to fix or remove this false positive. I tried adding method sanitized_locale which checks the I18n.locale to a whitelist (in the same file), but Brakeman still sees this as a vulnerability. I tried adding sanitized_locale to the --safe-methods option, but this doesn't work either (it appears these methods are only used for the XSS scan). It seems there are only two ways of removing this false positive now:

• Ignoring each occurrence manually using https://brakemanscanner.org/docs/ignoring_false_positives/. This feels a bit cumbersome. We have these queries often in our codebase.
• Wrapping each occurrence in ApplicationRecord.sanitize_sql(...) (so for example: Country.where(code: codes).order("name_#{ApplicationRecord.sanitize_sql(I18n.locale.to_s.split("-").first)}"). Feels a bit cumbersome as well, leading to long lines (of which then Rubocop complains :p). It also doesn't actually fix the SQL vulnerability if there were actually one.

Anyone has any suggestions for a nice and preferably application-wide fix (or ignore method)?

[kreintjes]

Note: the correct way to fix this potential SQL injection vulnerability is probably to wrap each column in ApplicationRecord.connection.quote_column_name. So for example: Country.where(code: codes).order(ApplicationRecord.connection.quote_column_name("name_#{I18n.locale.to_s.split("-").first}")). This works and Brakeman views this as secure (although I wrongfully thought it didn't at first). However it still feels a bit cumbersome to do this for every occurrence when there isn't an actual vulnerability.

[presidentbeef]

We should just ignore I18n.locale in SQL. 👍