On Apr 12, 2011 9:27 AM, "Hyrum K Wright" <hyrum_at_hyrumwright.org> wrote:
>
> 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.
Fair enough. I read more into your info. Sorry.
>
> > 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.
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.
>
> 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".
I understand, and would suggest an alternative if I had one right now. I
feel the same as you about "not that rock, another".
Cheers,
-g
Received on 2011-04-12 18:26:37 CEST