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