MantisBT - AContent
View Issue Details
0005777AContentContentpublic2017-03-24 19:582018-02-01 18:17
greg 
cindy 
normalmajorhave not tried
closedfixed 
 
1.41.4 
0005777: $addslashes broken
With 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
The 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.
No tags attached.
related to 0005522closed greg Update AContent for mysqli 
Issue History
2017-03-24 19:58gregNew Issue
2017-03-24 19:58gregStatusnew => assigned
2017-03-24 19:58gregAssigned To => cindy
2017-03-24 20:05gregNote Added: 0007559
2017-04-06 20:55cindyNote Added: 0007560
2017-04-07 20:20gregNote Added: 0007562
2017-05-26 19:17gregNote Added: 0007570
2017-05-27 09:24gregRelationship addedrelated to 0005522
2018-01-31 19:01gregNote Added: 0007691
2018-01-31 19:01gregStatusassigned => resolved
2018-01-31 19:01gregFixed in Version => 1.4
2018-01-31 19:01gregResolutionopen => fixed
2018-02-01 18:17gregNote Added: 0007716
2018-02-01 18:17gregStatusresolved => closed

Notes
(0007559)
greg   
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   
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   
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   
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   
2018-01-31 19:01   
$addslashes now completely replaced with mysqli->bind_param
(0007716)
greg   
2018-02-01 18:17   
Resolved in AContent 1.4