[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c

From: Greg Stein <gstein_at_gmail.com>
Date: Tue, 12 Apr 2011 03:12:36 -0400

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?

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? 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.

>
> 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".

Cheers,
-g
Received on 2011-04-12 09:13:11 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.