Wait, was T179080 never fixed? Kind of sounds a little like that. But im not sure, very likely could be wrong. think that is unrelated

So I'm not very familiar with LQT, so i might be overreacting, but to me that exception looks like what would happen if there was some sort of deserialization vuln happening

If possible, it would help to know what the subject line of any LQT threads from that page were, if they were suspicious [possibly the suspicious subject lines were just being previewed and not saved]

It is unclear to me what is causing a code injection if any? My guess is one can craft a title in a LiquidThread topic that causes a PHP file in the include path to be autoloaded (and thus potentially allowing arbitrary code execution)?

As noted by Niklas, the is_callable( [ $value, '__toString' ] ) in \Wikimedia\Message\ScalarParam constructor comes from 9d56257d8c15.

In T377912#10253191, @hashar wrote:

It is unclear to me what is causing a code injection if any? My guess is one can craft a title in a LiquidThread topic that causes a PHP file in the include path to be autoloaded (and thus potentially allowing arbitrary code execution)?

Agreed – though I’m not sure if this can actually cause arbitrary code execution. Also, my guess would be that this affects a lot more than LQT, as it seems to be triggered by a bog-standard Message::plaintextParams() call.

Possible fix?

diff --git a/includes/libs/Message/ScalarParam.php b/includes/libs/Message/ScalarParam.php
index 729746f500..467ef2855a 100644
--- a/includes/libs/Message/ScalarParam.php
+++ b/includes/libs/Message/ScalarParam.php
@@ -42,7 +42,7 @@ public function __construct( $type, $value ) {
 			// Ensure that $this->value is JSON-serializable, even if $value is not
 			// (but don't do it when using ParamType::OBJECT, since those objects may not expect it)
 			$value = MessageValue::newFromSpecifier( $value );
-		} elseif ( $value instanceof Stringable || is_callable( [ $value, '__toString' ] ) ) {
+		} elseif ( is_object( $value ) && ( $value instanceof Stringable || is_callable( [ $value, '__toString' ] ) ) ) {
 			// TODO: Remove separate '__toString' check above once we drop PHP 7.4
 			$value = (string)$value;
 		} elseif ( !is_string( $value ) && !is_numeric( $value ) ) {

(I.e. avoid the is_callable() check if the arg is a string.)

In T377912#10253219, @Lucas_Werkmeister_WMDE wrote:

In T377912#10253191, @hashar wrote:

It is unclear to me what is causing a code injection if any? My guess is one can craft a title in a LiquidThread topic that causes a PHP file in the include path to be autoloaded (and thus potentially allowing arbitrary code execution)?

Agreed – though I’m not sure if this can actually cause arbitrary code execution. Also, my guess would be that this affects a lot more than LQT, as it seems to be triggered by a bog-standard Message::plaintextParams() call.

Possible fix?

diff --git a/includes/libs/Message/ScalarParam.php b/includes/libs/Message/ScalarParam.php
index 729746f500..467ef2855a 100644
--- a/includes/libs/Message/ScalarParam.php
+++ b/includes/libs/Message/ScalarParam.php
@@ -42,7 +42,7 @@ public function __construct( $type, $value ) {
 			// Ensure that $this->value is JSON-serializable, even if $value is not
 			// (but don't do it when using ParamType::OBJECT, since those objects may not expect it)
 			$value = MessageValue::newFromSpecifier( $value );
-		} elseif ( $value instanceof Stringable || is_callable( [ $value, '__toString' ] ) ) {
+		} elseif ( is_object( $value ) && ( $value instanceof Stringable || is_callable( [ $value, '__toString' ] ) ) ) {
 			// TODO: Remove separate '__toString' check above once we drop PHP 7.4
 			$value = (string)$value;
 		} elseif ( !is_string( $value ) && !is_numeric( $value ) ) {

(I.e. avoid the is_callable() check if the arg is a string.)

I agree that seems like a reasonable fix.

Proper patch with a test, if anyone wants to review:

(I’m not sure if the test class in that file actually needs to be called *TestCase or if I’m hallucinating the structure test that insists on *Test(Case).php names in tests/phpunit/, but we can always change that later once it’s on Gerrit and we actually have CI.)

Amended patch to make phpcs happy (just added a line break in the condition to bring it below 120 chars again):

Note: a handful of tests in tests/phpunit/unit/includes/libs/Message/ are skipped under PHP 8.2, so if anyone with a local PHP 7.4 / 8.1 install could run those with the patch, that would be great ^^

From security perspective, I guess you can load any file in includes ending in .php, since that is what the MediaWiki namespace is mapped to (and who knows what else in extensions and vendor). Still it seems hard to come up with anything evil.

The best i can think of is \\MediaWiki\\DevelopmentSettings which will disable rate limits and set some debug settings which might leak some info (however some of these settings might be set too late to take effect)

FWIW I've deployed a local patch on translatewiki to remove the is_callable given the comment about it only being there for 7.4 support. We are already on 8.x on translatewiki.

In T377912#10253403, @Bawolff wrote:

From security perspective, I guess you can load any file in includes ending in .php, since that is what the MediaWiki namespace is mapped to (and who knows what else in extensions and vendor). Still it seems hard to come up with anything evil.

The best i can think of is \\MediaWiki\\DevelopmentSettings which will disable rate limits and set some debug settings which might leak some info (however some of these settings might be set too late to take effect)

FWIW, I can confirm that locally, without my patch checked out, loading index.php?title=MediaWiki\DevelopmentSettings is enough to load DevelopmentSettings.php (and cause lots of warnings). At least some of the settings are indeed no longer taking effect by this point (index.php?title=MediaWiki\DevelopmentSettings&uselang=x-xss does not result in the “XSS” language code from T340201 being used, probably because the relevant service was passed a copy of $wgUseXssLanguage long before DevelopmentSettings.php was loaded and changed it to true), but I wouldn’t want to bet that this is true for all of them.

We make Composer dump a classmap in production so it won't autoload files that weren't supposed to be autoloadable. (Files that were supposed to be autoloadable would presumably have no side effect.) Extensions don't typically have side effects, either. The MediaWiki autoloader will load everything under includes but none of the files that weren't supposed to be autoloaded look truly dangerous at a glance. DefaultSettings & co operate in the current scope, not the global one, so they can't be used to mess with configuration settings; DevelopmentSettings just relaxes throttles + sets $wgShowExceptionDetails but it's too late for that to take effect. I guess you could first load DevelopmentSettings.php then Setup.php to reinstall the exception handler, but Setup.php is huge and I'm sure something would break.

I'm moving this to MW Eng Group radar. Please, let me know if I'm missing something or our support is required.

Well, it would be great if someone™ could review the patch from T377912#10253394…

In T377912#10254155, @Lucas_Werkmeister_WMDE wrote:

Well, it would be great if someone™ could review the patch from T377912#10253394…

+2. Thanks for attaching a test case!

Patch looks good to me. Sorry about that.

I wonder if this is something that could be caught by Phan taint check.

In T377912#10254358, @matmarex wrote:

I wonder if this is something that could be caught by Phan taint check.

Interesting… yeah, maybe it should check for tainted calls to is_callable (first arg) and class_exists.

In T377912#10253394, @Lucas_Werkmeister_WMDE wrote:

0001-SECURITY-Message-Don-t-call-is_callable-on-strings.patch4 KBDownload

I just deployed this patch. Given that the issue was only introduced in the last train branch, and translatewiki already deployed a different fix (T377912#10253407), I think it would be fine to upload it on Gerrit pretty soon?

So, this is mostly my fault – while I knew how is_callable works in regards to autoloading, and I knew about similar issues caused by unserializing user input, I just never connected these two things in my mind as a potential security issue to look out for. I'm not sure if it is common knowledge.

However, I think it's also worth noting that this wouldn't have occurred if we didn't have to write code for PHP 7.4. In this case the is_callable could be replaced by an instanceof check (per the code comment), but more generally, the first class callable syntax introduced in PHP 8.1 also allows replacing is_callable( [ $value, '__toString' ] ) with is_callable( $value->__toString( ... ) ), which does not work (and thus does not trigger the autoloading) when $value is a string.

In T377912#10254577, @Lucas_Werkmeister_WMDE wrote:

I just deployed this patch. Given that the issue was only introduced in the last train branch, and translatewiki already deployed a different fix (T377912#10253407), I think it would be fine to upload it on Gerrit pretty soon?

Thanks. Given that this is a core patch, the default would be to hold it for the next security release (T375621). But for things like this that only briefly existed due to a bad commit, we've sometimes backported them sooner. Thoughts, @Reedy?

It has to be backported soon if we want to avoid getting it into the release candidate for 1.44, right?

rc.0 is in theory about a week away - https://m.mediawiki.org/wiki/Version_lifecycle

Just FYI - In a separate conversation w/ @Reedy, he and I were both fine with backporting this now.

In T377912#10253549, @Tgr wrote:

We make Composer dump a classmap in production so it won't autoload files that weren't supposed to be autoloadable. (Files that were supposed to be autoloadable would presumably have no side effect.) Extensions don't typically have side effects, either. The MediaWiki autoloader will load everything under includes but none of the files that weren't supposed to be autoloaded look truly dangerous at a glance. DefaultSettings & co operate in the current scope, not the global one, so they can't be used to mess with configuration settings; DevelopmentSettings just relaxes throttles + sets $wgShowExceptionDetails but it's too late for that to take effect. I guess you could first load DevelopmentSettings.php then Setup.php to reinstall the exception handler, but Setup.php is huge and I'm sure something would break.

Worth noting that in T378006 we verified that the dumped classmap in production still contains directory entries, so any file in those directories can potentially be loaded.

@Reedy @sbassett Are you planning to submit Lucas's patch to Gerrit yourselves, or waiting for someone else to do it?

In T377912#10260298, @matmarex wrote:

@Reedy @sbassett Are you planning to submit Lucas's patch to Gerrit yourselves, or waiting for someone else to do it?

Oh, figured Lucas might, but I can go ahead and post it now.

Change #1082849 had a related patch set uploaded (by SBassett; author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] SECURITY: Message: Don’t call is_callable() on strings

https://gerrit.wikimedia.org/r/1082849

Sorry, I thought it’s usually the security team who uploads those patches. +2ed.

Change #1082849 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Message: Don’t call is_callable() on strings

https://gerrit.wikimedia.org/r/1082849

In T377912#10255836, @Reedy wrote:

rc.0 is in theory about a week away - https://m.mediawiki.org/wiki/Version_lifecycle

So where do we need to backport before then? wmf/next?

In T377912#10261934, @Tgr wrote:

So where do we need to backport before then? wmf/next?

Do we need to? It's patched in Wikimedia production right now per Lucas' deploy and the master patch is merged in gerrit. So it should get branched as part of 1.43.0-wmf.29 next week, and then the security patch can fall off from the deployment hosts.

In T377912#10264019, @sbassett wrote:

Do we need to? It's patched in Wikimedia production right now per Lucas' deploy and the master patch is merged in gerrit. So it should get branched as part of 1.43.0-wmf.29 next week, and then the security patch can fall off from the deployment hosts.

Next week is 1.44.0-wmf.1 though. We still need to get the fix into the 1.43 release. I suppose just backporting to 1.43.0-wmf.28 should do the trick?

In T377912#10265135, @Tgr wrote:

We still need to get the fix into the 1.43 release. I suppose just backporting to 1.43.0-wmf.28 should do the trick?

REL1_43

Change #1083368 had a related patch set uploaded (by Bartosz Dziewoński; author: Lucas Werkmeister (WMDE)):

[mediawiki/core@REL1_43] SECURITY: Message: Don’t call is_callable() on strings

https://gerrit.wikimedia.org/r/1083368

Change #1083368 merged by jenkins-bot:

[mediawiki/core@REL1_43] SECURITY: Message: Don’t call is_callable() on strings

https://gerrit.wikimedia.org/r/1083368

I think that's all. Thanks everyone. This task could be made public now.

This has been marked as a subtask of T375622 "Tracking bug for MediaWiki 1.39.11/1.41.5/1.42.4", but the bug does not affect any of those versions, just 1.43.

The only affected WMF branch is 1.43.0-wmf.28, which has a private security patch applied, so a backport there is not needed either (but it could be done for clarity/transparency if anyone feels like doing it on Monday).

In T377912#10265232, @matmarex wrote:

This has been marked as a subtask of T375622 "Tracking bug for MediaWiki 1.39.11/1.41.5/1.42.4", but the bug does not affect any of those versions, just 1.43.

Yeah, it can probably be removed as a sub-task unless @Reedy wants to mention it in the next security release as a footnote or something.

Thank you everyone for the close collaboration while addressing this task \o/