`columns_hash` an acceptable whitelist for SQL injections?

[chrisarcand]

I have an SQL injection warning detected since updated to the latest Brakeman:

# 'from' is passed in to this method
hdws = Hardware.where("#{from}_id" => targets).select(select)

1. Should this be a warning at all? It was pointed out to me that ActiveRecord will automatically quote the column name passed in like this.

2. On the other hand, I can see why Brakeman might still consider this worth a warning, so I whitelisted it like this (successfully):

column_name = Hardware.column_names.detect { |col| col == "#{from}_id" }
hdws  = Hardware.where(column_name => targets).select(select)

But that's O(n) lookup, let's do O(1) instead:

column_name = Hardware.columns_hash["#{from}_id"].name
hdws        = Hardware.where(column_name => targets).select(select)

This last one, however, still triggers the warning.

I know Brakeman finds detect and find as whitelisting, should something to do with accessing columns_hash be added as an acceptable mitigation technique?

Thanks!

[presidentbeef]

@chrisarcand sorry for the delay.

That Brakeman doesn't warn when using detect is kind of coincidental.

I don't think there is a risk of traditional SQL injection here. I actually made a rare comment on this code. What I was considering was that allowing an attacker to control the columns in a query probably isn't a good thing. Your proposed use of columns_hash doesn't really make a difference in that case.

I am kind of on the fence on this. Still seems a little dangerous, but it's not really SQL injection per se.

[chrisarcand]

I am kind of on the fence on this. Still seems a little dangerous, but it's not really SQL injection per se.

I agree completely and can respect either side of that fence 😕

I guess the only thing that would tip me either way is consistency; using detect or columns_hash essentially does the same sort of [maybe worthless?] filtering, but one is O(n) and one is O(1) and one is a warning while the other is not. That's the part that puzzled me, really. I think I'd consider either warning on both or warning on neither, depending on whether or not it's decidedly dangerous.

Lastly I wonder how common this sort of thing is (dynamically using a different column based on input). If common, perhaps no warnings for either?

In summary: ¯_(ツ)_/¯ :)

[Fryguy]

I can't see how there can be an injection after using the hash. The hash filters to known columns and then on top of that you have AR automatic quoting of fields. Either one alone is sufficient, IMO, and are even stronger when combined.