Re: [AppDB] Implement maintainer ratings. - wine-devel

5 Jan 2005

      Hey Tony,
Few comments.
...

  maintainer_rating  text,

  maintainer_release text,

Why text, why not a varchar of 100 characters. It doesn't matter that
much but it just optimizes our code a bit.
...

   $versionName   = addslashes($_REQUEST['versionName']);

   $description   = addslashes($_REQUEST['description']);

   $webPage       = addslashes($_REQUEST['webPage']);

   $versionName        = addslashes($_REQUEST['versionName']);

   $keywords           = $_REQUEST['keywords'];

   $description        = addslashes($_REQUEST['description']);

   $webPage            = addslashes($_REQUEST['webPage']);

   $maintainer_rating  = $_REQUEST['maintainer_rating'];

   $maintainer_release = $_REQUEST['maintainer_release'];

...
     //did anything change?
     if ($VersionChanged)
     {
         $query = "UPDATE appVersion SET versionName = '".$versionName."', ".
             "keywords = '".$_REQUEST['keywords']."', ".
             "description = '".$description."', ".

           "webPage = '".$webPage."'".

           "webPage = '".$webPage."',".

           "maintainer_rating = '".$maintainer_rating."',".

           "maintainer_release = '".$maintainer_release."'".
           " WHERE appId = ".$_REQUEST['appId']." and versionId = ".$_REQUEST['versionId'];
       if (mysql_query($query))
       {

This is far from sql injection safe. Anyone can enter anything into
$_REQUEST['mantainer_release'] or $_REQUEST['mantainer_rating'] and
change any field from the appVersion table. It isn't a real problem in
this case because they can already change most fields as maintainer.
But they can for example change the versionId to make an app-version
belong to any application.
To prevent this we should make our queries with the compile_*_string
functions from include/db.php. That function is doing addslashes. And
generates an as safe as possible query string. Usage can be seen in
admin/editAppFamily.php.
Also query_appdb should be used instead of mysql_query().
And appId and versionId should always be check with is_numeric().
Right now without giving out details it's possible to change any
application as long as you are maintainer of one application.
Will you fix it or shall I?
Paul