Hello rubyonrails-core,
I’ve been looking into possible changes to ActiveRecord / Arel to make it easier to write Rails applications that are free of SQL injection vulnerabilities, and in particular do so in a way that makes it easy for a code reviewer to verify that the app is safe from such bugs.
The concern:
With the ActiveRecord API as is, it’s relatively easy to write applications with SQL injection bugs, and those bugs don’t particularly stand out in code reviews. For example,
Customer.where("name like “%#{params[:id]}%”)
While a careful security code review would likely spot this type of bug, there is still a significant risk that it’d be missed, in particular in an agile project with frequent releases that can’t afford to do an in-depth security review between each release.
It is possible to overlook user input in string interpolations - especially if they have been given a temporary variable name, not just params[thing]. But that is one of the things Brakeman is good at detecting already: https://github.com/presidentbeef/brakeman
Hmm, brakeman looks interesting. Having said that, static analysis is a nice security blanket, but it would still be nice to have an enforceable runtime policy that Christoph is alluding to.
This seems to be related also: Redirecting to Google Groups - Christoph, any thoughts?
ig
https://groups.google.com/forum/#!topic/rubyonrails-core/hvfx2MOLnoU - This looks good but there have been instances where some issue or the other vulnerabilities have made their way through SafeBuffer, if I remember correctly. This again could be due to the way the app developer writes the code.
So, Christoph’s approach is ofcourse better, quite detailed and well thought-out. I just feel that it expects some work from the app developers which could make it difficult to adopt.
Just my two cents.
Anuj
Thanks for the comments!
From a brief look, what it appears to be doing is to essentially pattern-match over the parse tree of the app’s source code. However, it does not appear to do any static data flow analysis (except for basic in-lining of variables).
While it does flag,
condition = “name like ‘%#{params[:name]}%’”
@post = Post.where(condition)
it doesn’t flag either
if true
condition = “name like ‘%#{params[:name]}%’”
else
condition = “foo”
end
@post = Post.where(condition)
nor,
@post = Post.where(build_where_clause(params[:name]))
(even if build_where_clause is defined in the same class).
This is not surprising; in a dynamic language like ruby it’s fundamentally impossible to statically derive an accurate control- and data-flow graph from source code.
If I understand correctly, a SafeBuffer is essentially a string that comes with the promise that it’s safe to use in a particular context (in the existing case, HTML).
It seems that this is a useful concept that could be used in conjunction with the approach I’ve proposed, but doesn’t replace it. In a way, the ArelSqlStatement type I’ve proposed (to represent SQL that was obtained by flattening an Arel AST) is essentially a kind of SqlSafeBuffer. I.e., one way of creating a SqlSafeBuffer is to build an Arel AST and flatten it into string. However, I think we would still want to modify Arel itself to ensure that SQL literals are indeed program constants (in order to guarantee that flattened Arel ASTs are indeed safe).
It’s certainly worth considering to add additional ways of creating SqlSafeBuffers, e.g. via string interpolation that automatically quotes values.
If there are other ways of constructing SqlSafeBuffers, it might also make sense to allow Arel leaf nodes of kind SqlSafeBuffer (which would not be quoted during flattening).
That said, I would be concerned about how to ensure that SqlSafeBuffers are in fact constructed safely. It seems nothing would prevent a programmer from writing
"name like ‘%#{param[:name]}%’ ".sql_safe
Of course, this should stick out in code reviews, but relying on that would be just as brittle as relying on a code review catching Foo.where("name like ‘%#{param[:name]}%’ ")
https://groups.google.com/forum/#!topic/rubyonrails-core/hvfx2MOLnoU - This looks good but there have been instances where some issue or the other vulnerabilities have made their way through SafeBuffer, if I remember correctly. This again could be due to the way the app developer writes the code.
So, Christoph’s approach is ofcourse better, quite detailed and well thought-out. I just feel that it expects some work from the app developers which could make it difficult to adopt. Well, it would essentially require that developers either
write their queries as Arel. I suppose this really isn’t extra work; rather it’s a different style of coding (and if I understand correctly, considered the “proper way” of writing queries in Rails 3?)
if they want to use prepared statements, they’d have to write a separate call to register the statement. This is indeed a little bit of extra typing, and does have somewhat of a negative impact on readability.
The benefit for this cost would be a reasonably high degree of confidence that an app is free of SQL injection bugs. Which are very serious bugs (often resulting in complete compromise of an application’s data), and hence many developers should be willing to make that trade off.
cheers,
–xtof