Refactored filters to avoid potential XSS attacks

[sambauers]

Attached patch introduces new and refactored functions for filtering input. They mostly work as additional defence against invalid UTF8 XSS attacks in IE6.

New wp_specialchars() is optimised for PHP 5.2.3+

[miqrogroove]

+			if ( 'single' === $quote_style ) {
+				$string = str_replace( '"', '"', $string );
+			}
+		}
+		$string = htmlspecialchars( $string, $_quote_style, $charset );
+	}

This part of your patch would convert every double quote in $string to " which could look pretty ugly in form fields.

Also it seems unrealistic to propose such drastic changes for 2.7.1 given its due date is less than two weeks.

[sambauers]

Replying to miqrogroove:

This part of your patch would convert every double quote in $string to " which could look pretty ugly in form fields.

It wouldn't do that because of the $_quote_style that would be set. But it was wrong in another way, so thanks, fixed now.

Also it seems unrealistic to propose such drastic changes for 2.7.1 given its due date is less than two weeks.

The vulnerability being addressed could form the basis of a zero-day XSS exploit, and has been in the past on other platforms.

[miqrogroove]

I'm also concerned about the return ; statements. This is not typical of UTF-8 sanitizers.

With only a week to go now, I think this patch is more likely to introduce new problems than improve risk factors.

[sambauers]

Replying to miqrogroove:

I'm also concerned about the return ; statements. This is not typical of UTF-8 sanitizers.

If wp_check_invalid_utf8() encounters bad UTF8 the default behaviour is to return an empty string. It can also attempt to strip the bad chars if desired, but the default is more secure. Bad UTF8 chars in a UTF8 poor browser (like IE6) can do very unpredictable things, so blanking the string is the best approach.

In that sense it is less like a sanitiser and more like a validator.

[sambauers]

I've refactored the patch based on problems which emerged from the bbPress implementation of the same functions.

Mainly removes wp_entities() completely due to charset issues in PHP 4.

[link92]

wp_check_invalid_utf8() uses PCRE to check UTF-8 validity.

Of note:

• PCRE before 4.5 (by default PHP < 4.3.5) didn't check for overlong sequences.
• PCRE before 7.3 (by default PHP4 < 4.4.9; PHP5 < 5.2.5) allowed six byte sequences.

As WP supports back to PHP 4.3.0, if you want to check for UTF-8 validity, it won't do.

[link92]

​http://applesoup.googlepages.com/bypass_filter.txt describes the issues with some UAs.

[azaozz]

(In [10297]) Refactor filters to avoid potential XSS attacks, props sambauers and DD32, see #8767

[azaozz]

Lets test this for a few days in trunk. Also DD32 found an infinite loop with get_option( 'blog_charset' ) calling back wp_specialchars(), changed it to get the option directly.

[sambauers]

Replying to azaozz:

Lets test this for a few days in trunk.

You somehow used the old patch, not the one I re-uploaded yesterday!

[azaozz]

(In [10298]) Latest version of the patch for refactor filters to avoid potential XSS attacks, props sambauers and DD32, see #8767

[link92]

Also, there are plenty of copies of PHP with a PCRE that doesn't have Unicode support enabled at all. This has been a fair issue for Habari (which requires PHP 5.2.x), I imagine it'll be a lot worse for something that requires PHP 4.3.0.

[sambauers]

Replying to link92:

Also, there are plenty of copies of PHP with a PCRE that doesn't have Unicode support enabled at all. This has been a fair issue for Habari (which requires PHP 5.2.x), I imagine it'll be a lot worse for something that requires PHP 4.3.0.

I would think that we just need examples of how these fail so we can do it gracefully instead. I doubt we would build alternatives for them though.

[link92]

Replying to sambauers:

Replying to link92:

Also, there are plenty of copies of PHP with a PCRE that doesn't have Unicode support enabled at all. This has been a fair issue for Habari (which requires PHP 5.2.x), I imagine it'll be a lot worse for something that requires PHP 4.3.0.

I would think that we just need examples of how these fail so we can do it gracefully instead. I doubt we would build alternatives for them though.

Off the top of my head, any PCRE just silently fails to match anything, which without iconv would make this patch a huge void hole.

[sambauers]

Replying to link92:

Off the top of my head, any PCRE just silently fails to match anything, which without iconv would make this patch a huge void hole.

Yes, for those cases, but there is still a majority of cases where this will enhance security.

[sambauers]

Replying to link92:

Off the top of my head, any PCRE just silently fails to match anything, which without iconv would make this patch a huge void hole.

Sorry, I misread your last comment.

Yes, PCRE fails silently when it encounters bad UTF8. The wp_check_invalid_utf8() function exploits that to "detect" the bad UTF8. So if the preg_match() passes, it's not a bad string and it is returned. There is a "strip" option which will attempt to use iconv() but it is not utilised in any of the changes in this patch.

From the patch...

        // preg_match fails when it encounters invalid UTF8 in $string
        if ( 1 === @preg_match( '@^.@us', $string ) ) {
                return $string;
        }

Yes, if the installed PCRE doesn't support UTF8 then it will fail and the string will be blanked.

We already require PCRE with UTF8 in at least one other place, but I could potentially detect for it's absence and return the string unscathed.

Something like...

        // Check that PCRE handles UTF8 - if not they are on their own
        if ( !@preg_match( '@^.@u', 'utf-ate nom nom nom' ) ) {
                return $string;
        }

... then the test.

[link92]

Replying to sambauers:

Replying to link92:

Off the top of my head, any PCRE just silently fails to match anything, which without iconv would make this patch a huge void hole.

Sorry, I misread your last comment.

Yes, PCRE fails silently when it encounters bad UTF8. The wp_check_invalid_utf8() function exploits that to "detect" the bad UTF8. So if the preg_match() passes, it's not a bad string and it is returned. There is a "strip" option which will attempt to use iconv() but it is not utilised in any of the changes in this patch.

From the patch...

        // preg_match fails when it encounters invalid UTF8 in $string
        if ( 1 === @preg_match( '@^.@us', $string ) ) {
                return $string;
        }

Yeah, sure, but see link92 for shortcomings with even this.

Yes, if the installed PCRE doesn't support UTF8 then it will fail and the string will be blanked.

We already require PCRE with UTF8 in at least one other place, but I could potentially detect for it's absence and return the string unscathed.

Something like...

        // Check that PCRE handles UTF8 - if not they are on their own
        if ( !@preg_match( '@^.@u', 'utf-ate nom nom nom' ) ) {
                return $string;
        }

... then the test.

Ah, I'm probably just confused because according to ​http://wordpress.org/about/requirements/ WP doesn't even require PCRE, yet alone PCRE-with-Unicode-support (and there are plenty of copies of PCRE-without-Unicode-support out there). Either the patch needs changing, or the requirements need to be changed. I would not be surprised if requiring PCRE-with-Unicode cut out a larger number of users than requiring PHP 5 would cut out.

[azaozz]

(In [10355]) Speed up wp_specialchars, props sambauers, see #8767

[sambauers]

Attached patch fixes some issues where the right quote style was not being used. Probably fixes a bunch of issues with kses doing weird things to quoted attributes in post text.

[azaozz]

(In [10376]) Fix incorrect quote style in wp_specialchars, props sambauers, see #8767

[ryan]

(In [10391]) Refactor filters to avoid potential XSS attacks, props sambauers and DD32. fixes #8767 for 2.7