Page MenuHomePhabricator

Personal and site-wide CSS and JavaScript is loaded on Special:PasswordReset
Closed, ResolvedPublic

Description

Personal and site-wide CSS and JavaScript is loaded on Special:PasswordReset. Is this not a security issue?

Event Timeline

Nirmos created this task.Apr 13 2018, 5:56 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 13 2018, 5:56 AM
Reedy added a subscriber: Reedy.Apr 13 2018, 8:56 AM

SpecialUserLogin extends LoginSignupSpecialPage... SpecialPasswordReset extends FormSpecialPage

LoginSignupSpecialPage calls OutputPage::disallowUserJs... SpecialPasswordReset and FormSpecialPage don't.

Ladsgroup added a subscriber: Ladsgroup.

Is this good for now?

Is this good for now?

Maybe I should call that method in alterForm instead of construct. Can't say for sure tbh.

Urbanecm triaged this task as Low priority.Nov 12 2019, 8:52 AM

@Ladsgroup - patch above still applies to MW 1.35. I'm personally fine with this as a stop-gap measure or even if it becomes more of a permanent mitigation. Though I'm not really sure there's a demonstrated security issue here. Yes, certain user css/js can do unintentional or bad things on wiki pages where it's run, but that's a fundamental part of such functionality existing in the first place. Anyhow, I'm happy to deploy this as a core security patch and backport it to supported release branches, though IMO, it might barely meet the security patch threshold.

sbassett moved this task from Incoming to Watching on the Security-Team board.

@Ladsgroup - patch above still applies to MW 1.35. I'm personally fine with this as a stop-gap measure or even if it becomes more of a permanent mitigation. Though I'm not really sure there's a demonstrated security issue here. Yes, certain user css/js can do unintentional or bad things on wiki pages where it's run, but that's a fundamental part of such functionality existing in the first place. Anyhow, I'm happy to deploy this as a core security patch and backport it to supported release branches, though IMO, it might barely meet the security patch threshold.

So the biggest part of the problem is that people can steal others' email addresses that is considered private data. It's not as bad as stealing their passwords (that's what I initially thought, it's PasswordReset) but I realized it's email address only. I tested it locally and it works.

Maybe I should call that method in alterForm instead of construct. Can't say for sure tbh.

[[ https://codesearch.wmflabs.org/core/?q=disallowUserJs%5C(%5C)&i=nope&files=&repos= | Most other specials seem to call it in execute() ]] though SpecialPasswordReset.php doesn't call execute() right now. I'm not sure there's anything wrong with calling disallowUserJs() within the constructor, though I'm sure that could be modified to look more like what is done in SpecialChangeEmail for consistency's sake. Anyhow, whatever you want to do, I'd like to try to deploy this during the security deployment window this Monday, if possible.

sbassett moved this task from Backlog to Waiting on the user-sbassett board.

Maybe I should call that method in alterForm instead of construct. Can't say for sure tbh.

[[ https://codesearch.wmflabs.org/core/?q=disallowUserJs%5C(%5C)&i=nope&files=&repos= | Most other specials seem to call it in execute() ]] though SpecialPasswordReset.php doesn't call execute() right now. I'm not sure there's anything wrong with calling disallowUserJs() within the constructor, though I'm sure that could be modified to look more like what is done in SpecialChangeEmail for consistency's sake. Anyhow, whatever you want to do, I'd like to try to deploy this during the security deployment window this Monday, if possible.

Okay, I made another patch that makes me feel better (I was worried that construct would have been ran in other contexts like in Special:SpecialPages)

Thanks, @Ladsgroup. Patch applies and tests fine locally on MW master. Will deploy shortly.

sbassett closed this task as Resolved.Dec 9 2019, 10:45 PM
sbassett assigned this task to Ladsgroup.
sbassett moved this task from Pending deployment / release to Done on the acl*security board.

Deployed to wmf.5 and wmf.8. Tested on enwiki and all looks good to me and nothing bad happening in logstash:FatalMonitor. I'm going to resolve for now and make public. I don't think this merits a CVE (it's really more code-hardening IMO) but backports to supported release branches could certainly happen.

Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptDec 9 2019, 10:45 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".

Change 556073 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@master] Do not allow user scripts on Special:PasswordReset

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

Change 556074 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_34] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556075 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_33] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556076 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_32] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556077 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_31] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556077 merged by jenkins-bot:
[mediawiki/core@REL1_31] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556076 merged by jenkins-bot:
[mediawiki/core@REL1_32] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556075 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556074 merged by jenkins-bot:
[mediawiki/core@REL1_34] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556073 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Maintenance_bot moved this task from Incoming to Done on the User-Ladsgroup board.Dec 9 2019, 11:15 PM