problem with $_REQUEST & magic_quotes_gpc in keywordmanager

[Andi]

Hello

when  magic_quotes_gpc  is OFF, then by changing a Keyword with a Quote ( ' ) included, an error occurs:

While executing query "UPDATE cpg140_pictures SET `keywords` = 'huhu'huuuuuuu huhu'huuuuuuu' WHERE `pid` = '2'" on 0
mySQL error: 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 'huuuuuuu huhu'huuuuuuu' WHERE `pid` = '2'' at line 1

File: D:\Localhost\dev-Coppermine\devel\include\functions.inc.php - Line: 238

[offtopic]ok, it's crazy to do this, i know  [/offtopic]

The causer is this line in keywordmgr.php:

Code Select Expand

$newquerys[] = "UPDATE {$CONFIG['TABLE_PICTURES']} SET `keywords` = '$keywords' WHERE `pid` = '$id'";
There is no function to delete or escape the Quotes or other types.


By analyzing this Problem, i see that the global variable $_REQUEST is in use in several files. But $_REQUEST is not checked if magic_quotes_gpc is ON or OFF in init.inc.php like $_POST or $_GET. Maybe this causes other or future problems?

We has the same problem in our vkpMx and do this, after check and fix this setting:

Code Select Expand

$_REQUEST = array_merge($_GET,$_POST,$_COOKIE);

[donnoman]

I encountered a similar problem in a plugin I was doing, I ended up doing this: str_replace("'","''",$_POST['title'])

Code Select Expand


   $query = "UPDATE {$CONFIG['TABLE_CMS']} SET ID = '".$_POST['id']."', catid='".$_POST['catid']."', title = '".str_replace("'","''",$_POST['title'])."', content = '".str_replace("'","''",$_POST['content'])."'  WHERE ID = ".$_POST['id'] ;


I also did something similar in CPG DEVEL for data being brought in via IPTC fields

Code Select Expand


               foreach ($IPTC_data as $key=>$data) {
                  $IPTC_data[$key] = str_replace("'","''",$data); //sanitize data against sql injection
               }


I think this is such a common thing we are going to need to protect against I advocate building a function in functions.inc.php that does basic sql injection sanitizing.

Maybe making it something like how I did for bad word filtering

Code Select Expand


function filter_content($str)
{
   global $lang_bad_words, $CONFIG, $ercp;
   if ($CONFIG['filter_bad_words']) {
       static $ercp=array();
       if (!count($ercp)) foreach($lang_bad_words as $word) {
           $ercp[] = '/' . ($word[0] == '*' ? '': '\b') . str_replace('*', '', $word) . ($word[(strlen($word)-1)] == '*' ? '': '\b') . '/i';
       }
       if (is_array($str)) {
           $new_str=array();
           foreach ($str as $key=>$element) {
               $new_str[$key]=filter_content($element);
           }
           $str=$new_str;
       } else {
       $stripped_str = strip_tags($str);
       $str = preg_replace($ercp, '(...)', $stripped_str);
       }
   }
return $str;
}


I suppose it kind of makes sense to perhaps combine the quote replacement IN filter_content, though we'll have to make sure the sql injection stuff is always run, whereas the bad word filtering is only active based on the config setting. Of course it also strips html tags, which may not be wanted across the board.

[donnoman]

another problem and I just noticed it in my plugin code is that NONE of the other arguements are sanitized in any way shape or form.  They can be subverted as easily as the strings can.

The rest of coppermine does a better job of casting numeric data as (int)$_POST['cat'] or whatever. Obviously I need to go address that in the plugin.

[Joachim Müller]

You're right in proposing a general method of sanitizing $_REQUEST vars, so yes: please create a function that will accomplish what you're suggesting. As this will be a core function, maybe some other devs might want to look into this as well. For now I suggest you create a function in functions.inc.php and use it with your plugin (as a first step) and ask other devs to look into it. We could then gradually change the exiting code to use the sanitizing method.

Joachim

[Tarique Sani]

some sanitization of $_GET $_POST and cookie data is already being done in init.inc.php may be you would want to make that more robust

[donnoman]

I've found a pretty good article http://education.nyphp.org/phundamentals/PH_storingretrieving.php

They recommend disabling magic_quotes_gpc then fixing all the magic_quotes if they exist (which we do in init.inc.php)

Secondly they recommend:
Use mysql_escape_string if you are using a version of PHP prior to ver. 4.3.
Use mysql_real_escape_string if you are using PHP ver. 4.3 or better.
When writing to the database.  

Thirdly keep the data in as close to raw format as you can while your processing it.

Then use an approrpriate function when you go to display the data; ie: you would use htmlentities if displaying to html, but you could output raw to a csv, just by adding whatever is needed for csv.

If you use htmlentities too soon, then you have to decode when you go to use that data for something OTHER than displaying in html.  (it looks like we do a subset of htmlentities in the santization process in init.inc.php)

Also our current sanitation process doesn't remove single quotes from the stream but we do stripslashes, leaving us open to sql injection attacks unless we make sure we are doing some kind of sanitation before writing to the database. (from what I've seen the code in coppermine seems to do this pretty well)

FYI: php.net docs say both of those mysql_escape functions are for 4.3 or greater

also in my tests mysql_escape_string is none-to-bright as it will escape things that are already escaped causing duplicate slashes... and your back where you started.

If we do ANYTHING in init.inc.php we will break a lot of code.

ie: if we double up the single quotes, later in the code when we do an addslash, we will get \'\' written to the database, which isn't what we want.

The moral of the story is education and consistency. Anytime we use get post or cookie variables either directly or indirectly we have to assume the single quotes are unescaped, and cast the var as an int (int)$_GET['cat'] or use addslashes($_GET['title']) for text fields.

I saw a semi-standard going through coppermines code. If we could formulate it as a rule I think that would help. (if it already wasn't)

Never use the gpc globals directly in an sql statement.

take this example from db_input.php

Code Select Expand


$aid = (int)$_POST['aid'];
       $title = addslashes(trim($_POST['title']));
       $category = (int)$_POST['category'];
       $description = addslashes(trim($_POST['description']));
               $keyword = addslashes(trim($_POST['keyword']));
       $thumb = (int)$_POST['thumb'];
       $visibility = (int)$_POST['visibility'];
       $uploads = $_POST['uploads'] == 'YES' ? 'YES' : 'NO';
       $comments = $_POST['comments'] == 'YES' ? 'YES' : 'NO';
       $votes = $_POST['votes'] == 'YES' ? 'YES' : 'NO';
       $password = $_POST['alb_password'];
               $password_hint = addslashes(trim($_POST['alb_password_hint']));
       $visibility = !empty($password) ? FIRST_USER_CAT + USER_ID : $visibility;

       if (!$title) cpg_die(ERROR, $lang_db_input_php['alb_need_title'], __FILE__, __LINE__);

       if (GALLERY_ADMIN_MODE) {
           $query = "UPDATE {$CONFIG['TABLE_ALBUMS']} SET title='$title', description='$description', category='$category', thumb='$thumb', uploads='$uploads', comments='$comments', votes='$votes', visibility='$visibility', alb_password='$password', alb_password_hint='$password_hint', keyword='$keyword' WHERE aid='$aid' LIMIT 1";
       } else {
           $category = FIRST_USER_CAT + USER_ID;
           $query = "UPDATE {$CONFIG['TABLE_ALBUMS']} SET title='$title', description='$description', thumb='$thumb',  comments='$comments', votes='$votes', visibility='$visibility', alb_password='$password', alb_password_hint='$password_hint',keyword='$keyword' WHERE aid='$aid' AND category='$category' LIMIT 1";
       }


Make the variables pretty (sanitized) then and only then use them in an sql query. 

[Tarique Sani]

@donoman - We are expereinced enough to know what you are talking about BUT the bigger moral of the the story with a project as big as CPG is that do not mess with thing that are working for now.  For now fix only what is broken. In this case and in every case

[donnoman]

Sorry Tarique I thought I was pretty clear, for the most part its done right. Obviously we have a problem in keywordmanager and what I was saying is we should handle it like we did in db_input instead of creating additional changes in init.inc.php or making another function to handle any sanitization since it's already done.

I think were saying the same thing different ways, it just took me longer to get there.

[Tarique Sani]

I think were saying the same thing different ways, it just took me longer to get there.

If you are talking of changes in just the keywordmanager then yes I miss understood you - sorry - go ahead do as you are proposing.

When I had proposed that you look into init.inc.php I had meant in context of what Gaugau was suggesting

[Joachim Müller]

well, anyway it won't hurt to remind all devs from time to time that there are such things as xss and sql injection we have to be aware of, so this general approach of donnoman's posting is welcome imo and should be taken into account (or rather: discussed in detail) for cpgNG. I think it's a big plus for coppermine that no serious vulnerability has been discovered in the past, because of the strong code base it is built on: the desastrous side-effects a widely-exploited security flaw can have on the reputation of a software project mustn't be under-estimated (e.g. the Santy worm for phpBB or the exploits for YaBB 1 Gold that ruined YaBB SE's reputation).
Back to the original subject: I agree with Tarique - let's fix keywordmanager first to get cpg1.4 rolling.

Joachim

[donnoman]

I added sanitizing the $_REQUEST global to init.inc.php

Code Select Expand


$HTML_SUBST = array('&' => '&amp;', '"' => '&quot;', '<' => '&lt;', '>' => '&gt;');
if (get_magic_quotes_gpc()) {
   if (is_array($_POST)) {
       foreach ($_POST as $key => $value) {
           if (!is_array($value))
               $_POST[$key] = strtr(stripslashes($value), $HTML_SUBST);
           if (isset($$key)) unset($$key);
       }
   }

   if (is_array($_GET)) {
       foreach ($_GET as $key => $value) {
           $_GET[$key] = strtr(stripslashes($value), $HTML_SUBST);
           if (isset($$key)) unset($$key);
       }
   }

   if (is_array($_COOKIE)) {
       foreach ($_COOKIE as $key => $value) {
           if (!is_array($value))
               $_COOKIE[$key] = stripslashes($value);
           if (isset($$key)) unset($$key);
       }
   }
   if (is_array($_REQUEST)) {
       foreach ($_REQUEST as $key => $value) {
           if (!is_array($value))
               $_REQUEST[$key] = stripslashes($value);
           if (isset($$key)) unset($$key);
       }
   }
} else {
   if (is_array($_POST)) {
       foreach ($_POST as $key => $value) {
           if (!is_array($value))
               $_POST[$key] = strtr($value, $HTML_SUBST);
           if (isset($$key)) unset($$key);
       }
   }

   if (is_array($_GET)) {
       foreach ($_GET as $key => $value) {
           $_GET[$key] = strtr($value, $HTML_SUBST);
           if (isset($$key)) unset($$key);
       }
   }

   if (is_array($_COOKIE)) {
       foreach ($_COOKIE as $key => $value) {
           if (isset($$key)) unset($$key);
       }
   }
   if (is_array($_REQUEST)) {
       foreach ($_REQUEST as $key => $value) {
           if (!is_array($value))
               $_REQUEST[$key] = strtr($value, $HTML_SUBST);
           if (isset($$key)) unset($$key);
       }
   }
}


I added a few instances of addslashes to the $_REQUEST vars in keywordmgr.php

I also found and fixed an error in keywordmgr.php that was deleting ALL of the keywords on a pic, if it had THE keyword that needed to be deleted. Now only the specific keyword is removed out of the pics keyword list.

Couple addslashes added to delete.php

All changes comitted, can someone confirm that this is fixed.