Page MenuHomePhabricator
Create Task
Maniphest T297725

<inputbox> extension should not be allowed to touch .js sub-pages
Closed, ResolvedPublicSecurity
Actions

Description

It looks like the extension can be used to make a user create e.g. a [[Special:MyPage/vector.js]] sub-page with any payload. The following example is disguised as a fake banner at the top of the page with a close button in the corner. Clicking the close button opens an edit window for a .js sub-page, preloaded with any payload the attacker wants. From there it's a single click on either "save" or even "preview" to execute this code on the user's machine.

<div style="position:absolute;top:0;right:0;width:100%;height:100px;background:#333;">
<div style="position:absolute;top:-19px;right:3px;">
<inputbox>
type = create
default = Special:MyPage/vector.js
preload = User:Attacker/payload.js
buttonlabel = ✕
hidden = 1
</inputbox>
</div>
</div>

It's only two button clicks for the user, at the moment. With T194606 it would be a single click.

Details

Risk Rating
Low
Author Affiliation
Wikimedia Deutschland
SubjectRepoBranchLines +/-
De-obfuscate attempts to trick people into editing .js pagesmediawiki/extensions/InputBoxmaster+6 -0
Remove unused EditPage::$mPreloadContent propertymediawiki/coremaster+0 -5
Allow EditPage's preload feature only on wikitext pagesmediawiki/coremaster+9 -5
Customize query in gerrit

Related Objects
Search...

Event Timeline

thiemowmde created this task.Dec 14 2021, 4:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 14 2021, 4:50 PM
thiemowmde mentioned this in T194606: It should be able to add a parameter to skip the form edit page and directly save the target text.Dec 14 2021, 4:51 PM
Bawolff subscribed.EditedDec 15 2021, 5:24 AM

Nothing particularly special about this extension - you could just directly link the url instead.

If a change in behaviour is warranted, it makes more sense to change how index.php handles preload parameters, then to change this one extension which makes a pretty frontend.

gerritbot added a comment.Dec 15 2021, 10:58 AM

Change 747491 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/InputBox@master] De-obfuscate attempts to trick people into editing .js pages

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

gerritbot added a project: Patch-For-Review.Dec 15 2021, 10:58 AM
gerritbot added a comment.Dec 15 2021, 12:27 PM

Change 747504 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Allow EditPage's preload feature only on wikitext pages

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

thiemowmde claimed this task.Dec 15 2021, 12:28 PM
thiemowmde added a project: WMDE-TechWish-XMAS-Sprint-2021-12-15.
thiemowmde merged a task: Restricted Task.
thiemowmde added a project: Security-Team.
thiemowmde moved this task from Sprint Backlog to Tech Review on the WMDE-TechWish-XMAS-Sprint-2021-12-15 board.
thiemowmde added subscribers: Andrew-WMDE, lilients_WMDE.
thiemowmde added a parent task: T297778: [survey] Add an option to InputBox and the preload parameter, supporting immediate save rather than an edit form.Dec 15 2021, 12:30 PM
thiemowmde changed the subtype of this task from "Task" to "Security Issue".
sbassett changed Author Affiliation from N/A to Wikimedia Deutschland.Dec 15 2021, 7:35 PM
sbassett changed Risk Rating from N/A to High.
sbassett set Security to Software security bug.
sbassett changed the visibility from "Public (No Login Required)" to "Custom Policy".
sbassett added a subscriber: gerritbot.
sbassett subscribed.
gerritbot added a comment.Dec 20 2021, 2:57 PM

Change 747504 merged by jenkins-bot:

[mediawiki/core@master] Allow EditPage's preload feature only on wikitext pages

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

thiemowmde added a comment.EditedDec 20 2021, 4:01 PM

