Re: [AppDB] - protect sql insert statements from injection attacks

[Chris Morgan]

Here is the new version.  I've switched to the fully expanded method of 
writing out the sql.  This is the same format used by several db wrapper 
libraries, in prepared sql statements and is the recommended style under 
c#/.net for queries.

I've tested creating new users and distributions and submitting an 
application.

Chris

On Friday 23 June 2006 1:05 am, Chris Morgan wrote:
> On Thursday 22 June 2006 10:21 pm, Tony Lambregts wrote:
> > Chris Morgan wrote:
> > > Change compile_insert_string() to compile_insert_array() and add a new
> > > first parameter of $sTable that represents the table you are building
> > > the insert statement for.  The output of compile_insert_array() is
> > > passed to query_insert() to perform the insertion.  Change all existing
> > > calls of compile_insert_string() to compile_insert_array() and specify
> > > the table as the first parameter.  Make all instances of sql inserts
> > > that didn't use compile_insert_string() use compile_insert_array().
> > >
> > > The use of mysql_real_escape_string() inside of compile_insert_array()
> > > protects the insert statement from sql injection attacks.
> > >
> > > Chris
> >
> > Just a couple of nits
> >
> > This Could have been broken before but when I try to edit a bundle I
> > get:  *Database Error!*
> > Query: SELECT bundleId, appBundle.appId, appName FROM appBundle,
> > appFamily WHERE bundleId = AND appFamily.appId = appBundle.appId
> > You have an error in your SQL syntax; check the manual that corresponds
> > to your MySQL server version for the right syntax to use near 'AND
> > appFamily.appId = appBundle.appId' at line 1
>
> This is fixed now.  Was broken all the way back to the import into cvs :-)
> > This is probably broken from before (makesafe?) but I cannot create a
> > new distribution.
>
> Yes, makesafe changes broke this.  Fixed now.
> > Here is a big one though. I cannot create a new account. I just get a
> > blank screen. This is .new see below
> >
> > You could have broken the patch in two IE:
> >
> > Change log:  The use of mysql_real_escape_string() on inserts to protect
> > from sql injection attacks.
> >
> > Files changed:
> >     include/application.php /
> >     include/bugs.php /
> >     include/category.php /
> >     include/comment.php /
> >     include/db.php /
> >     include/distributions.php
> >     include/monitor.php /
> >     include/note.php /
> >     include/screenshot.php
> >     include/testResults.php /
> >     include/url.php /
> >     include/user.php /
> >     include/util.php /
> >     include/vendor.php /
> >     include/version.php /
> >
> > Change log: Make all remaining instances of sql inserts use
> > compile_insert_array().
> >
> > Files changed:
> >     maintainersubmit.php /
> >     admin/adminAppDataQueue.php /
> >     admin/editBundle.php /
> >     include/appdb.php /
> >     include/vote.php /
> >
> > I'm not say you should have just that it could have been done that way.
> >
> > You should include the "change log:" and "files changed:" to the patch
> > the files. "Changed files:  really helps in that it gives you an
> > overview of the scope of the patch.
> > > -
> > >
> > > ? compile_insert_array.patch
> > > ? hits_table_alter
> > > ? injection_protect.patch
> > > ? injection_protect2.patch
> > > ? limittestresults.patch4
> > > ? vote_table_alter
> > > ? data/screenshots
> >
> > Last really trivial nit (these are not really part of the patch)
> > > Index: include/user.php
> > >
> > > RCS file: /opt/cvs-commit/appdb/include/user.php,v
> > > retrieving revision 1.67
> > > diff -u -r1.67 user.php
> > > --- include/user.php    21 Jun 2006 01:04:13 -0000    1.67
> > > +++ include/user.php    22 Jun 2006 20:07:17 -0000
> > > @@ -83,14 +83,15 @@
> > >              return false;
> > >          } else
> > >          {
> > > -
> > >        $aInsert = compile_insert_string(array( 'realname' =>
> > >
> > > $sRealname,
> > > -
> > >                                                'email' =>
> > >
> > > $sEmail, -                                                   
> > > 'CVSrelease' => $sWineRelease ));
> > > -
> > >        $aInsert = compile_insert_array("user_list",
> > > -
> > >                                         array( 'realname'   =>
> > >
> > > $sRealname,
> > > -
> > >                                                'email'      =>
> > >
> > > $sEmail,
> > > -
> > >                                                'CVSrelease' =>
> > >
> > > $sWineRelease,
> > > -
> > >                                                'password'   =>
> > >
> > > password($sPassword),
> > > -
> > >                                                'stamp'      =>
> > >
> > > "NOW()",
> > > -
> > >                                                'created'    =>
> > >
> > > "NOW()"));
> >
> > I remember fighting with this before.  password() and NOW() cannot be
> > put into the array like this  I do not have an easy answer for this at
> > all at this point
> > > -
> > >        $sFields = "({$aInsert['FIELDS']}, `password`, `stamp`,
> > >
> > > `created`)";
> > > -
> > >        $sValues = "({$aInsert['VALUES']},
> > >
> > > password('".$sPassword."'), NOW(), NOW() )";
> > > -
> > > -
> > >        query_appdb("INSERT INTO user_list $sFields VALUES
> > >
> > > $sValues", "Error while creating a new user.");
> > > -
> > >        query_insert($aInsert, "Error while creating a new
> > >
> > > user.");
> > >          $retval = $this->login($sEmail, $sPassword);
> > >          $this->setPref("comments:mode", "threaded"); /* set the
> > >
> > > users default comments:mode to threaded */
> > > @@ -183,7 +184,11 @@
> > >              return false;
> > >      $hResult = query_appdb("DELETE FROM user_prefs WHERE userid =
> > >
> > > ".$this->iUserId." AND name = '$sKey'");
> > > -
> > >    $hResult = query_appdb("INSERT INTO user_prefs
> > >
> > > VALUES(".$this->iUserId.", '$sKey', '$sValue')");
> > > -
> > >    $aInsert = compile_insert_array("user_prefs",
> > > -
> > >                                     array( 'userid' =>
> > >
> > > $this->iUserId,
> > > -
> > >                                            'name'   => $sKey,
> > > -
> > >                                            'value'  => $sValue));
> > > -
> > >    $hResult = query_insert($aInsert);
> > >    return $hResult;
> > > }
> >
> > Other than that its clean.  We need to find a way of fixing  new
> > accounts though.
> >
> > --
> >
> > Tony Lambregts
>
> I've got a fix for this.  The basic jist is to simply get rid of
> compile_insert_array() and use a fancier version of query_parameter() that
> uses pear db replacement operators.  There isn't any other way to know
> whether any given variable should be surrounded with '' or not
> escaped(something else that is useful in certain cases apparently).  Should
> have this change in place soon.
>
> Chris