On Tue, Apr 12, 2011 at 2:12 AM, Greg Stein <gstein_at_gmail.com> wrote:
>
> On Apr 11, 2011 10:58 PM, "Hyrum K Wright" <hyrum_at_hyrumwright.org> wrote:
>>
>> On Mon, Apr 11, 2011 at 9:41 PM, Greg Stein <gstein_at_gmail.com> wrote:
>> > Woah. When did svn_sqlite__prepare arrive?
>>
>> $ svnd blame subversion/libsvn_subr/sqlite.c | grep svn_sqlite__prepare
>> 875453 hwright
>> SVN_ERR(svn_sqlite__prepare(&db->prepared_stmts[stmt_idx], db,
>> 873188 gstein svn_sqlite__prepare(svn_sqlite__stmt_t **stmt,
>> svn_sqlite__db_t *db,
>> 875770 hwright SVN_ERR(svn_sqlite__prepare(&stmt, db, "PRAGMA
>> user_version;", scratch_pool));
>>
>> It looks like you introduced (or resurrected it) in r873188.
>
> Really, Hyrum? Is that the argument here? That I can't disagree with this
> function because I'm in the blame list?
I made no such argument. You asked "When did svn_sqlite__prepare
arrive?" and I answered the question using readily available methods.
> You use the term "resurrect" because you've probably see the log for
> r873188. That function came from an earlier implementation. It got
> (re)introduced by bringing that rev back. Then discussions between yourself
> and me determined that we wanted static text, rather than arbitrary
> preparation, so the current (static) system was devised, and we built it.
>
> So yeah: despite the implication that I should be OK with this... no. I'm
> not.
>
>>
>> > I'm basically -1 on that.
>> >
>> > 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.
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".
-Hyrum
Received on 2011-04-12 15:28:21 CEST