We did a quick search for possibly use-cases on some wikis. Here is an example: https://en.wikipedia.org/wiki/Special:Search?search=insource%3Apreload+insource%3A%2Fpreload%3D%5B%5E%5C%5B%5C%5D%3C%3E%7B%7D%5D*%5C.js%2F&ns0=1&ns1=1&ns2=1&ns3=1&ns4=1&ns5=1&ns6=1&ns7=1&ns8=1&ns9=1&ns10=1&ns11=1&ns12=1&ns13=1&ns14=1&ns15=1&ns100=1&ns101=1&ns828=1&ns829=1&ns2300=1&ns2301=1&ns2302=1&ns2303=1

What we found:

  • It's rare.
  • Many, if not most of these links are broken anyway:
    • Any link with section=new doesn't work anyway on non-wikitext pages. Here is an example. It looks like this was possible a long time ago, but isn't any more. The relevant ContentHandler::supportsSections() method exists since 2012.
    • The trick to "one-click install" anything to the user's common.js would not work anyway when the page already exists. It's unreliable and effectively useless because of this.
  • Some gadgets make use of the fact that .js subpages are protected, and allow the user to provide some configuration there. Sometimes these subpages don't even contain JavaScript. This feature continues to work. What won't work any more are "one-click install" links or buttons – which have the same problems as above.
thiemowmde moved this task from Tech Review to Demo on the WMDE-TechWish-XMAS-Sprint-2021-12-15 board.Dec 20 2021, 4:02 PM
sbassett edited projects, added SecTeam-Processed; removed Security-Team.Dec 20 2021, 4:39 PM
WMDE-Fisch moved this task from Incoming to Backlog on the WMDE-TechWish-Maintenance board.Dec 20 2021, 4:54 PM
sbassett added a comment.EditedDec 21 2021, 4:07 PM

Thanks for the analysis, @thiemowmde. I think we can make this public again - I just wanted to be extra-careful when discussing anything security-related.

In T297725#7580588, @thiemowmde wrote:
  • The trick to "one-click install" anything to the user's common.js doesn't work any more when the page exists.

I think this is the key point. While the preload functionality does/did allow for a slightly-confusing or easily-obfuscated workflow which an attacker can exploit, a targeted user still had to actually interact with the UI to write any potentially malicious javascript to one of their javascript pages. i.e. there was never any way for an attacker to inject any malicious content without the user at least having to click once to write said content, if I'm understanding this correctly.

sbassett triaged this task as Low priority.Dec 21 2021, 4:08 PM
sbassett changed Risk Rating from High to Low.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
thiemowmde added a comment.EditedDec 21 2021, 4:26 PM

[…] there was never any way for an attacker to inject any malicious content without the user at least having to click once to write said content, if I'm understanding this correctly.

Yes, correct. An attacker needs to convince the user to click the save or preview button. The key argument is that the preload feature makes it very easy to make the edit window look harmless. This is much harder when the user needs to manually paste something.

WMDE-Fisch moved this task from Backlog to In progress on the WMDE-TechWish-Maintenance board.Dec 22 2021, 10:05 AM
awight moved this task from Demo to Done on the WMDE-TechWish-XMAS-Sprint-2021-12-15 board.Jan 4 2022, 9:21 AM
thiemowmde closed this task as Resolved.Jan 5 2022, 2:01 PM
Legoktm reopened this task as Open.Jan 9 2022, 11:40 PM
Legoktm added subscribers: Ammarpad, Erutuon, Legoktm.

Re-opening for further discussion and follow-ups. The ticket and patch talk about the risks of this with JavaScript pages (which I agree with and support), except the code itself hardcodes a restriction to only wikitext pages, leaving no opportunity for extensions or other models to re-enable the preload behavior when it's safe to do so.

From the duped report:

In T270542#7607690, @Erutuon wrote:
In T270542#7607613, @Ammarpad wrote:

This is now intentionally and explicitly allowed only in wikitext content model for security reasons cf. r26087d4. Whether it worked before or never did there's no point in keeping this open.

