On Wed, Apr 13, 2011 at 1:49 PM, Greg Stein <gstein_at_gmail.com> wrote:
> 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 still think that's painting with quite a broad brush, but since
we've found an alternative solution for this (and similar cases), I'm
happy to let it go. We've got version control, we can always
resurrect that API if needed.
>>...
>>> 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.
Sounds great!
Thanks,
-Hyrum
Received on 2011-04-15 00:11:10 CEST