[AppDB] - protect sql insert statements from injection attacks

[Chris Morgan]

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