I can see that preload= was potentially dangerous in .js and .css pages in the User namespace, but that commit has disabled it in all non-wikitext pages, which breaks legitimate uses of preload= in the Module namespace. On English Wiktionary, Template:zh-dial displays dialectal synonyms of a Chinese word that are loaded from a data module. When the data module doesn't exist, it links to the data module with preload=Module:zh/data/dial-syn to load a template containing the full list of Chinese dialect codes: Module:zh/data/dial-syn. As of a few days ago, the link from the template no longer loads the data module template as it used to.

The author and reviewers of the commit apparently didn't look for legitimate use-cases before disabling preload= in non-wikitext pages. They should perhaps only have disabled preload= in the User namespace (perhaps also MediaWiki?) in JavaScript and CSS (and JSON?) pages, where the security concerns apply. From my perspective, the change landed without notice (at least I didn't see any in Tech News), and broke a feature of a template that editors of Chinese entries regularly use.

I'm inclined to start a ticket to enable preload= in the Module namespace, where it is definitely useful and was being used before the commit. (It might be fine to enable it in some other non-wikitext pages, like sanitized CSS and non-User-namespace JavaScript and CSS and JSON pages, but I don't know of any uses for preloading there.) In the meantime I can write some JavaScript to make Template:zh-dial work as intended.

Legoktm merged a task: T270542: Preload params don't work for non-wikitext content models.Jan 9 2022, 11:40 PM
Legoktm added subscribers: DannyS712, RhinosF1, Justinrleung, Bharatkhatri351.
Zabe subscribed.Jan 9 2022, 11:41 PM
gerritbot added a comment.Jan 10 2022, 9:12 AM

Change 752605 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Remove unused EditPage::$mPreloadContent property

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

thiemowmde added a project: User-notice.Jan 10 2022, 9:30 AM

What I find critical is that it's an allow list, i.e. the feature is only allowed in contexts where we know it's safe. We can expand this when legitimate uses-cases arise. No problem. One obvious option is to add CONTENT_MODEL_TEXT to the list. Not sure if this would make a difference. Another option is to add something like canPreloadContent() to either TextContent or the Content interface, maybe as a static method. Not sure where it makes the most sense.

But I'm not sure there are non-suspicious non-wikitext use-cases.

As far as I understand the examples listed in T270542 they all talk about pretty much exactly what this is about: semi-automatically injecting arbitrary code into a place where it can be executed. While attack-scenarios are a bit more unlikely in case of Lua modules, there is no guarantee either. Would we really make ScribuntoContent::canPreloadContent() return true?

I asked this as a question in T270542. As far as I understand it's really just for convenience, currently used about once per day. The same can still be done by manually copy-pasting code from one page to another.

thiemowmde updated the task description. (Show Details)Jan 10 2022, 9:36 AM
gerritbot added a comment.Jan 10 2022, 10:18 AM

Change 752625 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] [POC] Add ContentHandler::supportsPreloadContent() feature

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

gerritbot added a comment.Jan 10 2022, 10:20 AM

Change 752626 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Scribunto@master] [POC] Add ContentHandler::supportsPreloadContent() feature

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

gerritbot added a comment.Jan 10 2022, 1:37 PM

Change 752605 merged by jenkins-bot:

[mediawiki/core@master] Remove unused EditPage::$mPreloadContent property

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.17; 2022-01-10).Jan 10 2022, 2:00 PM
thiemowmde added a project: WMDE-TechWish-Sprint-2022-01-05.Jan 12 2022, 8:49 AM
thiemowmde moved this task from Sprint Backlog to Watching / Epic / Stalled on the WMDE-TechWish-Sprint-2022-01-05 board.
Erutuon added a comment.Jan 12 2022, 8:43 PM

As far as I understand the examples listed in T270542 they all talk about pretty much exactly what this is about: semi-automatically injecting arbitrary code into a place where it can be executed. While attack-scenarios are a bit more unlikely in case of Lua modules, there is no guarantee either. Would we really make ScribuntoContent::canPreloadContent() return true?

