Page MenuHomePhabricator

Allow loading gadget module by URL query parameter
Closed, ResolvedPublic

Description

Inspired by the "withJS-url parameter"-script floating around various wikis (which calls importScript() if the passed pagename is in the MediaWiki:-namespace), I think such functionality would qualify as core functionality.

For backwards compatiblity and to avoid any security issues on wikis which use the MediaWiki:-namespace on a less-than-sysop security level, this should be disabled by default, and enableable with a wiki global (eg. $wgAllowWithModuleLoading=true;)

However given how the resourceloader currently works and how it will/may work in the future [1].
I think it's best not to implemenent the script as it is now on those wikis, instead I've made this bug depend on bug 27535 ( registering wikistyles and wikiscripts as part of a module ), and propose to make the parameter someting like withModule.

Example:

That way it can be used to load CSS as well, since CSS can be (part of) a module.

This also stops the need to create mini-scriptpages in the MediaWiki:-namespace that would call importScript() and importStylesheet() several times (once for every css/js part of the module) in order to make it work with the withJS-hack.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Krinkle renamed this task from Add configuration option to enable loading modules based on url parameter to Allow loading modules by on request url query parameter.Apr 17 2017, 11:39 PM
Krinkle renamed this task from Allow loading modules by on request url query parameter to Allow loading modules by request url query parameter.
Krinkle reopened this task as Open.
Krinkle removed Krinkle as the assignee of this task.

As long as any implementation honours page restrictions surrounding module origins, I'm okay with this. (E.g. Special:Login?withModule=site will not work). There's no need to make this configurable imho.

Krinkle lowered the priority of this task from Medium to Lowest.Aug 18 2018, 6:11 AM
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Moving to radar as we're not likely to work on this at any point. Patches welcome, though.

I don't see this as something that needs to be part of core and am less sure if we'd accept it as patch nowdays. It's still require maintenance and non-trivial security considerations in terms of being able to load code in code in unanticipated places.

The original request is for gadgets, which are currently enabled only in site-wide fashion (until T63007). I'll narrow the scope of this ticket to just that part of it, which would be less problematic, and imho a more natural fit in the product roadmap. There's a clear use case for it, and I think it would be something a maintainer of Gadgets would get to eventually and might also accept patches for before that.

Krinkle renamed this task from Allow loading modules by request url query parameter to Allow loading gadget module by URL query parameter.Oct 8 2021, 5:53 PM

Change 730684 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/extensions/Gadgets@master] Allow loading gadget module by URL query param

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

I'm copying in the Security team to give this a lookover as well. As it stands this would be supported on all index.php contexts regardless of wiki visibility/configuration or logged-in state (e.g. private wikis will run the code when visited by someone who received such a crafted link), namespace (incl special pages for blocking users, wikidata, articles, and file pages), or page action (e.g. view page, page history, edit mode, delete/undelete, etc.).

I'm copying in the Security team to give this a lookover as well. As it stands this would be supported on all index.php contexts regardless of wiki visibility/configuration or logged-in state (e.g. private wikis will run the code when visited by someone who received such a crafted link), namespace (incl special pages for blocking users, wikidata, articles, and file pages), or page action (e.g. view page, page history, edit mode, delete/undelete, etc.).

Thanks for flagging this - we had a chat about this at the Security-Team clinic this morning. From what we can tell, this functionality would really only provide an additional layer of convenience and maybe abstraction for an attacker to exploit, as similar functionality can already be accomplished via importScript() and importStylesheet() (as the task description notes) and any modules would be loaded from pages within the theoretically more trusted MediaWiki: namespace. In this case, from a security perspective, we'd likely rate this as low risk, unless there are some additional concerns here that we're not understanding.

Change 730684 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Add global 'withgadget' query parameter for ad-hoc loading of gadgets

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

Gadgets can now be loaded with ?withgadget= URL parameter. This is live in production wikis.

This one can probably also be a tech news notice. Something like:

