Page MenuHomePhabricator

CVE-2022-41765: HTMLUserTextField exposes existence of hidden users
Closed, ResolvedPublicSecurity

Description

At Miraheze, we use the UserTextField to allow users to assign wikis to a user on CreateWiki. See https://github.com/miraheze/CreateWiki/blob/master/includes/CreateWiki/SpecialCreateWiki.php#L36

When I attempt to input a locked & hidden user, while autocomplete doesn't suggest it, it still succeeds perfectly fine if I enter it manually and submit the form.

This seems to be a info leak to me. I discussed with @AntiCompositeNumber on IRC too

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

While this could be handled in downstream code, I think it would make more sense (and prevent accidental disclosures) to handle it by default in HTMLUserTextField, when checking for user existence. That would make the default behavior consistent with other pages like Special:Contribs.

Not sure if it was clear but Miraheze use CentralAuth. Not sure if that is the issue or just the forms system in general.

So addressing this within HTMLUserTextField would likely involve adding a check of $user->getBlock()->getHideName() within this conditional block, though not sure if it makes more sense in the if or elseif, given the slight differences in error messages.

mmartorana changed the task status from Open to In Progress.Jun 13 2022, 10:03 AM
mmartorana triaged this task as Low priority.
mmartorana changed Risk Rating from N/A to Low.

@mmartorana: hi, you changed this to 'In Progress' - who exactly is working on this?

So addressing this within HTMLUserTextField would likely involve adding a check of $user->getBlock()->getHideName() within this conditional block, though not sure if it makes more sense in the if or elseif, given the slight differences in error messages.

htmlform-user-not-exists sounds like the correct one in this case I think.

Hi @Majavah, in progress means that we are actively processing this task, not necessarily that we are working on a patch.

In this case, since this pertains MediaWiki core, Platform Engineering and Editing-team should be in charge of that.

Can you explain why Editing-team? Sure, we would be able to help if there's somehow no one else capable of resolving this, but I am very confused why anyone would think we're responsible for it…

Hi @matmarex - we may have made a mistake in our assignment, but the reasoning was likely due to the fact that you are part of the Editing-team.

Could you please point me to someone capable of resolving this?

Thank you

I imagine any engineer would be capable, @sbassett has pretty much already solved it in his comment T309894#7987574.

But if you're looking for someone responsible for it, then there is no one. In the past, members of the Security team would have resolved security issues with no maintainers like this.

I'm happy to write the patch, but I want to make it clear that it's in my volunteer capacity. Editing-team has never worked on this code, and it's not an area that it would make sense for us be responsible for.

Security patch:

There are several non-security bugs in the existing logic though: T177329, T311948. I submitted a patch to fix those to Gerrit: https://gerrit.wikimedia.org/r/c/813727. If it is merged, this is the security patch to use instead:

The behavior can be most easily tested on Special:EmailUser.

Hey all -

I just deployed @matmarex's v1 patch from above (thanks again) to 1.39.0-wmf.19. Tested with enwiki:Special:EmailUser on mwdebug1002 and then in prod and all looks good - it seems to be throwing the correct error message for hidden users now. No related errors in logstash either. If anybody sees anything off about it, let me know. Otherwise, this task/patch should be held for the next mediawiki security release (T311775).

But if you're looking for someone responsible for it, then there is no one. In the past, members of the Security team would have resolved security issues with no maintainers like this.

Just a follow-up note on this - yes, this is true in that members of the Security Team were traditionally tasked with "resolving" security issues reported for any Wikimedia codebase. Unfortunately, given the enormous backlog of security issues that accumulated under this policy, this was never a sustainable policy and is quite far from the industry standard of code maintainers owning the risk for their security issues. I think we've failed as a team in communicating this concept to the Foundation and Community over the past several years, but this is going to change in the very near future when our team begins pushing smaller, more frequent updates out over multiple communications channels. It is hoped that this will better set expectations regarding responsibilities, policies and workflows related to the current incarnation of the Security Team.

Hey all -

I just deployed @matmarex's v1 patch from above (thanks again) to 1.39.0-wmf.19.

Thanks for the deployment. I ran into your patch when doing another, unrelated deployment (the applied version of the patch did not have any prefix identifying it as a secpatch, so I had to check whether it is an unmarked sec patch, or uncommitted code). Since it turns out to be a sec patch, I slightly changed the patch file to include the SECURITY: prefix, to avoid confusing others. FTR, this is the currently applied version:

But if you're looking for someone responsible for it, then there is no one. In the past, members of the Security team would have resolved security issues with no maintainers like this.

Just a follow-up note on this - yes, this is true in that members of the Security Team were traditionally tasked with "resolving" security issues reported for any Wikimedia codebase. Unfortunately, given the enormous backlog of security issues that accumulated under this policy, this was never a sustainable policy and is quite far from the industry standard of code maintainers owning the risk for their security issues. I think we've failed as a team in communicating this concept to the Foundation and Community over the past several years, but this is going to change in the very near future when our team begins pushing smaller, more frequent updates out over multiple communications channels. It is hoped that this will better set expectations regarding responsibilities, policies and workflows related to the current incarnation of the Security Team.

