Page MenuHomePhabricator
Create Task
Maniphest T347708

CVE-2024-23172: Several not properly escaped messages in the CheckUser extension
Closed, ResolvedPublicSecurity
Actions

Assigned To
Mstyles
Authored By
Dreamy_Jazz
Sep 29 2023, 1:50 PM
Tags
Referenced Files
F41659453: T347708-3.patch
Jan 8 2024, 7:56 PM
F37853187: T347708-2.patch
Sep 29 2023, 5:14 PM
F37853063: T347708.patch
Sep 29 2023, 3:30 PM
F37852730: image.png
Sep 29 2023, 1:50 PM
Subscribers
Aklapper
Bawolff
Daimona
Dreamy_Jazz
gerritbot
Mstyles
sbassett
thcipriani

Description

In testing CheckUser using the new x-xss language (as recently announced on Wikitech-l) I found that the following messages cause the display of the alert in several interfaces.

Special:CheckUserLog:

  • checkuser-log-entry-userips
  • checkuser-log-entry-ipedits
  • checkuser-log-entry-ipusers
  • checkuser-log-entry-ipedits-xff
  • checkuser-log-entry-ipusers-xff
  • checkuser-log-entry-useredits
  • checkuser-log-entry-investigate
  • parentheses

Special:CheckUser 'Get users':

  • checkuser-massblock-text

Special:Investigate 'Timeline':

  • september (month related message)

Some of the messages that start with checkuser-log- contain HTML but are not in the RawHtmlMessages config. The other messages need to be properly escaped.

Example:

image.png (1×2 px, 595 KB)

Details

Risk Rating
Low
Author Affiliation
WMF Technology
Related Changes in Gerrit:
SubjectRepoBranchLines +/-
SECURITY: Address many XSS vectors via message definitionsmediawiki/extensions/CheckUserREL1_40+24 -24
SECURITY: Address many XSS vectors via message definitionsmediawiki/extensions/CheckUserREL1_41+24 -24
SECURITY: Address many XSS vectors via message definitionsmediawiki/extensions/CheckUserREL1_39+24 -24
SECURITY: Address many XSS vectors via message definitionsmediawiki/extensions/CheckUsermaster+24 -24
Customize query in gerrit

Related Objects

Event Timeline

Dreamy_Jazz created this task.Sep 29 2023, 1:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 29 2023, 1:50 PM
Dreamy_Jazz added a comment.EditedSep 29 2023, 2:11 PM

Some of these messages in en.json have HTML for bidirectional isolation. Others do not contain HTML. I will be shortly uploading a patch that:

  • Adds the messages with HTML to wgRawHtmlMessages.
  • Calls Message::text when the message key is in wgRawHtmlMessages and calls Message::escaped when the message is not in that config
  • Passes the parameters to these messages as raw parameters as they contain HTML.
  • Escapes the parentheses message instead of calling ::text
Dreamy_Jazz renamed this task from XSS in CheckUserLog line messages to Multiple XSS vectors via editing MediaWiki namespace messages in Special:CheckUserLog.Sep 29 2023, 2:13 PM
Dreamy_Jazz updated the task description. (Show Details)Sep 29 2023, 2:17 PM
Dreamy_Jazz renamed this task from Multiple XSS vectors via editing MediaWiki namespace messages in Special:CheckUserLog to Multiple XSS vectors in the CheckUser extension via editing MediaWiki namespace pages.Sep 29 2023, 2:24 PM
Dreamy_Jazz added a project: CheckUser.
Dreamy_Jazz updated the task description. (Show Details)
Dreamy_Jazz updated the task description. (Show Details)Sep 29 2023, 2:52 PM
Dreamy_Jazz renamed this task from Multiple XSS vectors in the CheckUser extension via editing MediaWiki namespace pages to Users without editsitecss and editsitejs can add JS code to messages used in the CheckUser extension by editing MediaWiki namespace pages.Sep 29 2023, 3:00 PM
Dreamy_Jazz renamed this task from Users without editsitecss and editsitejs can add JS code to messages used in the CheckUser extension by editing MediaWiki namespace pages to Several not properly escaped messages in the CheckUser extension.Sep 29 2023, 3:03 PM
Dreamy_Jazz added a comment.Sep 29 2023, 3:30 PM

