MantisBT

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005777AContentContentpublic2017-03-24 19:582018-02-01 18:17
Reportergreg 
Assigned Tocindy 
PrioritynormalSeveritymajorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version1.4Fixed in Version1.4 
Summary0005777: $addslashes broken
DescriptionWith the switch toward mysqli, the old mysql_real_escape_string is deprecated and soon to disappear. Because mysqli_real_escape_string and $db->real_escape_string require a DB connection, $addslashes no longer works to escape quotes when text is sent to the DB
Additional InformationThe last commit, where the problem exists.
https://github.com/atutor/AContent/commit/86ba9a3db0bc9cf97e3697eeefa75b62ad216aa2 [^]

$addslashes is used 492 time in AContent, so hoping there a way to fix $addslashes, instead of having to replace them all.
TagsNo tags attached.
Attached Files

- Relationships
related to 0005522closedgreg Update AContent for mysqli 

-  Notes
(0007559)
greg (administrator)
2017-03-24 20:05

using mysql_real_escape_string works for now, but will fail in PHP 7, and it generates a lot of deprecated messages in the log.
(0007560)
cindy (administrator)
2017-04-06 20:55

As we've known, $addslashes used in AContent is an equivalant of mysql_real_escape_string() that is to escape the string data being saved into the mysql databases to prevent the SQL injection.

After doing some research on how to prevent the SQL injection with mysqli, see:
http://stackoverflow.com/questions/2214244/htmlentities-vs-addslashes-vs-mysqli-real-escape-string [^]
https://www.youtube.com/watch?v=etPK7BBp-eQ [^]

the recommended way is to use mysqli_stmt::prepare/mysqli_stmt_prepare to prepare the SQL statement instead of using mysqli_real_escape_string. See http://php.net/manual/en/mysqli-stmt.prepare.php. [^]

In my opinion, the proper way to fix this issue is:
1. Abandon the use of $addslashes;
2. Improve the execute() function in DAO.class.php to use mysqli_stmt::prepare/mysqli_stmt_prepare work flow to guard and execute SQL statements using mysqli type databases, and continue to use mysql_real_escape_string() to guard statements for mysql databases. DAO.class.php is the base SQL statement handler inherited by all other DAO classes. Enhancing it would automatically enhance all DAO classes.

I've searched AContent source code for all spots that $addslashes is used. Most usages are for guarding SQL statements but there are a few locations that $addslashes is misused for verifying form data. Those locations should be carefully re-evaluated to switch to use htmlentities() or the actual addslashes(). Once those locations are fixed, $addslashes can be completed abandoned.

I know this is not a quick fix. It would affect many files. The benefit is it will produce a more secured AContent.

According to the PHP announcement for mysql: mysql extension is deprecated as of PHP 5.5.0, and has been removed as of PHP 7.0.0. See http://php.net/manual/en/intro.mysql.php. [^] Probably it's time to consider to only support:
1. mysqli;
2. PHP 5.5.0 and above
(0007562)
greg (administrator)
2017-04-07 20:20

Thanks Cindy. I was afraid you would say that. Looks there some work to do to fix this.
(0007570)
greg (administrator)
2017-05-26 19:17

Added my_add_null_slashes to DAO.class. Works with this->my_add_null_slashes where integers are being filtered, but not where text is. Looking for a way to leave the addslashes throughout the code, while undoing addslashes with dao->execute and redoing with $db->real_escape_string.

function my_add_null_slashes( $string ) {
        if(defined('MYSQLI_ENABLED')){
            //$string = str_replace(array("\n", "\t", "\r"), '', $string);
            return self::$db->real_escape_string(stripslashes($string));
        }else{
            return mysql_real_escape_string(stripslashes($string));
        }

    }
(0007691)
greg (administrator)
2018-01-31 19:01

$addslashes now completely replaced with mysqli->bind_param
(0007716)
greg (administrator)
2018-02-01 18:17

Resolved in AContent 1.4

- Issue History
Date Modified Username Field Change
2017-03-24 19:58 greg New Issue
2017-03-24 19:58 greg Status new => assigned
2017-03-24 19:58 greg Assigned To => cindy
2017-03-24 20:05 greg Note Added: 0007559
2017-04-06 20:55 cindy Note Added: 0007560
2017-04-07 20:20 greg Note Added: 0007562
2017-05-26 19:17 greg Note Added: 0007570
2017-05-27 09:24 greg Relationship added related to 0005522
2018-01-31 19:01 greg Note Added: 0007691
2018-01-31 19:01 greg Status assigned => resolved
2018-01-31 19:01 greg Fixed in Version => 1.4
2018-01-31 19:01 greg Resolution open => fixed
2018-02-01 18:17 greg Note Added: 0007716
2018-02-01 18:17 greg Status resolved => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker