WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 10 months ago

#39865 new defect (bug)

Escaping functions have filters that allow them to be bypassed

Reported by: welcher Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Formatting Keywords: 2nd-opinion close
Focuses: Cc:

Description

The esc_* family of function all contain filters that pass the unescaped content as the second parameter. This seems to defeat the purpose of the functions as all it would take would be to add a filter similar to the following to completely bypass the escaping features.

add_filter( 'esc_html', 'bypass_escaping', 10, 2 );

function bypass_escaping( $safe_text, $text ) {
     return $text;
}

// Call this in any template
echo esc_html( '<script>alert(\'Haxxed\')</script>' );

My suggestion would be to deprecate the second parameter and if needed add a filter to for just the $text is that is called before the escaping in the internals of the functions.

Change History (2)

#1 follow-up: @dd32
10 months ago

  • Keywords 2nd-opinion close added

I really feel this is by-design and should NOT be changed. We shouldn't pretend that WordPress operates in a clean sandboxed mode where code can only change what it is expected to.

Everything is filterable in WordPress, using filters within escaping functions allows for enhancing escaping functions where needed, but they also make it easier to selectively undo it for certain edge-cases when needed - knowing the input text is required there. If people need to use the parameter, they're going to use it even if deprecated, otherwise the only option would be a hacky workaround.

If malicious code wanted to output non-escaped test in a location where esc_html() was used, there'd be numerous ways it could achieve it - the simplest would be to output the content upon one of the many filters which is probably called within what is going into esc_html().

#2 in reply to: ↑ 1 @welcher
10 months ago

Replying to dd32:

I really feel this is by-design and should NOT be changed. We shouldn't pretend that WordPress operates in a clean sandboxed mode where code can only change what it is expected to.

Everything is filterable in WordPress, using filters within escaping functions allows for enhancing escaping functions where needed, but they also make it easier to selectively undo it for certain edge-cases when needed - knowing the input text is required there. If people need to use the parameter, they're going to use it even if deprecated, otherwise the only option would be a hacky workaround.

While I agree with the concept of filtering all the things, in this case, I feel that we're compromising site security for that ideal. If there is a use case where some needs to selectively undo the escaping, then perhaps not using the function is a better choice than filtering it away. Is there a use-case for this in core?

If malicious code wanted to output non-escaped test in a location where esc_html() was used, there'd be numerous ways it could achieve it - the simplest would be to output the content upon one of the many filters which is probably called within what is going into esc_html().

I cannot find any other filters being called in the internals of esc_html() but I see your point.

Note: See TracTickets for help on using tickets.