Wikis can now load gadgets ad hoc (<-- probably not easily translatable, so better word wanted) with the withgadget URL parameter. This can be used to replace an earlier snippet that typically looks like withJS or withCSS.

JJMC89 subscribed.

Current draft:

Gadgets can now be loaded on demand with the withgadget URL parameter. This can be used to replace an earlier snippet that typically looks like withJS or withCSS.

This seems like a bad idea. The point of gadgets is to allow users to make risk assessment via an opt-in mechanism - someone with checkuser access might be much more cautious about what tools they enable than someone with no elevated privileges.

From what we can tell, this functionality would really only provide an additional layer of convenience and maybe abstraction for an attacker to exploit, as similar functionality can already be accomplished via importScript() and importStylesheet() (as the task description notes) and any modules would be loaded from pages within the theoretically more trusted MediaWiki: namespace.

This allows an attacker to enable a gadget for anyone just by making them visit an URL. That wasn't possible before. It doesn't allow an attacker to write any code that would be executed, but now all users are going to be as vulnerable as the most vulnerable gadget (eg. something experimental), regardless of how many people actually use it. That increases the attack surface considerably. It also makes social engineering attacks easier - it's probably easier to convince an interface admin to turn your code into a gadget than to turn it into a default gadget.

It will also expose hidden gadgets which were not available to users until now (could only be loaded programmatically by another gadget), and that might expose some new vulnerability, although that does seem like a fringe scenario.

IMO this should be limited to gadgets which indicate in their definition that they can be loaded this way.

(Also, might want to support providing multiple values to the parameter.)

+1 to the suggestion from Tgr to limit this to a subset of gadgets.

On frwiki, we rejected a request to add withJS/withCSS because this seemed too unsafe (https://fr.wikipedia.org/wiki/Wikip%C3%A9dia:Demande_d%27intervention_sur_un_message_syst%C3%A8me/Archives2#MediaWiki:Common.js_%E2%80%93_prise_en_charge_des_param%C3%A8tres_withJS_&_withCSS). The consensus on that request was to not do that unless it's restricted to a very short list of preapproved pages.

frwiki has approximately 90 gadgets, which I would not consider as a short and managable list. Among those, at least 6 load pages from other wikis, including one from a user page (WikEd). Some of these gadgets are very complex and were written by long gone users which make them hard to maintain (e.g. LiveRC). I'm not comfortable with the idea that just visiting a link could load any of these gadgets.

Ammarpad changed the task status from Open to In Progress.
Ammarpad raised the priority of this task from Lowest to Medium.

Change 752995 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/extensions/Gadgets@master] Restrict ?withgadget query parameter to opt-in gadgets.

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

While it's obviously not risk-free, I don't think it adds any more risk than what exists already with unmaintained and antiquated gadgets in wide usage. By default, I think gadgets should be treated as safe. It doesn't make sense to consider trustworthiness of gadgets when anyway they can be edited by just interface-admins. Many wikis are already using withJS and withCSS so for those sites nothing much changes (other than the delivery being RL-optimised). Communities where it's possible to get interface-admins to gadgetize vulnerable/malicious code likely have larger problems to worry about.

Would it make sense to make it opt-out rather than opt-in, so that communities can opt out the ones which they feel are not too safe – such as the ones loaded from another wiki, or from userspace (not that any gadget should exist that loads from userspace), or which rely on external APIs, or are particularly ancient?

SD0001 said a lot of what I had typed up.

In T29766#7611796, @Tgr wrote:

The point of gadgets is to allow users to make risk assessment via an opt-in mechanism - someone with checkuser access might be much more cautious about what tools they enable than someone with no elevated privileges.

We already have withJS/CSS onwiki and there is the snippet directly published on mediawiki.org. If you want to say this is a bad idea, then you should also argue that security should review that snippet and decide whether that is safe or not, and subsequently to enforce removal of those scripts if security comes down on the 'not-safe' side.

This allows an attacker to enable a gadget for anyone just by making them visit an URL. That wasn't possible before. It doesn't allow an attacker to write any code that would be executed, but now all users are going to be as vulnerable as the most vulnerable gadget (eg. something experimental), regardless of how many people actually use it. That increases the attack surface considerably.

I think you're likely to have larger issues than a subtle 'open this gadget' if an attacker has access to an interface administrator account, or one of the accounts that have scripts being imported to gadgets. (I think that second vector is up to interface admins to improve, possibly driven by some kind and considerate WMF employees; I actually started on cleaning up one script of this very sort this past week, but I know there are at least 2 or 3 others.)

It also makes social engineering attacks easier - it's probably easier to convince an interface admin to turn your code into a gadget than to turn it into a default gadget.

I don't think this change changes that vector meaningfully.

It will also expose hidden gadgets which were not available to users until now (could only be loaded programmatically by another gadget), and that might expose some new vulnerability, although that does seem like a fringe scenario.

Actually, I was intending specifically to move some of our current scripts loaded on-click-demand to hidden gadgets. There's no reason to have those available to general users who will simply be confused by the existence on that preferences tab page. (I guess I could put them in a "withJS" section on the gadgets page, but we already know that's not an amazingly designed preferences page without any hierarchy.)

IMO this should be limited to gadgets which indicate in their definition that they can be loaded this way.

See also T8883: Allow page-specific inclusion of <script>s, etc. in header (I'm not sure precisely what Krinkle is referencing there when he says "it's existed for a while") and the linked 'allow more limited gadget loading'. I'd personally be fine with a |pages= parameter in the definition which I think would probably obviate the need for this change entirely (I don't know if there's scope for having some basic wildcard support in such so that subpages have the same access as some root page).

That all said, not every wiki has savvy int admins, but maybe we should be leaning more on global int admins then who mostly are JavaScript savvy.

By default, I think gadgets should be treated as safe.

This is, unfortunately, wishful thinking. I have been reviewing gadgets on enwiki over the last few weeks, and have found XSS vulnerabilities in two of them. I am also pretty sure that I have found a third, and there are very likely to be more vulnerabilities in gadgets that I have not looked at yet. Now that only interface admins can add gadgets the situation for new gadgets will likely improve, but existing gadgets still have a lot of problems.

Many wikis are already using withJS and withCSS so for those sites nothing much changes (other than the delivery being RL-optimised).

I think the addition of withgadget is a good move for these wikis security-wise, as it allows them to limit pages loaded to currently active gadgets instead of any JavaScript page in the MediaWiki namespace. However, I tend to agree with @Tgr that it should be limited to gadgets that opt in to being loaded in this fashion. That would allow wikis to move away from using withJS and withCSS, while still keeping the attack surface as small as possible.

This is, unfortunately, wishful thinking. I have been reviewing gadgets on enwiki over the last few weeks, and have found XSS vulnerabilities in two of them.

Thank you for fixing the vulnerabilities. It is much appreciated. But those gadgets are also enabled by thousands of users, so an attacker could exploit them without needing to convince someone to follow a ?withgadget link, which is why I think this doesn't increase the attack surface in any meaningful way, and presumably why the Security-Team approved this as "low-risk". In general, vulnerabilities introduced by accident (rather than with malicious intent) could be present even in the JS deployed by mediawiki core or extensions.

I don't think it adds any more risk than what exists already with unmaintained and antiquated gadgets in wide usage.

I think there's is some difference here. "Unmaintained and antiquated gadgets" must either be made default — which made the 'the community' responsible for that — or be specifically enabled by the user, which shifts the responsibility to 'the user' for it to be executed for them. The new query parameter introduces third possibility, by executing gadget for a user just after following a random link. The user is still responsible but less so in control than the former ways. I agree the risk is still low too given the already existing possibilities and the gatekeeping on the gadgets' sources.

We already have withJS/CSS onwiki and there is the snippet directly published on mediawiki.org. If you want to say this is a bad idea, then you should also argue that security should review that snippet and decide whether that is safe or not, and subsequently to enforce removal of those scripts if security comes down on the 'not-safe' side.

I think it is a bad idea (and replacing it with this feature would certainly be an improvement, as gadgets are more manageable than all MediaWiki JS pages in existence). The trend for on-wiki scripts is to move from bad to less bad over time, security-wise (all admins being able to edit JS was a bad idea, being able to load JS from outside the MediaWiki / user space is a bad idea etc), so I don't think precedent means much here.

I think you're likely to have larger issues than a subtle 'open this gadget' if an attacker has access to an interface administrator account, or one of the accounts that have scripts being imported to gadgets. (I think that second vector is up to interface admins to improve, possibly driven by some kind and considerate WMF employees; I actually started on cleaning up one script of this very sort this past week, but I know there are at least 2 or 3 others.)

Malicious (or hijacked) interface admins and vulnerable scripts are two very different threat models. There is no way to stop malicious interface admins without human detection and intervention; the goal there is to prevent account hijacking, make the group no larger than needed and support peer review of interface-admin actions. Putting partial restrictions on gadgets is certainly no help there.

Threat from vulnerable scripts on the other hand can be limited by 1) restricting when the script can run, 2) improving code review. Having to allow-list gadgets for loading-by-url would help both, by limiting the reach of vulnerable scripts (due to the usual attention mechanics, scripts that are used by few people are more likely to have bugs), and by letting reviewers prioritize scripts that do need to reach all users.

