Ruby on Rails SQL Injection - Building a query

Question

I'm resolving all the SQL Injections in a system and I've found something that I don't know how to treat.

Can somebody help me?

Here is my method

def get_structure()
   #build query
   sql = %(
           SELECT pc.id AS "product_id", pc.code AS "code", pc.description AS "description", pc.family AS "family", 
                  p.code AS "father_code", p.description AS "father_description", 
                  p.family AS "father_family"
           FROM products pc
           LEFT JOIN imported_structures imp ON pc.id = imp.product_id
           LEFT JOIN products p ON imp.product_father_id = p.id
           WHERE pc.enable = true AND p.enable = true
   )
   #verify if there is any filter
   if !params[:code].blank?
     sql = sql + " AND UPPER(pc.code) LIKE '%#{params[:code].upcase}%'"
   end
   #many other parameters like the one above
   #execute query
   str = ProductStructure.find_by_sql(sql)
end

Thank you!

This won't solve your issue, but does make things more readable. When having large blocks of text consider using heredoc. Some IDEs also adjust syntax highlighting based on the provided tag. In this case <<~SQL or <<~'SQL' if you don't want string interpolation. — 3limin4t0r
– 3limin4t0r, Commented Dec 4, 2018 at 22:00

engineersmnky · Accepted Answer · 2018-12-04 20:54:27Z

4

You could use Arel which will escape for you, and is the underlying query builder for ActiveRecord/Rails. eg.

products = Arel::Table.new("products")
products2 = Arel::Table.new("products", as: 'p')
imported_structs = Arel::Table.new("imported_structures")
query = products.project(
  products[:id].as('product_id'),
  products[:code],
  products[:description], 
  products[:family], 
  products2[:code].as('father_code'),
  products2[:description].as('father_description'),
  products2[:family].as('father_family')).
  join(imported_structs,Arel::Nodes::OuterJoin).
    on(imported_structs[:product_id].eq(products[:id])).
  join(products2,Arel::Nodes::OuterJoin).
    on(products2[:id].eq(imported_structs[:product_father_id])).
  where(products[:enable].eq(true).and(products2[:enable].eq(true)))
if !params[:code].blank?
  query.where(
     Arel::Nodes::NamedFunction.new('UPPER',[products[:code]])
       .matches("%#{params[:code].to_s.upcase}%")
  )
end

SQL result: (with params[:code] = "' OR 1=1 --test")

SELECT 
  [products].[id] AS product_id, 
  [products].[code], 
  [products].[description], 
  [products].[family], 
  [p].[code] AS father_code, 
  [p].[description] AS father_description, 
  [p].[family] AS father_family 
FROM 
  [products] 
  LEFT OUTER JOIN [imported_structures] ON [imported_structures].[product_id] = [products].[id] 
  LEFT OUTER JOIN [products] [p] ON [p].[id] = [imported_structures].[product_father_id] 
WHERE 
  [products].[enable] = true AND 
  [p].[enable] = true  AND 
  UPPER([products].[code]) LIKE N'%'' OR 1=1 --test%'

To use

ProductStructure.find_by_sql(query.to_sql)

I prefer Arel, when available, over String queries because:

it supports escaping
it leverages your existing connection adapter for sytnax (so it is portable if you change databases)
it is built in code so statement order does not matter
it is far more dynamic and maintainable
it is natively supported by ActiveRecord
you can build any complex query you can possibly imagine (including complex joins, CTEs, etc.)
it is still very readable

edited Dec 4, 2018 at 20:54

answered Dec 4, 2018 at 19:27

engineersmnky

30.5k2 gold badges42 silver badges64 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

3limin4t0r Over a year ago

I have a few questions/comments. 1) Don't you need to save the query.where(...) result (inside the if statement) back into the query variable? 2) I would instantiate the arel tables by simply calling Model.arel_table and in the case you need the alias Model.arel_table.alias('name'). 3) I would use .outer_join(arel_table) over .join(arel_table, Arel::Nodes::OuterJoin).

engineersmnky Over a year ago

@JohanWentholt 1) No. Arel's where is mutating. 2) The code provided can be used with or without rails (Arel only) 3) Yes you could do this

3limin4t0r · Accepted Answer · 2018-12-04 21:24:09Z

1

You need to turn that into a placeholder value (?) and add the data as a separate argument. find_by_sql can take an array:

def get_structure
   #build query
   sql = %(SELECT...)
   query = [ sql ]

   if !params[:code].blank?
     sql << " AND UPPER(pc.code) LIKE ?"
     query << "%#{params[:code].upcase}%"
   end

   str = ProductStructure.find_by_sql(query)
end

Note, use << on String in preference to += when you can as it avoids making a copy.

edited Dec 4, 2018 at 21:24

3limin4t0r

21.4k3 gold badges38 silver badges61 bronze badges

answered Dec 4, 2018 at 17:55

tadman

212k23 gold badges237 silver badges266 bronze badges

Collectives™ on Stack Overflow

Ruby on Rails SQL Injection - Building a query

2 Answers 2

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related