Thanks for the comments!
* Re: brakeman. Looks like an interesting and useful too, however it
appears that it only can only find relatively specific patterns of bugs,
and therefore doesn't provide particularly high confidence that an app is
free of SQL injection (or other classes of bugs).
>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.
* Re: SafeBuffer:
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]}%' ")
On Mon, Nov 28, 2011 at 23:49, Anuj Dutta <[email protected]> wrote:
> 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
>
> Just my two cents.
>
> Anuj
>
>
>
> On 28 November 2011 20:16, Ilya Grigorik <[email protected]> wrote:
>
>> 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:
>> https://groups.google.com/forum/#!topic/rubyonrails-core/hvfx2MOLnoU -
>> Christoph, any thoughts?
>>
>> ig
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Ruby on Rails: Core" group.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msg/rubyonrails-core/-/Xiw70fs5eo0J.
>>
>> To post to this group, send email to [email protected].
>> To unsubscribe from this group, send email to
>> [email protected].
>> For more options, visit this group at
>> http://groups.google.com/group/rubyonrails-core?hl=en.
>>
>
>
>
> --
> Anuj DUTTA
>
> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Core" group.
> To post to this group, send email to [email protected].
> To unsubscribe from this group, send email to
> [email protected].
> For more options, visit this group at
> http://groups.google.com/group/rubyonrails-core?hl=en.
>
--
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Core" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