Actually, I was intending specifically to move some of our current scripts loaded on-click-demand to hidden gadgets. There's no reason to have those available to general users who will simply be confused by the existence on that preferences tab page. (I guess I could put them in a "withJS" section on the gadgets page, but we already know that's not an amazingly designed preferences page without any hierarchy.)

Sure, I think that's a good plan. But then from a security perspective those are basically default-on gadgets, and should be marked. Since you are specifically creating them with loading-by-URL in mind, flagging them as such should be no obstacle.

See also T8883: Allow page-specific inclusion of <script>s, etc. in header (I'm not sure precisely what Krinkle is referencing there when he says "it's existed for a while") and the linked 'allow more limited gadget loading'. I'd personally be fine with a |pages= parameter in the definition which I think would probably obviate the need for this change entirely (I don't know if there's scope for having some basic wildcard support in such so that subpages have the same access as some root page).

T241524: Parser function for loading gadgets could also benefit from such a flag.

As for specifying the list of pages, from a security perspective I don't think it has much benefit over just flagging the gadget as "can be loaded even if the user hasn't explicitly enabled it". Having to target a specific page doesn't really make it any harder for an attacker to make you load the gadget and use a reflected XSS or such. From a usability perspective, a page list might be nice, although IMO T241524 would give much of those benefits while being more flexible and easier to handle wrt cache invalidation.

Thank you for fixing the vulnerabilities. It is much appreciated. But those gadgets are also enabled by thousands of users, so an attacker could exploit them without needing to convince someone to follow a ?withgadget link, which is why I think this doesn't increase the attack surface in any meaningful way, and presumably why the Security-Team approved this as "low-risk". In general, vulnerabilities introduced by accident (rather than with malicious intent) could be present even in the JS deployed by mediawiki core or extensions.

The risk from an attacker breaching an account is fairly negligible unless the account is an interface-admin, bureaucrat or steward (or checkuser, to a smaller extent). Hopefully those users are more security-conscious and cautious about what gadgets they use. So whether a gadget is enabled by thousands of users matters less than which users it is enabled by.

Only a small fraction of gadgets really benefit from being possible to enable by URL. Typically the gadgets which do doesn't really need to be loaded in other ways. I think there is little harm in requiring a flag. (It will break the "demo" use case but it's not that hard to enable a gadget so I don't think that's a big deal.)

This comment was removed by Izno.
In T29766#7617613, @Tgr wrote:

I think it is a bad idea... so I don't think precedent means much here.

