Page MenuHomePhabricator

Message -> string transformations should not affect each other
Closed, ResolvedPublic

Description

$msg = new Message( 'foo' );
echo $msg; // escaped
echo $msg->plain();
echo $msg; // not escaped

Given how hard it is to track when messages are stringified via __toString, this makes security review for XSS holes hard and should be killed. If something relies on setting the formatting type upstream from the place of outputting, that should be done in a more explicit way.

Event Timeline

core patch:


config patch:
This sets up a log channel for all Message::__toString calls that are affected by a previous transformation. I am hoping that nothing actually relies on that behavior and it can be removed.

I am hoping that nothing actually relies on that behavior and it can be removed.

I'm pretty sure stuff relies on it.

Patch itself looks sane. It'd be nicer if this weren't having to be done secretly though, is there an actual security vulnerability exposed here or is it extremely theoretical "something somewhere might be affected by this, maybe"?

This sounds good to me. However, I'd like to make sure that the language folks have no objections to this given it is the Message class.

I agree with @Anomie about it being better to do this publicly.

Tgr changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 22 2016, 9:34 PM
Tgr changed Security from Software security bug to None.

Change 312401 had a related patch set uploaded (by Gergő Tisza):
Log when Message::__toString has an unexpected format

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

Change 312404 had a related patch set uploaded (by Gergő Tisza):
Add 'message-format' log channel

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

However, I'd like to make sure that the language folks have no objections to this given it is the Message class.

Please describe what is this.

Our documentation has always instructed to use explicit format method instead of relying on the string conversion. It is unlikely that the same message instance would be output multiple times.

Moreover, message related security issues are already rare, thanks to T85864: Special pages, actions and views whose messages don't escape text.

Change 312401 merged by jenkins-bot:
Log when Message::__toString has an unexpected format

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

Change 312404 merged by jenkins-bot:
Add 'message-format' log channel

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

No hit in three weeks, looks like nothing uses it at least on WMF sites.

Change 320548 had a related patch set uploaded (by Gergő Tisza):
Revert "Add 'message-format' log channel"

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

Change 320548 abandoned by Gergő Tisza:
Revert "Add 'message-format' log channel"

Reason:
On second thought we can keep this for now.

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

Change 320550 had a related patch set uploaded (by Gergő Tisza):
Always use parse in Message::__toString()

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

Change 320550 merged by jenkins-bot:
Deprecate Message::$format (mostly)

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

Change 320966 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
More robust, cleaned up MessageTest

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

Change 320966 merged by jenkins-bot:
More robust, cleaned up MessageTest

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

Nemo_bis triaged this task as Medium priority.Nov 12 2016, 4:41 PM
Tgr moved this task from Backlog to Pending on the User-Tgr board.

Still need to remove the deprecated methods.

Krinkle added a subscriber: Krinkle.

This seems important to get rid of. Proposing for 1.35 release. @Tgr How much would the remaining work be? Can you or someone in your team take this on this or next quarter?

Zero logs in Logstash so we can just kill it. Hopefully won't cause much disruption for third parties; ->toString() is such a super generic call that it is impossible to search for it.
We'd have to skip the hard-deprecation step though, which I apparently forgot to do :/

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

This clearly isn't going to be done in the next day or so. Re-tagging to the 1.36 release.

Hey there, should this be moved to 1.37? The cut for 1.36 has happened, and 1.36.0-rc.0 will be cut in a fortnight or so, after which feature changes shouldn't be landed and back-ported.

I suggest we ship hard-deprecation for this in 1.36 now that we still can, otherwise we may have to left this anti-security compat issue linger for another year.

Change 681824 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/Wikibase@master] tests: Remove use of $msg->toString() in SetClaimTest

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

Change 681818 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] Message: Deprecate toString() with an implicit format

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

Change 681824 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] tests: Remove use of $msg->toString() in SetClaimTest

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

Change 681818 merged by jenkins-bot:

[mediawiki/core@master] Message: Deprecate toString() with an implicit format

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

Change 681740 had a related patch set uploaded (by Jforrester; author: Krinkle):

[mediawiki/extensions/Wikibase@REL1_36] tests: Remove use of $msg->toString() in SetClaimTest

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

Change 681741 had a related patch set uploaded (by Jforrester; author: Krinkle):

[mediawiki/core@REL1_36] Message: Deprecate toString() with an implicit format

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

Change 681740 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_36] tests: Remove use of $msg->toString() in SetClaimTest

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

Change 681741 merged by jenkins-bot:

[mediawiki/core@REL1_36] Message: Deprecate toString() with an implicit format

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

Change 700219 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Message: Removed deprecated format property

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

Change 700219 merged by jenkins-bot:

[mediawiki/core@master] Message: Remove deprecated format property

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

Jdforrester-WMF assigned this task to Tgr.

Change 721313 had a related patch set uploaded (by Daimona Eaytoy; author: Gergő Tisza):

[mediawiki/core@wmf/1.37.0-wmf.21] Message: Remove deprecated format property

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

Change 721313 merged by jenkins-bot:

[mediawiki/core@wmf/1.37.0-wmf.21] Message: Remove deprecated format property

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

Mentioned in SAL (#wikimedia-operations) [2021-09-16T10:59:34Z] <hashar@deploy1002> Synchronized php-1.37.0-wmf.21/includes/language/Message.php: Message: Remove deprecated format property - T146416 T291124 (duration: 01m 06s)