Could you clarify what attack scenarios you are thinking of? Because Lua modules are sandboxed, they can do a lot less than JavaScript. All a bad module can do is generate wikitext or an error message that messes up a page that is already transcluding the newly created module (restating what I said on the other task). That is, modules can do about what transcluded wikitext pages can do. Is there anything that modules can do that makes them significantly more dangerous than wikitext pages?

I'm trying to think of dangerous things modules can do that wikitext can't. Modules can create infinite loops that run up against the Lua execution time limit, generating a module error message, and they can allocate tons of memory by filling up tables and run into the Lua memory limit. Not sure if those qualify as a security risk.

I asked this as a question in T270542. As far as I understand it's really just for convenience, currently used about once per day. The same can still be done by manually copy-pasting code from one page to another.

In the Chinese dialectal synonym modules, it helps ensure that newly created modules have the same format and will not cause module errors. The average might be about once per day, but in the past year as many as 11 modules have been created on the same day, which would at least be tedious with copy-pasting.

thiemowmde added a subscriber: daniel.Jan 13 2022, 1:02 PM

I think everything you said is correct and makes a lot of sense. One additional scenario I can think of is when the Lua module pulls a lot of data from Wikidata. This is not only a read operation, but a lot will be recorded as usages. This might still be a serious DoD attack vector, even if most resource usage is limited.

I find it important to approach a powerful and potentially scary feature like this with an "allow list" in mind. It should be disabled by default, and only be enabled when we know it's useful and safe. It looks like this is the case for Lua modules. In https://gerrit.wikimedia.org/r/752626 I suggest to re-enable it for Lua. I just don't know who is going to review and merge this, or suggest a better alternative? The only comment so far is from @daniel.

Quiddity subscribed.Jan 13 2022, 10:09 PM

Re: Tech News (User-notice) - What wording would you suggest as the content, and When should it be included?
If it needs to be in the current edition (delivered on Monday), then we need an entry in https://meta.wikimedia.org/wiki/Tech/News/2022/03 within the next ~22 hours. Thanks!

Ammarpad unsubscribed.Jan 14 2022, 8:14 AM
thiemowmde added a comment.Jan 14 2022, 10:57 AM

I added a sentence to the upcoming TechNews.

Quiddity moved this task from To Triage to In current Tech/News draft on the User-notice board.Jan 14 2022, 9:45 PM
Jony subscribed.Jan 18 2022, 1:50 AM
Kipod subscribed.Jan 18 2022, 3:29 AM

IMO, the exploit should not be exposed, to avoid causing more pain to sites running vulnerable versions.

peace.

WMDE-Fisch added a project: WMDE-TechWish-Sprint-2022-01-19.Jan 19 2022, 9:10 AM
Lena_WMDE moved this task from Sprint Backlog to Watching / Epic / Stalled on the WMDE-TechWish-Sprint-2022-01-19 board.Jan 19 2022, 9:10 AM
Quiddity moved this task from In current Tech/News draft to Already announced/Archive on the User-notice board.Jan 20 2022, 7:18 PM
BEANS-X2 subscribed.Jan 24 2022, 6:19 PM
thiemowmde closed this task as Resolved.Feb 1 2022, 4:43 PM
thiemowmde mentioned this in T300644: Re-enable core's preload feature for certain non-wikitext content types.
thiemowmde added a parent task: T300644: Re-enable core's preload feature for certain non-wikitext content types.

What this ticket here asked for is done. I created T300644: Re-enable core's preload feature for certain non-wikitext content types for further discussions.

sbassett removed a project: Patch-For-Review.Feb 1 2022, 5:11 PM
gerritbot added a comment.Feb 5 2022, 11:41 PM

Change 747491 merged by jenkins-bot:

[mediawiki/extensions/InputBox@master] De-obfuscate attempts to trick people into editing .js pages

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.21; 2022-02-07); removed MW-1.38-notes (1.38.0-wmf.17; 2022-01-10).Feb 6 2022, 12:00 AM
Kipod unsubscribed.Feb 6 2022, 12:42 AM
Ladsgroup edited projects, added User-notice-archive; removed User-notice.Aug 13 2022, 1:54 PM
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