[aur-dev] [PATCH 2/3] SQL: treat all UID/ID values as numbers, not strings

[Lukas Fleischer]

On Tue, Apr 26, 2011 at 10:44:33AM -0500, Dan McGee wrote:
> On Tue, Apr 26, 2011 at 10:04 AM, Lukas Fleischer
> <archlinux at cryptocrack.de> wrote:
> > On Mon, Apr 25, 2011 at 11:23:01PM -0500, Dan McGee wrote:
> >> Ensure we are not quoting these values in any of our SQL queries.
> >>
> >> Thanks-to: elij <elij.mx at gmail.com>
> >> Signed-off-by: Dan McGee <dan at archlinux.org>
> >> ---
> >>
> >> Smoke tested:
> >> * Submitted a package update and it worked
> >> * Updated a user's password from non-salted to salted variety
> >> * Numerous login/logout cycles
> >>
> >>  web/html/passreset.php |    4 ++--
> >>  web/html/pkgsubmit.php |    2 +-
> >>  web/lib/acctfuncs.inc  |   18 +++++++++---------
> >>  web/lib/aur.inc        |    6 +++---
> >>  4 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >
> > Mhh. Basically looks good to me :)
> >
> > Did you check if all affected variables are correctly coerced into
> > integers? If there's any unquoted parameter that is escaped using
> > mysql_real_escape_string() instead of intval() (or something similar),
> > we might become vulnerable to SQL injections, again.
> >
> > We should have used integer conversions in the first place, of course,
> > but this patch will probably turn any mysql_real_escape_string() misuse
> > into an exploitable injection vulnerability.
> 
> I did not; you may want to take a quick peek- I know you've been more
> on the ball with fixing these than me, so you can probably spot the
> code paths real quick. I more just made sure things worked as before.

Skimming through the affected functions, it looks like there shouldn't
be any security regression introduced by this. Pushed that (along with
the other patches this series).