theme-editor.php broken: stripslashes() and add_magic_quotes() screw up CR LFs

[astounding]

Hi,

I'm running wordpress 2.0.4 (installed this past weekend) under PHP 5 (and configured so that get_magic_quotes_gpc() returns zero--PHP's magic quotes are the spawn of the devil in my opinion and have caused more trouble for PHP users... I digress...) When I tried editing theme files with the theme editor (theme-editor.php), as soon as the edit was submitted, the file was totally screwed up. Where before there were newlines in the file, now the character pairs "rn" appeared (no slashes).

I traced it down to thus:

theme-editor.php requires admin.php which requires wp-config.php which in turn includes wp-settings.php

In wp-settings.php, the _POST array gets bulk-quoted with this line:

$_POST = add_magic_quotes($_POST );

Later on in theme-editor.php, there's this line:

$newcontent = stripslashes($_POSTnewcontent?);

These two things screw things up together. The first converts all CR LF pairs to "\r\n" (backslashes followed by "r" and "n"). The next strips out the slashes. The end result: All newlines get converted to "rn" which is meaningless to web browsers, web servers, etc. Do it to an important theme file and watch your wordpress site be useless.

So... What's the fix? Hasn't this issue turned up for other (all?) users? Or if PHP's built-in magic quotes are ON, does the process of removing 'em and adding 'em back in the wp-settings.php file NOT convert CR LF characters? It's a biggie.

My current work-around is a preg_replace() in theme-editor.php to convert "\r" and "\n" back to the actual CR and LF characters just before stripslashes() gets called. This seems to me to be an ugly kludge to the real problem. But maybe a true design fix would require far too much work, potentially introducing loads more bugs...

Thanks!

[ryan]

Magic quotes and stripslashes don't affect CR LF since they are 0x0d and 0x0a, not \r \n. Even if you type in \r and \n, the theme editor will preserve the slashes. Everything works fine for me, and I haven't heard any other reports of this. What web server are you running and on what OS?

[astounding]

My gut instinct was that CR LF should NOT be affected either, but actual real-world experience demonstrates that the call in wp-settings.php to add_magic_quotes() is guilty of converting CRs to "\r" and LFs to "\n". I was surprised.

I'm running FreeBSD 6.1-RELEASE, Apache 2.0.59 with mod_php version 5.1.4 (all from the FreeBSD ports collection).

[astounding]

Resolved!

Further tracking shows that add_magic_quotes() calls the database's quote() which in my case is PHP's mysqli's escape function. And of course the database's escape function will convert CRs and LFs to the "\r" and "\n" sequences.

If the data's being pre-quoted to be safe for database insertion, why later strip slashes? That's brain-dead poor design. Better to keep the data "tainted" and untrusted in the original user-supplied form up until the time it actually needs to be inserted, and then quote it there. As long as all modules and components match this behavior (and consider all user-supplied data tainted), you still get security, without weird oddities that almost always creep in when

Oh, and NOT using the database's quoting mechanism (as in the current mysql db module where the database's escape functions are commented out) does open up the threat that in some future database upgrade, the non-database-provided escaping function will fail to escape something critical, opening one up to SQL injection vulnerabilities.

Since the database-provide escaping function does things that other sections of code and/or modules do NOT desire (i.e. in this case converting CRs and LFs), some well-meaning WordPress coder, rather than undertaking the arduous and immense task of fixing a fundamental design problem (And I agree, I'm not going to undertake that task either!--WAAAAY to big a job for the little time available...) the coder just commented out the database escaping code and substituted PHP's addslashes() (which is NOT guaranteed to always be free of potential SQL-injection vulnerabilities, esp. as new database types are added in the future).

[astounding]

Oh, here's a patch that really ought to be used. Since wp-db.php currently just uses addslashes() for doing escaping, and since add_magic_quotes() in functions.php really doesn't need anything more, just move it directly to add_magic_quotes().

This opens the door to more database types for wp-db.php without breaking how things currently work AS LONG AS before actual database calls, the true database escaping code is called (and I haven't examined the code to be sure of this). But if this is not the case, there are potential issues already in existence.

This patch does NOT change things (security-wise) in any way, since addslashes() is all that gets called via the wp-db.php escape() function call. The above was just speculating about the future.

--- wp-includes/functions.php.orig Tue Aug 15 19:53:36 2006 +++ wp-includes/functions.php Tue Aug 15 19:54:22 2006 @@ -2148,7 +2148,7 @@

if ( is_array($v) ) {

$array[$k] = add_magic_quotes($v);

} else {

• $array[$k] = $wpdb->escape($v);

+ $array[$k] = addslashes($v);

}

} return $array;

@@ -2503,4 +2503,4 @@

die();

}

-?> \ No newline at end of file +?>

[astounding]

(Oh, great, a bug tracking system that does NOT work with inline patches... *sigh* Oh well, I suppose I'm spoiled by better systems elsewhere.)

[war59312]

Just attach it as a simple .diff file ...