[AppDB] - protect sql insert statements from injection attacks

[Tony Lambregts]

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 probably broken from before (makesafe?) but I cannot create a 
new distribution.

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