On Tue, Apr 12, 2011 at 11:26 AM, Greg Stein <gstein_at_gmail.com> wrote:
>...
>> >> > The whole idea behind static statements was to avoid SQL injection
>> >> > attacks.
>> >> > Allowing the *code* to construct statements opens us up.
>> >> >
>> >> > This is Not Good.
>> >>
>> >> This is still a prepared statement, arguments are still bound using
>> >> the standard SQLite binding mechanism, so they will be properly
>> >> escaped.
>> >
>> > Arguments, yes. But this edits the statement itself, outside of binding.
>> >
>> >> Yes, one can put arbitrary text in the
>> >> statement-to-be-prepared, but the preparation step helps prevent
>> >> injection attacks. (You can't prepare multiple statements, which
>> >> vastly decreases the attack space.)
>> >
>> > No. You may be referring to *this* statement. What about the next one
>> > which
>> > is an INSERT or UPDATE? Slippery slope that I disagree with. We spoke
>> > about
>> > this, and I haven't changed my mind. Static text only.
>> >
>> >>
>> >> Plus, we're talking about local data here. While sql injection is a
>> >> Bad Thing, I suspect somebody wanting to hose a working copy will do
>> >> something much simpler: rm .svn/wc.db. :)
>> >
>> > Thanks for the strawman.
>> >
>> > Are you sure that *nothing* will cause an injection? Trojans? Bad server
>> > input? Inappropriate clicks in a UI?
>>
>> Bugs in the sqlite library? A compromised compiler? A corporate
>> lackey intentionally sabotaging their compiled Subversion client? ...
>>
>> > The static text design was to disable
>> > all such attacks. I did't even realize we had left the prepare function
>> > in
>> > there, since we had designed all callers to avoid it.
>>
>> You know as well as I do that we can not possibly prevent all attacks.
>> We do our best through review and testing. Even in a manually
>> prepared statement, binding all parameters removes the possibility of
>> allowing non-escaped values into the query, just as with static text.
>
> Yes, but the statement itself can be attacked if you manually build it. With
> static text, that is not possible. Attackers cannot alter the semantics in
> any way.
I guess I'm just dense, because I can't come up with a scenario in
which a dynamic statement is more prone to injection than a static
one, as long as both escape their arguments correctly. Dynamic
queries are more complex, it is true, so I don't recommend using them
liberally, but I'm not sure banning them outright is the answer.
If you're saying that a string on the heap or stack is more likely to
be attacked than a string in the data section of the program, I tend
to agree, but the same argument could be made for any of the plethora
of dynamic strings we construct throughout Subversion. It isn't
unique to sqlite queries.
>> We still need to review all queries, even the static ones in
>> wc-queries.sql. Those can fall victim to programmer error just as
>> well as dynamically generated queries. (And since all external input
>> is parameterized and bound, both are protected from injection
>> attacks.)
>>
>> >> On a higher-level, if we *don't* use this API, what are your
>> >> suggestions on how to create a IN clause with arbitrary numbers of
>> >> values?
>> >
>> > Not sure. Maybe we can work through some ideas. But "we have no other
>> > choice" is not a good enough reason to keep this. That is an even worse
>> > slope to slide down. Doing things simply because they are "convenient".
>>
>> The general consensus seems to be to revert the whole lot of these
>> changes, and I'll bow to those wishes and comply.
>>
>> On a more general note, this is the second time in as many weeks I've
>> read "don't do that" in reviews. It's a good learning experience to
>> know that others don't consider an approach feasible. It would be
>> much more productive, however, if alternate ideas were presented
>> instead of playing "bring me a rock".
>
> I understand, and would suggest an alternative if I had one right now. I
> feel the same as you about "not that rock, another".
While riding my bike this afternoon, I came up with an alternate
proposal. The *common* case is one or zero changelists, so we could
just have two queries, one for each of those scenarios. In the
uncommon case that the changelist filter contains multiple
changelists, we'd just iterate over them using the single changelist
query. I think this approach is generalizable to other operations
which use changelist filtering.
(I'm happy to implement the above on practical grounds, but would
still like to better understand the claimed attack vectors for dynamic
queries.)
-Hyrum
Received on 2011-04-13 00:20:00 CEST