I am not suggesting it is 'precedent', I am saying it exists and should be judged on whatever arbitrary scale we're using today and subsequently dealt with. (So, in fact, I am saying the opposite - it is not precedential.)

Sure, I think that's a good plan. But then from a security perspective those are basically default-on gadgets, and should be marked. Since you are specifically creating them with loading-by-URL in mind, flagging them as such should be no obstacle.

I do not know what you mean by 'flagging them as such'. In gadgets definition? I already accepted that as a reasonable limitation, I just took it the step further.

See also T8883: Allow page-specific inclusion of <script>s, etc. in header (I'm not sure precisely what Krinkle is referencing there when he says "it's existed for a while") and the linked 'allow more limited gadget loading'. I'd personally be fine with a |pages= parameter in the definition which I think would probably obviate the need for this change entirely (I don't know if there's scope for having some basic wildcard support in such so that subpages have the same access as some root page).

As for specifying the list of pages, from a security perspective I don't think it has much benefit over just flagging the gadget as "can be loaded even if the user hasn't explicitly enabled it".

You know exactly where, why, and how the gadget will load and subsequently you know who to warn if Something Bad happens (not that you have access to who has visited Arbitrary Page in some period). "Attackers were able to attack this gadget, which means if you viewed these pages your account may be in danger" and so on.

That's besides the "we've actually locked down where this gadget can be used and if you want to use it in more places you can ask an interface admin who can judge whether that use is appropriate".

Having to target a specific page doesn't really make it any harder for an attacker to make you load the gadget and use a reflected XSS or such. From a usability perspective, a page list might be nice, although IMO T241524 would give much of those benefits while being more flexible and easier to handle wrt cache invalidation.

I think T241524: Parser function for loading gadgets is a terrible idea. I don't want to have to manage the use of such parser functions either as a wikignome or as an interface admin. But that's off this topic.

I do not know what you mean by 'flagging them as such'. In gadgets definition? I already accepted that as a reasonable limitation, I just took it the step further.

Not sure what we are arguing about then :)

You know exactly where, why, and how the gadget will load and subsequently you know who to warn if Something Bad happens (not that you have access to who has visited Arbitrary Page in some period). "Attackers were able to attack this gadget, which means if you viewed these pages your account may be in danger" and so on.

I think it's about equally hard to get from the logs the list of people who visited a specific page, and the list of people who visited a specific URL query.

That's besides the "we've actually locked down where this gadget can be used and if you want to use it in more places you can ask an interface admin who can judge whether that use is appropriate".

As I said I don't think that's useful from a security perspective. It doesn't seem useful from a wiki administration perspective, either - why would you want that kind of restriction?
The main benefit would be that it would allow enabling gadgets for specific pages without having to craft special links and sending people through them. But caching would get complicated. These gadgets would not be limited to logged-in users, so you'd need to clear edge caches whenever the gadget definition changes.

[…] those gadgets are also enabled by thousands of users, so an attacker could exploit them without needing to convince someone to follow a ?withgadget link, which is why I think this doesn't increase the attack surface in any meaningful way, […]

If a gadget exists and contains a vulnerability, this is not exploitable by default. The victim has to load the gadget for it to have effect, and (in the case of XSS) the victim would also need to follow a link to a certain on-wiki context where the gadget's weakness is exploited.

Most gadgets aren't enabled by default, and people with sensitive access may have even fewer gadgets enabled on their accounts. Allowing any URL to arbitrarily load any gadget, unequivocally increases the attack surface in a meaningful way. Without it, someone with sensitive access could (mostly) follow any web link and simply trust the platform (in combination with their previous choices, preferences, and safeguards in place). With this feature in place, this is no longer the case, and that's a major set back. I regretfully failed to realize this.

As for withCSS and withJS, those are similarly terrible in this way. If an interface admin chooses to install through on a wiki's Common.js, they should be taking that into account with every other JS file in the MediaWiki-namespace. It's a non-trivial risk, but it can be managed, and it's a local decision. When we add it to the wiki software, this isn't decided locally but globally, and there's currently no way to opt-out for third parties. It increases risk surface for all WMF wikis, and all other MediaWiki installs worldwide.

