WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#8767 closed defect (bug) (fixed)

Refactored filters to avoid potential XSS attacks

Reported by: sambauers Owned by: ryan
Milestone: 2.7.1 Priority: high
Severity: major Version: 2.7
Component: Security Keywords: has-patch, needs-testing, XSS
Focuses: Cc:

Description

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+

Attachments (3)

filters.patch (9.3 KB) - added by sambauers 7 years ago.
filters-speedup.patch (2.4 KB) - added by sambauers 6 years ago.
filters-compat-fixes.patch (3.6 KB) - added by sambauers 6 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 follow-up: @miqrogroove7 years ago

+			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.

comment:2 in reply to: ↑ 1 @sambauers7 years ago

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.

comment:3 follow-up: @miqrogroove7 years ago

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.

comment:4 in reply to: ↑ 3 @sambauers7 years ago

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.

comment:5 @sambauers7 years ago

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.

@sambauers7 years ago

comment:6 @link927 years ago

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.

comment:8 @azaozz7 years ago

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

comment:9 follow-up: @azaozz7 years ago

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.

comment:10 in reply to: ↑ 9 @sambauers7 years ago

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!

comment:11 @azaozz7 years ago

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

comment:12 @link927 years ago

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.

comment:13 follow-up: @sambauers7 years ago

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.

comment:14 in reply to: ↑ 13 ; follow-ups: @link926 years ago

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.

comment:15 in reply to: ↑ 14 @sambauers6 years ago

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.

comment:16 in reply to: ↑ 14 ; follow-up: @sambauers6 years ago

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.

comment:17 in reply to: ↑ 16 @link926 years ago

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.

@sambauers6 years ago

comment:18 @azaozz6 years ago

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

comment:19 @sambauers6 years ago

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.

comment:20 @azaozz6 years ago

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

comment:21 @ryan6 years ago

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

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

Note: See TracTickets for help on using tickets.