I understand, and I don't mean to say that you should be responsible for it, I agree that it's a bad situation to have a "security team" fixing all the issues in other people's code. However, there really is no one else responsible for it, and I'm afraid that just communicating that won't solve that issue – I think it's a management/leadership problem. I would actually enjoy doing more of this work, but right now I feel like I'm neglecting my more "official" responsibilities when I do.

Thanks for the deployment. I ran into your patch when doing another, unrelated deployment (the applied version of the patch did not have any prefix identifying it as a secpatch, so I had to check whether it is an unmarked sec patch, or uncommitted code). Since it turns out to be a sec patch, I slightly changed the patch file to include the SECURITY: prefix, to avoid confusing others. FTR, this is the currently applied version:

Ok, thanks for fixing that and cleaning everything up on deployment.

I understand, and I don't mean to say that you should be responsible for it, I agree that it's a bad situation to have a "security team" fixing all the issues in other people's code. However, there really is no one else responsible for it, and I'm afraid that just communicating that won't solve that issue – I think it's a management/leadership problem. I would actually enjoy doing more of this work, but right now I feel like I'm neglecting my more "official" responsibilities when I do.

Agreed. I think some combination of efforts will be necessary to enact any real change. Better communication and expectation-setting on the part of the Security-Team is one small thing we can easily change which could have an impact. I think some members of our team would also like to help write and CR more security patches, but we'll need to find a middle-ground between not doing any of this work and doing all of it, neither of which are reasonable positions IMO.

(https://gerrit.wikimedia.org/r/c/813727 was merged, so you'll need to v2 patch for next WMF branch)

(https://gerrit.wikimedia.org/r/c/813727 was merged, so you'll need to v2 patch for next WMF branch)

Ok, thanks for the heads up. Looks like this didn't make 1.39.0-wmf.21, but I'll go ahead and update the patch on deployment for next week's train.

Sadly, it looks like the v2 patch fell off of production releases for a bit, but is now back on wmf.23 and wmf.25 and redeployed (sal 1, sal 2).

Security patch:

There are several non-security bugs in the existing logic though: T177329, T311948. I submitted a patch to fix those to Gerrit: https://gerrit.wikimedia.org/r/c/813727. If it is merged, this is the security patch to use instead:

The behavior can be most easily tested on Special:EmailUser.

TLDR here/for confirmation...

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/813727 made it into REL1_39 (and master), so it should get

, which was superceded by

For <= REL1_38, we should use

(or some variant thereof, modifying as needed for backporting) ?

01-T309894.patch (F35321391) doesn't apply for me on master. It looks identical to my v1 patch. I wonder if you uploaded the right file?

01-T309894.patch (F35321391) doesn't apply for me on master. It looks identical to my v1 patch. I wonder if you uploaded the right file?

I didn't upload any files, I just reused the linked files from above..

Oh, my bad, I didn't notice where it came from.

It looks like it was a replacement for the v1 patch with a tweaked commit message:

Thanks for the deployment. I ran into your patch when doing another, unrelated deployment (the applied version of the patch did not have any prefix identifying it as a secpatch, so I had to check whether it is an unmarked sec patch, or uncommitted code). Since it turns out to be a sec patch, I slightly changed the patch file to include the SECURITY: prefix, to avoid confusing others. FTR, this is the currently applied version:

So, I think the correct procedure is: use the original patches on whichever branches they apply to, but change the commit message of both to read:

SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

…as I used the wrong convention for the prefix.

Yeah, I usually fix up descriptions etc when I apply the patches.

It's just trying to confirm upfront which patches should be applied, especially when we have multiple versions flying around.

Security patch:

There are several non-security bugs in the existing logic though: T177329, T311948. I submitted a patch to fix those to Gerrit: https://gerrit.wikimedia.org/r/c/813727. If it is merged, this is the security patch to use instead:

The behavior can be most easily tested on Special:EmailUser.

^ I was going based on that, as https://gerrit.wikimedia.org/r/c/813727 had been merged, and then the superceded one...

Reedy renamed this task from HTMLUserTextField exposes existence of hidden users to CVE-2022-41765: HTMLUserTextField exposes existence of hidden users.Sep 29 2022, 5:28 PM
Reedy added a subscriber: gerritbot.

Change 836892 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@REL1_35] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836897 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@REL1_37] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836901 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@REL1_38] HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836906 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@REL1_39] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836911 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@master] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836892 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836897 merged by Reedy:

[mediawiki/core@REL1_37] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836901 merged by Reedy:

[mediawiki/core@REL1_38] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836911 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Change 836906 merged by jenkins-bot:

[mediawiki/core@REL1_39] SECURITY: HTMLUserTextField: Treat hidden users as unregistered if current user can't view them

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

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Tue, Nov 1, 6:07 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".