For wikis that do already have withJS, I think it compares both better and worse to withgadget.
Worse: because (as others said) it also exposes random drafts and old abandoned scripts.
Better: because it does not offer the ability to load code as a module. Many gadget scripts become harmless and simply do not work if loaded via withJS, e.g. due to missing dependencies, incorrect scope, or missing sibling pages that need to run for the module to work.

Another aspect here is privacy. It is one thing for a script to be vulnerable. It is another for a script to make choices on the user's behalf without their consent. E.g. scripts that send information to third-party services with the understanding that the user gives consent when they enable a gadget.

In any event, making the withgadget feature opt-in is trivial with a boolean option. Something like [allow-impromptu] or [anywhere=allowed], with the documentation mentioning that the option allows the gadget to load by permalink even on accounts that have not chosen to enable it.

In any event, making the withgadget feature opt-in is trivial with a boolean option. Something like [allow-impromptu] or [anywhere=allowed], with the documentation mentioning that the option allows the gadget to load by permalink even on accounts that have not chosen to enable it.

Note the patch suggested by @Ammarpad: https://gerrit.wikimedia.org/r/752995.

Gadgets will need an explicit supportsUrlLoad flag in their definition for the withgadget URL parameter to work. Thanks @Ammarpad! Can you also update the extension docs?

Change 752995 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Restrict ?withgadget query parameter to opt-in gadgets.

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

Doc has been updated on wiki.

Tgr moved this task from Already announced/Archive to Announce in next Tech/News on the User-notice board.

Hi. Does this need a new announcement in next Monday's Tech News edition?
If so, what should it say?
(Or, please add it directly to https://meta.wikimedia.org/wiki/Tech/News/2022/05 within the next 24 hours. :-) )

Context: The prior announcement was in https://meta.wikimedia.org/wiki/Tech/News/2022/02 and said:

Gadgets can now be loaded on demand with the withgadget URL parameter. This can be used to replace an earlier snippet that typically looks like withJS or withCSS. [4]

Sidenote: In case it helps to choose the wording, there appear to be a few uses (or mentions?) already: https://global-search.toolforge.org/?q=withgadget&namespaces=8
Sidenote2: The related docs update was at https://www.mediawiki.org/w/index.php?title=Extension:Gadgets&diff=5034969&oldid=5032456&diffmode=source

Tgr moved this task from Already announced/Archive to Announce in next Tech/News on the User-notice board.

Hi. Does this need a new announcement in next Monday's Tech News edition?

Yes

If so, what should it say?

Not sure how to phrase it to Tech News requirement, but the gist is that, the ?withgadget URL parameter announced in the last Tech News is now disabled for gadgets by default. Gadget that wants support it (to be loadable with it) can specify so in their gadget definition as explained in the doc.

I've added it as:

If a gadget should support the new ?withgadget URL parameter that was announced 3 weeks ago, then it must now also specify supportsUrlLoad in the gadget definition (documentation).

Please edit that if needed, within the next 20 hours. Thanks!

@SD0001 can ?withgadet load multple gadgets, or be called multiple times?

Looking at the code (rEGAD includes/GadgetHooks.php:319 (at dfe7928f4edc)), only one name can be specified. Maybe a new task should be created to track allowing multiple gadgets to be loaded. However, unless it’s some ad-hoc pairing of two gadgets, it should not be necessary: gadgets can define dependencies and peer gadgets, which should cover the most use cases (while resulting in shorter URLs and making the chances of accidentally breaking the system lower).

It's a trivial change to make the parameter a comma- or pipe-separated list. The potential downsides are security review getting harder (maybe there is some vulnerability that only can be triggered when multiple gadgets execute in parallel - seems very unlikely) and vandalism (some sort of DoS attack where a ton of gadgets are loaded - again seems super unlikely, since the attack would only affect people using the link).