[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: Wed, 13 Apr 2011 14:49:12 -0400

On Tue, Apr 12, 2011 at 18:19, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
>...
>> 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.

In order to best answer this, I investigated the current code... and
now understand why you're probably confused about my assertion :-P

The construct_filter() function (which was outside the scope of the
diff) creates N *bind* parameters, rather than direct substitution of
the values. N bindable parameters doesn't have any obvious attack
(other than N=HUGE). I missed the diff after the prepare which shows
the 'changelists' array being bound to the prepared statement.

I'm assuming you already know the attack vector if the values had been
substituted straight into the statement for preparation (ie. class sql
injection), so I won't belabor that point.

So... yes, this particular usage seems safe, after I got a chance to
see the parts outside of the diff.

But I'm still very concerned about svn_sqlite__prepare existing. I
want to get rid of that, to ensure that a future developer doesn't see
it and go "ooh! lemme just strcat some stuff together and pass it to
prepare..." ... more below.

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

Ooh, yes! I definitely like this approach.

How about this, for fair trade: I convert the code to the above, in
penance for raising the red flag? :-P ... I take it you'd be fine with
changing the code to follow the above algorithm? And with that change,
then I can follow up with making prepare() a private function to
svn_sqlite.c.

Cheers,
-g
Received on 2011-04-13 20:49:43 CEST

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