Make WordPress Core

Opened 5 months ago

Last modified 11 days ago

#60261 new defect (bug)

Fatal error with invalid charset specified in Trackback

Reported by: dd32's profile dd32 Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Pings/Trackbacks Keywords: has-patch php81 needs-testing
Focuses: Cc:

Description (last modified by dd32)

wp-trackback.php accepts a charset parameter, but doesn't validate that the site supports it.

Code is effectively this:
https://core.trac.wordpress.org/browser/trunk/src/wp-trackback.php?marks=54-76#L53

$charset   = isset( $_POST['charset'] ) ? $_POST['charset'] : '';
$title     = isset( $_POST['title'] ) ? wp_unslash( $_POST['title'] ) : '';
$title     = mb_convert_encoding( $title, get_option( 'blog_charset' ), $charset );

charset should be a charset that's commonly accepted, such as UTF-8, but it could also be specified as foobar.

This would cause a PHP Warning in PHP 7.4:

E_WARNING: mb_convert_encoding(): Illegal character encoding specified in wp-trackback.php:76

and in PHP 8.0:

Fatal error: Uncaught ValueError: mb_convert_encoding(): Argument #3 ($from_encoding) contains invalid encoding "FOOBAR" in wp-trackback.php on line 76

Change History (12)

This ticket was mentioned in PR #5872 on WordPress/wordpress-develop by @dd32.


5 months ago
#1

  • Keywords has-patch added

Trac ticket: https://core.trac.wordpress.org/ticket/60261

Before

$ curl https://example.org/wp-trackback.php --data 'charset=foobar&title=foo' -i
HTTP/1.1 404 Not Found
...
<b>Fatal error</b>: Uncaught Error: mb_convert_encoding(): Argument #3 ($from_encoding) contains invalid encoding &quot;FOOBAR&quot;

After

$ curl https://example.org/wp-trackback.php --data 'charset=foobar&title=foo' -i
HTTP/1.1 404 Not Found
....
<message>I really need an ID for this to work.</message>

#2 @dd32
5 months ago

  • Keywords needs-unit-tests added

#3 @dd32
3 months ago

  • Keywords php81 added; needs-unit-tests removed
  • Milestone changed from Awaiting Review to 6.6

I can't really see any good way to add unit tests to this, without rewriting the way wp-trackbacks.php works, which I think is a bit too much of a step over this change.

#4 @verygoode
3 months ago

  • Focuses php-compatibility added

#5 follow-up: @jrf
3 months ago

  • Focuses php-compatibility removed

@verygoode The PHPCompatibility focus should not be used for small bug fixes like this.

#6 in reply to: ↑ 5 @verygoode
3 months ago

Replying to jrf:

@verygoode The PHPCompatibility focus should not be used for small bug fixes like this.

Sorry about that @jrf and I actually meant to apply it to another issue. Thanks!

#7 follow-up: @oglekler
2 weeks ago

  • Keywords needs-testing-info added

What it the difference between incorrect encoding and obsolete UTF-7 that will cause die() (I wonder if it should use wp_die() instead)?
@dd32, Can you, please, provide testing instructions?

Possibly for deciding how to test this function, we need another ticket. In normal project I would have been refactored it and moved charset verification to another function (private method is not an option here) but if we do this, it will become globally available and we don't need this.

#8 in reply to: ↑ 7 @dd32
2 weeks ago

  • Keywords needs-testing-info removed

Replying to oglekler:

What it the difference between incorrect encoding and obsolete UTF-7 that will cause die() (I wonder if it should use wp_die() instead)?

UTF-7 die()'ing is more of a security thing, It's obsoleted in that it was never an official standard and has known security vulnerabilities that were never going to be fixed.

Die'ing here for "invalid" charsets is fine, such as foobar but that's not necessarily wanted here.
There's also the case of 'unsupported' charsets, A receiver site might not support the sent charset, we probably still want to process the request in that case (Which is what happens today).

Can you, please, provide testing instructions?

The best testing instructions are the first comment on this trac ticket, listing curl commands.

It's worth noting, that AFAIK, this charset parameter isn't actually in the Trackback specification, and that it's supposed to be sent as part of the Content-Type header which WordPress ignores.

#9 @oglekler
2 weeks ago

  • Keywords needs-testing added

#10 @rajinsharwar
2 weeks ago

I have tried to check this in PHP 8.3, but it seems that without the patch applied, the Fatal error doesn't occur for me.

On Trunk: https://img001.prntscr.com/file/img001/2_n7rdjOTxCyCZL85R7LLw.png

It's the same in PHP 8.1.23 as well. Anything I am missing @dd32?

#11 @dd32
13 days ago

  • Description modified (diff)

@rajinsharwar Do you have the mbstring extension installed?

Testing with https://3v4l.org/pQk0K confirms that this is a PHP8 fatal.

#12 @rajinsharwar
11 days ago

@dd32 Yeah I do have mbstring enabled: https://img001.prntscr.com/file/img001/K2OSh7OaQ7Sw63U3-7zlIg.png

Maybe I'm missing something on my end, I will check on that.

Note: See TracTickets for help on using tickets.