Proposed patch:

T347708.patch7 KBDownload

Bawolff subscribed.Sep 29 2023, 5:01 PM

Not really that relevant to the issue at hand, but it seems kind of weird for the message to have the <bdi> in it, instead of just passing that as part of the $1 substitution text. If the <bdi> really has to be in the message, wouldn't it maybe make more sense to have the messages with <bdi> in them be ->parse() instead of raw html and added to the raw html list?

Dreamy_Jazz added a comment.Sep 29 2023, 5:03 PM
This comment was removed by Dreamy_Jazz.
Bawolff added a comment.Sep 29 2023, 5:03 PM

indeed, that makes sense

Dreamy_Jazz added a comment.Sep 29 2023, 5:04 PM

I hadn't tried using parse. I will try that now.

Also deleted my comment above just as your reply came in. Apologies.

Dreamy_Jazz added a comment.Sep 29 2023, 5:14 PM

Updated proposed patch:

T347708-2.patch6 KBDownload

Bawolff added a comment.EditedSep 29 2023, 5:15 PM

The main thing I'm curious about is why phan-taint-check didn't catch this instance.

  • LogPager::formatRow clearly uses i18n in text mode
  • ReverseChronologicalPager::getRow calls LogPager::formatRow
  • IndexPager::getBody calls ReverseChronologicalPager::getRow
  • SpecialCheckUserLog does $out->addHTML( ... . $pager->getBody() )

Seems like a fairly direct taintedness path.

Edit. phan-taint-check readme does say:

Phan does not currently have an API for accessing subclasses for a given class.
Therefore the SecurityCheckPlugin cannot accommodate certain data flows for
subclasses that should obviously be considered tainted. The workaround for this
is to mark any relevant subclass functions as @return-taint html.

So maybe that's the issue here.

Bawolff added a subscriber: Daimona.Sep 29 2023, 5:17 PM
Bawolff mentioned this in T347744: i18n-xss vectors on Special:SecurePoll.Sep 29 2023, 5:58 PM
sbassett moved this task from Incoming to Security Patch To Deploy on the Security-Team board.Oct 2 2023, 4:08 PM
sbassett added projects: SecTeam-Processed, Vuln-XSS.
Daimona added a comment.Oct 2 2023, 5:12 PM
In T347708#9211691, @Bawolff wrote:

The main thing I'm curious about is why phan-taint-check didn't catch this instance.

  • LogPager::formatRow clearly uses i18n in text mode
  • ReverseChronologicalPager::getRow calls LogPager::formatRow
  • IndexPager::getBody calls ReverseChronologicalPager::getRow
  • SpecialCheckUserLog does $out->addHTML( ... . $pager->getBody() )

Seems like a fairly direct taintedness path.

Edit. phan-taint-check readme does say:

Phan does not currently have an API for accessing subclasses for a given class.
Therefore the SecurityCheckPlugin cannot accommodate certain data flows for
subclasses that should obviously be considered tainted. The workaround for this
is to mark any relevant subclass functions as @return-taint html.

So maybe that's the issue here.

Sorry, I didn't get a notification when you added me as subscriber and therefore didn't see this comment. I haven't looked at the actual code, but based on your summary, I think this might indeed be caused by the limitation that you quoted, for which we have T289314.

If I understand it correctly, the issue with LogPager can be simplified to this demo, which is pretty much just a variant of the test case in T289314.

Dreamy_Jazz added a project: Patch-For-Review.Oct 2 2023, 11:07 PM
sbassett subscribed.Oct 4 2023, 7:04 PM
In T347708#9211689, @Dreamy_Jazz wrote:

Updated proposed patch:

T347708-2.patch6 KBDownload

CR+2 to this version of the patch. I think this could likely go out during next Monday's (2023-10-09) security deployment window.

Mstyles subscribed.Oct 16 2023, 10:06 PM
In T347708#9211689, @Dreamy_Jazz wrote:

Updated proposed patch:

T347708-2.patch6 KBDownload

deployed

