Page MenuHomePhabricator

Special:Userlogin loads mw gadget scripts - potential security vulnerability
Closed, ResolvedPublic

Description

Author: M8R-cyc3n3

Description:
The userlogin page does disable the MediaWiki:Common.js and
so on down to the the user's javascript.

However if a user has selected gadgets using specialprefs
and re-visits the userlogin page after logging in (perhaps
to switch to their other-other account :p) a malicious
gadget could go south with the password, just as easily as
it could on the specialprefs page where on-wiki javascript
is disabled for (I thought) the same concern.


Version: unspecified
Severity: enhancement
URL: http://i39.tinypic.com/29c2i2v.png
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=68521

Details

Reference
bz22929

Event Timeline

bzimport raised the priority of this task from to Normal.
bzimport set Reference to bz22929.
bzimport created this task.Mar 23 2010, 2:03 PM
IAlex added a comment.Apr 4 2010, 4:53 PM

Changing product/component to MediaWiki extensions/Gadgets since that's the Gadgets that should check $out->isUserJsAllowed() in wfGadgetsBeforePageDisplay().

daniel added a comment.Apr 6 2010, 8:43 PM

$out->isUserJsAllowed() always returns false for the cirtical pages, which is good. But it also returns false if $wgAllowUserJs is false, which is bad. it would mean that Gadgets don't work on wikis with user JS disabled.

Gadgets are more like site JS, not so much like user JS. So, $out->isUserJsAllowed() does not seem to be the right function to call.

The only alternative I see is to add more hard-coded checks in the gadgets extension. Which also sucks. Ideas?

daniel added a comment.Apr 6 2010, 9:01 PM

fixed in r64670 by adding hard-coded checks for Special:Userlogin and Special:Resetpass. Leaving a nicer solution for later.

M8R-cyc3n3 wrote:

Gadgets are more like site JS, not so much like user JS. So,
$out->isUserJsAllowed() does not seem to be the right function to call.

I guess you could add something like $out->isWikiJsAllowed()
which is basically:

/* do we load js from a wiki page under any circumstance */
return $wgAllowWikiJs && !($title->isCriticalPage());

And $out->isUserJsAllowed() would then become:

return $wgAllowUserJs && $out->isWikiJsAllowed();

In any case I was had hoped r64670 would be a candidate
for expedited scapping.

M8R-cyc3n3 wrote:

i don't know how new it is, but it just occurred to me that [[Special:Changepassword]] is now a separate page (!) so
we'll want to disable gadgets there too.

Should we perhaps not have custom js/css on any special pages then white-list
the ones it should be available?

M8R-cyc3n3 wrote:

Should we perhaps not have custom js/css on any special
pages then white-list the ones it should be available?

that seems a little rash as most of the js i've written and
used has been to enhance the functionality of special pages.

pages which leave the server while already containing any
element like <input name="wpPassword"> should refrain from
loading on-wiki javascript. seems like a no-brainer.

yeah, no solution is perfect, and a password prompt could be
added in post-production to potentially any page byOnloadHook
orCrook but i'd more likely suspect something is amiss than
remit my password for no apparent reason.

overlordq wrote:

I would agree with p858snake, that white listing instead of blacklisting would be a better solution.

happy.melon.wiki wrote:

Nicer implementation dones in r81524.