Fatal error with invalid charset specified in Trackback

[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

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>

[dd32]

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.

[jrf]

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

[verygoode]

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!

[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)?
@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.

[dd32]

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.

[rajinsharwar]

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:

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

[dd32]

@rajinsharwar Do you have the mbstring extension installed?

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

[rajinsharwar]

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

[oglekler]

@dd32 and @audrasjb This looks like a straight forward patch, but we are getting to the RC1, and I am suggesting to decide what to do with this one. Thank you!

[dd32]

As we're already in Beta, punting this is acceptable. This is not critical, and is an issue that already exists in released versions.

[oglekler]

I am moving this ticket to the next milestone to handle without hurry. Thanks everyone 🙏