Index: include/user.php

23 Jun 2006

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

    

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

Index: include/user.php