WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#24941 closed defect (bug) (invalid)

esc_sql in 3.6 changes how /r /n and maybe other characters handled

Reported by: sc0ttkclark Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.6
Component: Formatting Keywords:
Focuses: Cc:

Description

In Pods, we've been using esc_sql (maybe wrongly, after reading #21767) in 3.x, in conjunction with stripslashes_deep, when working with $_POST data.

By default, WP slashes all $_POST data on load, so we come back around and set a scoped var with the $_POST data, stripslashes_deep on the array, and use the values in the code. Once ready to save, in certain areas we've been using esc_sql.

Now I'm not yet sure what's going on well enough to fix it on my side for 3.6+, but it appears that this change 3 weeks ago may have a big part to do with it: 24718

It seems now at some point in 3.6, \r and \n become just "r" and "n".

We've been sanitizing data before it hits wp_update_post and other areas it looks like, so there's bound to be issues on our side here that we'll need to resolve now.

I'm posting this ticket at the request of @markjaquith who wanted to better understand what we're dealing with right now. I'll post updates as I determine what fixes I needed to make in case it helps other plugin developers who come across this issue.

If you read anything that would appear as a WP bug in this ticket, feel free to jump in and help figure out what needs to be done, otherwise you can close this as invalid if it ends up being a pilot error.

Code to reproduce:

$content = "Testing it out\n\nTest";

$postdata = array(
    'ID' => $post_ID,
    'post_content' => esc_sql( $content )
);

/*
   WP 3.x:
   $postdata[ 'post_content' ] = "Testing it out\n\nTest";

   WP 3.6:
   $postdata[ 'post_content' ] = "Testing it out\\n\\nTest";
*/

wp_update_post( $postdata );

Change History (14)

comment:1 sc0ttkclark9 months ago

OK so looks like our wp_update_post usage was unnecessarily sanitizing, removed that. However, the original code to reproduce shows the main difference between esc_sql's functionality pre 3.6 and now in 3.6 -- which is causing the other issues across our codebase. I'm digging into those areas to see where things go south.

comment:2 nofearinc9 months ago

Scott, just tested a sample use case in lastest 3.6:


  $content = "Testing it out\n\nTest";
  $content = esc_sql( $content );

There is no extra escaping there, i.e. the new content is exactly "Testing it out\n\nTest". Can you still confirm the issue on your end?

comment:3 sc0ttkclark9 months ago

$content = "Testing it out\n\nTest";
var_dump( $content );
var_dump( esc_sql( $content ) );
var_dump( addslashes( $content ) );

Outputs:

string(20) "Testing it out

Test"
string(22) "Testing it out\n\nTest"
string(20) "Testing it out

Test"
Last edited 9 months ago by SergeyBiryukov (previous) (diff)

comment:4 nofearinc9 months ago

I see now, thanks - wasn't clear from the code above. So why have you been using esc_sql for the post_content?

comment:5 sc0ttkclark9 months ago

$content = "Testing's it outs yo\r\nyahhhhh\r\n\"Yes\" `yeohoho` !@po`j-9j-``!#~!~Testing it out\n\nTest";
var_dump( $content );
var_dump( esc_sql( $content ) );
var_dump( addslashes( $content ) );

Outputs:

string(84) "Testing's it outs yo
yahhhhh
"Yes" `yeohoho` !@po`j-9j-``!#~!~Testing it out

Test"
string(93) "Testing\'s it outs yo\r\nyahhhhh\r\n\"Yes\" `yeohoho` !@po`j-9j-``!#~!~Testing it out\n\nTest"
string(87) "Testing\'s it outs yo
yahhhhh
\"Yes\" `yeohoho` !@po`j-9j-``!#~!~Testing it out

Test"
Last edited 9 months ago by SergeyBiryukov (previous) (diff)

comment:6 sc0ttkclark9 months ago

I removed the esc_sql from the three wp_update_post calls in the code. Not sure why it was using it there, this is just in a side part of Pods itself, a few of the components had been doing that. Core of Pods doesn't do this for wp_*_post.

There are still issues with the data elsewhere, I'm tracking those down right now.

Last edited 9 months ago by sc0ttkclark (previous) (diff)

comment:7 sc0ttkclark9 months ago

Thx for the edits @SergeyBiryukov

comment:8 nofearinc9 months ago

Maybe the core ninjas could give a reason for esc_sql to be used there, but I can't find any.

I see your point of testing with addslashes (even if we have functions like esc_html), just FYI if you think that esc_sql is using addslashes - it does only if no SQL connection is available in the $wpdb object when the esc_sql call is executed. Or moreover, mysql_real_escape_string is being called instead which could escape the things differently (in SQL it makes sense to parse input in a different way to prevent injections).

http://core.trac.wordpress.org/browser/trunk/wp-includes/wp-db.php#L879

comment:9 sc0ttkclark9 months ago

@nofearinc This was comparing WP 3.x to 3.6 -- 3.6 is _doing_it_right but 3.x had been using addslashes all along:

3.x: esc_sql >> wpdb::escape >> wpdb::_weak_escape >> addslashes

3.6: esc_sql >> wpdb::_escape >> wpdb::_real_escape >> mysql_real_escape_string (if db connection active), addslashes (plus _doing_it_wrong if no connection)

Last edited 9 months ago by sc0ttkclark (previous) (diff)

comment:10 nofearinc9 months ago

@sc0ttkclark, I understand your concern and I still have no idea how esc_sql would be useful in this scenario. The post insert/update process is reusing the $wpdb->insert() function which is calling a prepared statement, escaping eventual dangerous SQL code.

The only possible issue I see is with injecting markup/JavaScript when not needed, but that is not related to the esc_sql.

comment:11 sc0ttkclark9 months ago

Sorry for the miscommunication, I stated above that it was *our* mistake that we used esc_sql in a few wp_update_post calls we made, which weren't part of the main Pods core package (they were part of a few components we included). The esc_sql change continues to affect other areas of our code right now though, which is what I'm looking into next.

comment:12 sc0ttkclark9 months ago

I think what might be happening here is we were sanitizing certain arrays at first, then in certain areas down the line in functions, it would unsanitize the data using stripslashes_deep, which with the 3.6 change in slashing for \r \n and other characters, means that it will egregiously remove those backslashes incorrectly.

Deeper issue for use to solve, but I think that's the primary issue right now.

comment:13 sc0ttkclark9 months ago

  • Resolution set to invalid
  • Status changed from new to closed

OK, going to close this as invalid, looks like we've got a lot of work to get Pods sanitization back under control for the 3.6 slashing changes. Some of this stuff just "worked" because it was legacy and it was the way it was, so it ended up spreading across the codebase. We were able to slash and unslash data willy nilly because esc_sql mapped to addslashes, but there's no quick fix for this because stripslashes_deep isn't a proper anti-mysql_real_escape_string in 3.6

comment:14 helen9 months ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.