Page MenuHomePhabricator

legends in htmlform (including Special:Preferences headers) use raw html messages
Closed, ResolvedPublic

Description

By looking around I found three vulnerabilities due to improper escaping of system messages. All of these need changing such messages and thus aren't too big, but still it's possible to inject custom javascript there. As I was suggested to do the last time I reported a problem like this, I'm listing the proposed changes that would fix the issue. Also note that while the proposed solutions fix the problems, I don't know whether they may broke something elsewhere.

1

Problem: group-*-member messages aren't properly escaped in logs and user rights pages
Solution: Escape the message in UserGroupMembership.php line 449:

return $msg->isBlank() ? $group : $msg->escaped();

2

Problem: Months abbreviation aren't escaped, tested in Special:AbuseLog and Special:CheckUser at least
Solution: Escape message in language.php at line 909:

return $this->msg( $msg )->escaped();

3

Problem: Section names in Special:Preferences aren't escaped.
Solution: Escape them in HTMLForm.php line 1820:

return $this->msg( "{$this->mMessagePrefix}-$key" )->escaped();

Event Timeline

Problem: Section names in Special:Preferences aren't escaped.
Solution: Escape them in HTMLForm.php line 1820:

return $this->msg( "{$this->mMessagePrefix}-$key" )->escaped();

There is somewhat of a backwards compatibility problem here. We don't know if anything is relying on legend being raw html. We need to make sure we are consistent with https://www.mediawiki.org/wiki/Deprecation_policy. Notwithstanding that, we really do need to fix this.

Maybe setting it to ->parse() would reduce the backward compat risk, as it would allow safe html through well banning unsafe.

Problem: Months abbreviation aren't escaped, tested in Special:AbuseLog and Special:CheckUser at least
Solution: Escape message in language.php at line 909:

return $this->msg( $msg )->escaped();

I actually think here that it should be the responsibility of the person using these methods to escape it. We don't know if the month name is ultimately destined for html, or maybe somewhere else where html is no supported. (The notable exception, is in commaList and friends we do escape the comma, as often that is used to combine html snippets)

Problem: group-*-member messages aren't properly escaped in logs and user rights pages
Solution: Escape the message in UserGroupMembership.php line 449:

return $msg->isBlank() ? $group : $msg->escaped();

Similarly, sometimes this needs to be non-html escaped depending on where the group name is destined to. Part of the problem here is that phan is not smart enough to understand how this code block is working. See T183174

Could we determine what is causing such problems with the first point? And, for the other ones, do this means that escaping should be done properly for every use case?

Bawolff renamed this task from Three tiny XSS vulnerabilities with system messages to legends in htmlform (including Special:Preferences headers) use raw html messages.Jul 9 2018, 11:54 PM
Bawolff triaged this task as Medium priority.

Anyway, I think we should fix escaping for 1 and 2 where mentioned in task description (logs and special:userRights for 1, special:abuselog and special:checkuser for 2): it's actually possible to inject custom JS there.

Yes, they definitely need to be escaped, just as near to the output as possible

I accidentally rediscovered your problem #3 and fixed it in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/457551 before finding this task. (I made it escape in SpecialPreferences.php. It seems HTMLForm::getLegend() should really return plain text, and its return value is being escaped elsewhere.)

I can't reproduce any of the problems today.

1

Problem: group-*-member messages aren't properly escaped in logs and user rights pages
Solution: Escape the message in UserGroupMembership.php line 449:

return $msg->isBlank() ? $group : $msg->escaped();

That code today still uses ->text() (now found in Language: here), but that looks correct to me. I found some callers that have been corrected to escape the result: 3979b47cb06b and a0d7e49f0941. I guess that did it.

2

Problem: Months abbreviation aren't escaped, tested in Special:AbuseLog and Special:CheckUser at least
Solution: Escape message in language.php at line 909:

return $this->msg( $msg )->escaped();

Adding escaping in Language::getMessageFromDB() seems likely to be incorrect in at least some cases.

Escaping of dates and times was corrected in AbuseLog in ef7f867f35, and in CheckUser probably in 6d6b229c02.

3

Problem: Section names in Special:Preferences aren't escaped.
Solution: Escape them in HTMLForm.php line 1820:

return $this->msg( "{$this->mMessagePrefix}-$key" )->escaped();

As already noted, fixed in 376bcd30ee.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".