| Tgr | |
| Sep 22 2016, 8:28 PM |
| F4514344: 0001-SECURITY-Add-message-format-log-channel.patch | |
| Sep 22 2016, 8:48 PM |
| F4514331: 0001-SECURITY-Log-when-Message-__toString-has-an-unexpect.patch | |
| Sep 22 2016, 8:48 PM |
$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.
core patch:
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.
Change 312401 had a related patch set uploaded (by Gergő Tisza):
Log when Message::__toString has an unexpected format
Change 312404 had a related patch set uploaded (by Gergő Tisza):
Add 'message-format' log channel
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
Change 320548 had a related patch set uploaded (by Gergő Tisza):
Revert "Add 'message-format' log channel"
Change 320548 abandoned by Gergő Tisza:
Revert "Add 'message-format' log channel"
Reason:
On second thought we can keep this for now.
Change 320550 had a related patch set uploaded (by Gergő Tisza):
Always use parse in Message::__toString()
Change 320966 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
More robust, cleaned up MessageTest
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 :/
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
Change 681818 had a related patch set uploaded (by Krinkle; author: Krinkle):
[mediawiki/core@master] Message: Deprecate toString() with an implicit format
Change 681824 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] tests: Remove use of $msg->toString() in SetClaimTest
Change 681818 merged by jenkins-bot:
[mediawiki/core@master] Message: Deprecate toString() with an implicit format
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
Change 681741 had a related patch set uploaded (by Jforrester; author: Krinkle):
[mediawiki/core@REL1_36] Message: Deprecate toString() with an implicit format
Change 681740 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@REL1_36] tests: Remove use of $msg->toString() in SetClaimTest
Change 681741 merged by jenkins-bot:
[mediawiki/core@REL1_36] Message: Deprecate toString() with an implicit format
Change 700219 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):
[mediawiki/core@master] Message: Removed deprecated format property
Change 700219 merged by jenkins-bot:
[mediawiki/core@master] Message: Remove deprecated format property
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
Change 721313 merged by jenkins-bot:
[mediawiki/core@wmf/1.37.0-wmf.21] Message: Remove deprecated format property
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)