Mstyles mentioned this in T347659: Write and send supplementary release announcement for extensions and skins with security patches (1.35.14/1.39.6/1.40.2/1.41.0).Oct 16 2023, 10:18 PM
Mstyles moved this task from Security Patch To Deploy to Watching on the Security-Team board.Oct 16 2023, 11:37 PM
thcipriani subscribed.Jan 8 2024, 6:56 PM

Hi @Dreamy_Jazz

In T347708#9211689, @Dreamy_Jazz wrote:

Updated proposed patch:

T347708-2.patch6 KBDownload

This patch no longer applies cleanly on top of CheckUser. Could you rebase it for train this week?

Dreamy_Jazz added a comment.Jan 8 2024, 7:56 PM
In T347708#9443024, @thcipriani wrote:

Hi @Dreamy_Jazz

In T347708#9211689, @Dreamy_Jazz wrote:

Updated proposed patch:

T347708-2.patch6 KBDownload

This patch no longer applies cleanly on top of CheckUser. Could you rebase it for train this week?

Done:

T347708-3.patch5 KBDownload

Dreamy_Jazz added a comment.EditedJan 8 2024, 8:01 PM

I think we should be able to upload this patch to gerrit and backport it now, in a similar way to T347744: i18n-xss vectors on Special:SecurePoll. This is because these XSS vectors are only possible if the user performing the attack has the ability to edit MediaWiki namespace pages.

Any thoughts on doing this? Just saves time by not needing to rebase in the future.

sbassett added a comment.EditedJan 8 2024, 8:35 PM
In T347708#9443453, @Dreamy_Jazz wrote:

I think we should be able to upload this patch to gerrit and backport it now, in a similar way to T347744: i18n-xss vectors on Special:SecurePoll. This is because these XSS vectors are only possible if the user performing the attack has the ability to edit MediaWiki namespace pages.

Any thoughts on doing this? Just saves time by not needing to rebase in the future.

Thanks for the rebased patch. I think we're generally fine opening up issues like this whenever if they are for codebases not bundled with the regular MediaWiki releases. This issue will be made public soon anyways, as it will be announced with the upcoming supplemental release (T347659), which should be out later this week or very early next week, depending upon when Mitre gets us the relevant CVEs.

Mstyles added a subscriber: gerritbot.Jan 10 2024, 12:15 AM
Mstyles added a comment.Jan 10 2024, 12:30 AM

public patch is out: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/989179

gerritbot added a comment.Jan 10 2024, 2:03 PM

Change 989179 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] SECURITY: Address many XSS vectors via message definitions

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

gerritbot added a comment.Jan 10 2024, 2:19 PM

Change 989505 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@REL1_41] SECURITY: Address many XSS vectors via message definitions

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

gerritbot added a comment.Jan 10 2024, 2:22 PM

Change 989483 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@REL1_40] SECURITY: Address many XSS vectors via message definitions

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

gerritbot added a comment.Jan 10 2024, 2:27 PM

Change 989527 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@REL1_39] SECURITY: Address many XSS vectors via message definitions

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

Jdforrester-WMF added a project: MW-1.42-notes (1.42.0-wmf.14; 2024-01-16).Jan 10 2024, 4:00 PM
Mstyles renamed this task from Several not properly escaped messages in the CheckUser extension to CVE-2024-23172: Several not properly escaped messages in the CheckUser extension.Jan 16 2024, 6:43 PM
gerritbot added a comment.Jan 16 2024, 6:49 PM

Change 989527 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_39] SECURITY: Address many XSS vectors via message definitions

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

gerritbot added a comment.Jan 16 2024, 6:50 PM

Change 989505 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_41] SECURITY: Address many XSS vectors via message definitions

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

gerritbot added a comment.Jan 16 2024, 6:51 PM

Change 989483 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_40] SECURITY: Address many XSS vectors via message definitions

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

Mstyles closed this task as Resolved.Jan 16 2024, 6:52 PM
Mstyles claimed this task.
Mstyles moved this task from Watching to Our Part Is Done on the Security-Team board.
sbassett triaged this task as Low priority.Jan 16 2024, 10:51 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Low.
Stashbot added a comment.Jan 31 2024, 9:54 PM

Mentioned in SAL (#wikimedia-operations) [2024-01-31T21:54:52Z] <Dreamy_Jazz> Removed already applied patches for T347708 from /srv/patches

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits