Project

General

Profile

Bug #2970

We have code that depends on the setting magic_quotes_gpc

Added by Ivan Tcholakov over 8 years ago. Updated about 8 years ago.

Status:
Bug resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
27/02/2011
Due date:
% Done:

100%

Estimated time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

My configuration: Windows Vista, XAMPP 1.7.4 (PHP 5.3.5)

The PHP settings magic_quotes_* are deprecated and in the future probably they will be removed at all. In my configuration they are "Off" by default.

In rhis regard I discovered a problem in the "Forums" tool (Task #1297), the quotes in the entered html code were escaped. The same behavior exists in the "Documents" tool.

How to reproduce:

1. Make sure that in your configuration the settings magic_quotes_* are "Off", see the picture 4.
2. Create a new document using some template and save it (picture 1).
3. Preview the document, you will see that it is not shown correctly (picture 2).
4. See the source of the document in the browser - it would be broken, quotes are escaped (picture 3).
5. Set in your php.ini the setting magic_quotes_gpc to be "On", restart the web server (Apache).
6. Create a new document and preview it again. It should be OK (picture 5).
7. Restore in php.ini the setting magic_quotes_gpc as it was "Off".

In my opinion, we should avoid the dependency of the setting magic_quotes_gpc. The solution is using the function stripslashes() where it is applicable. At the moment I see that this is needed for the "Documents" tool.


Files

1_new_document_from_a_template.png (105 KB) 1_new_document_from_a_template.png Ivan Tcholakov, 27/02/2011 14:33
2_document_preview_no_frames.png (26 KB) 2_document_preview_no_frames.png Ivan Tcholakov, 27/02/2011 14:33
3_document_source.png (49.9 KB) 3_document_source.png Ivan Tcholakov, 27/02/2011 14:33
4_magic_quotes_settings.png (31.3 KB) 4_magic_quotes_settings.png Ivan Tcholakov, 27/02/2011 14:33
5_magic_quotes_gpc_on.png (86.2 KB) 5_magic_quotes_gpc_on.png Ivan Tcholakov, 27/02/2011 14:33
quote.png (74 KB) quote.png Julio Montoya, 26/04/2011 14:08

Associated revisions

Revision 2c2c3a4d (diff)
Added by Julio Montoya about 8 years ago

Adding stripslashes to the $_REQUEST, $_GET, $_POST and $_COOKIE when magic_quotes_gpc is on see #2970

Revision d9040a1b (diff)
Added by Yannick Warnier about 8 years ago

Added reference to task - refs #2970

History

#1

Updated by Yannick Warnier over 8 years ago

  • Target version changed from 1.8.8 beta to 1.8.8 stable

Agreed. Not really a super urgent matter, I think. I'm leaving it set to 1.8.8, but it is likely to be postponed if nobody can have a look at it until then.

#2

Updated by Julio Montoya over 8 years ago

I can't reproduce step 3:

magic_quotes_* are "Off"
php version 5.3.3-1
SO: Ubuntu Maverick

#3

Updated by Julio Montoya over 8 years ago

  • Target version changed from 1.8.8 stable to 1.8.8.4

Moving to 1.8.8.1

#4

Updated by Ivan Tcholakov over 8 years ago

Agreed.

Meanwhile I've changed my mind about the solution. Instead of adding stripslashes() everywhere, there is a correct way. For example, CMS Made Simple 1.9.4.1 in its file include.php (on the root directory) has the following code:

#Stupid magic quotes...
if(get_magic_quotes_gpc())
{
    stripslashes_deep($_GET);
    stripslashes_deep($_POST);
    stripslashes_deep($_REQUEST);
    stripslashes_deep($_COOKIE);
    stripslashes_deep($_SESSION);
}

Note about the last statement stripslashes_deep($_SESSION): I don't think that $_SESSION is affected by the magic quotes, I wonder why this statement has been put.

Here is the function stripslashes_deep(), file page.functions.php:

/**
 * Strips slashes from an array of values.
 *
 * @internal
 * @param array A reference to an array of strings
 * @return reference to the cleaned values
 */
function & stripslashes_deep(&$value) 
{ 
        if (is_array($value)) 
        { 
                $value = array_map('stripslashes_deep', $value); 
        } 
        elseif (!empty($value) && is_string($value)) 
        { 
                $value = stripslashes($value); 
        } 
        return $value;
}

Similar code may be added in Chamilo, in the global initialization file.

#5

Updated by Ivan Tcholakov over 8 years ago

A similar solution from phpMyAdmin:

// remove quotes added by php
// (get_magic_quotes_gpc() is deprecated in PHP 5.3, but compare with 5.2.99
// to be able to test with 5.3.0-dev)
if (function_exists('get_magic_quotes_gpc') && -1 == version_compare(PHP_VERSION, '5.2.99') && get_magic_quotes_gpc()) {
    PMA_arrayWalkRecursive($_GET, 'stripslashes', true);
    PMA_arrayWalkRecursive($_POST, 'stripslashes', true);
    PMA_arrayWalkRecursive($_COOKIE, 'stripslashes', true);
    PMA_arrayWalkRecursive($_REQUEST, 'stripslashes', true);
}
/**
 * calls $function vor every element in $array recursively
 *
 * this function is protected against deep recursion attack CVE-2006-1549,
 * 1000 seems to be more than enough
 *
 * @see http://www.php-security.org/MOPB/MOPB-02-2007.html
 * @see http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-1549
 *
 * @uses    PMA_arrayWalkRecursive()
 * @uses    is_array()
 * @uses    is_string()
 * @param   array   $array      array to walk
 * @param   string  $function   function to call for every array element
 */
function PMA_arrayWalkRecursive(&$array, $function, $apply_to_keys_also = false)
{
    static $recursive_counter = 0;
    if (++$recursive_counter > 1000) {
        die('possible deep recursion attack');
    }
    foreach ($array as $key => $value) {
        if (is_array($value)) {
            PMA_arrayWalkRecursive($array[$key], $function, $apply_to_keys_also);
        } else {
            $array[$key] = $function($value);
        }

        if ($apply_to_keys_also && is_string($key)) {
            $new_key = $function($key);
            if ($new_key != $key) {
                $array[$new_key] = $array[$key];
                unset($array[$key]);
            }
        }
    }
    $recursive_counter--;
}
#6

Updated by Julio Montoya about 8 years ago

  • Status changed from New to Needs more info
  • % Done changed from 0 to 80
#7

Updated by Yannick Warnier about 8 years ago

  • Status changed from Needs more info to Bug resolved
  • % Done changed from 80 to 100

OK. Will have to keep an eye on this just in case we get strange reports in the chamilo forum.

Also available in: Atom PDF