Page MenuHomePhabricator

Message -> string transformations should not affect each other
Open, NormalPublic

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

Tgr created this task.Sep 22 2016, 8:28 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 22 2016, 8:28 PM
Tgr added a subscriber: Anomie.Sep 22 2016, 8:48 PM

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

Tgr added a comment.Nov 1 2016, 5:27 PM

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 Normal priority.Nov 12 2016, 4:41 PM
Tgr moved this task from Backlog to Pending on the User-Tgr board.
Tgr reopened this task as Open.May 31 2017, 5:03 PM

Still need to remove the deprecated methods.

Krinkle removed Krinkle as the assignee of this task.Jun 7 2017, 5:49 PM
Krinkle added a subscriber: Krinkle.