Page MenuHomePhabricator

Exposed suppressed username via Special:Redirect
Closed, ResolvedPublic

Description

  • Problem: Special:Redirect/user/NNN does not check target user_id's suppressed status.
  • Step to reproduce :
    1. Go to Special:Redirect
    2. Make sure Lookup: type is User ID
    3. Entry Value: in optimal numbers. e.g. increase user_id until finding not listed entry at Special:ListUsers
  1. Directly link as [[Special:Redirect/user/SUPPRESSED_USER_ID]] replace SUPPRESSED_USER_ID for optimal numbers.
  2. Actual Results: Redirect to suppressed user's user page .
  3. Expected Results: Forbid redirecting to suppressed username.

Event Timeline

Rxy created this task.Aug 13 2019, 9:59 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 13 2019, 9:59 AM
Rxy added subscribers: sbassett, Reedy.

I wrote a patch for this ticket.

Rxy moved this task from Backlog to Security on the User-Rxy board.Aug 13 2019, 10:09 AM
Rxy added a subscriber: Ohgi.Aug 13 2019, 12:08 PM
sbassett triaged this task as High priority.Aug 13 2019, 2:44 PM
sbassett updated the task description. (Show Details)Aug 13 2019, 2:49 PM
sbassett updated the task description. (Show Details)
sbassett added a comment.EditedAug 13 2019, 7:18 PM

@Rxy - a couple of thoughts:

  1. So this patch just prevents a Special:Redirect based upon the numerical user_id of a hidden user, correct? I believe the User: pages of hidden users are still publicly-viewable if they exist (i.e. they don't display the User account "xxx" is not registered. error message.) The intention here is to throw an error if anyone tries to Special:Redirect to them, to prevent some information disclosure, correct?
  2. I'm not sure the hideuser permissions error message makes sense within the context of a Special:Redirect:
You do not have permission to block a username, hiding it from the public...

as it seems to be more directed at users with that permission as opposed to someone trying to visit a redirect. I agree that's the right permission to check but I might recommend something like this for throwing a more relevant error message:

throw new ErrorPageError( 'badaccess-group0', 'badaccess-group0' );

Otherwise, I think this seems fine and I can security-deploy it whenever, though honestly I'd view this more as a hardening measure that could probably be done in gerrit.

Rxy added a comment.Aug 13 2019, 10:43 PM
  1. Yes, It is correct. If who knew hideuser-ed user name, they can directly access to user page or they can check account existence via trying account creation. I guess no need to shown message User account "xxx" is not registered.. The patch is prevent leaks hideuser-ed account name who aren't know hideuser-ed account name by trying increase user_id.
  1. Here is Patch Set 2:

Thanks for reviewing.

PS2 looks good to me. I can try to deploy this later today or tomorrow, since there's no train this week.

I'm deploying this now through gerrit. Will make this task public once deployed.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 15 2019, 7:22 PM

Change 530441 merged by jenkins-bot:
[mediawiki/core@REL1_32] SECURITY: Add permission check for suppressed account

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

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/530440/ needs another CR+2 to tell Jenkins to merge - original gate-and-submit failed verification

Change 530443 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: Add permission check for suppressed account

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

Hmm, this is the second issue with revdel & Special:Redirect :( [The other one being T187638]. Guess we need to be careful with this page

Change 530440 merged by Jforrester:
[mediawiki/core@REL1_31] SECURITY: Add permission check for suppressed account

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

@Reedy - CVE requested. I'll post here when they send over the ID, then we can definitely resolve this task. Is there a specific date/time this week you were thinking about for the new release and announcements?

sbassett closed this task as Resolved.Sep 24 2019, 2:51 PM
sbassett claimed this task.
sbassett moved this task from Backlog / Other to Pending deployment / release on the Security board.

Why was this not held for the security release?

@Legoktm - I think there was some confusion on my part here as I had originally just done this as a code-hardening measure in gerrit (T230402#5416849) since it was some lightweight information disclosure, and for information findable in other ways via trivial User: namespace searches, our data dumps, etc. Then @Reedy mentioned getting a CVE for it in T225160, which I thought I'd just do for good measure. And we still aren't tracking this task on the current security release tracking bug: T225152. If we want to protect any security-related bug for core (and bundled extensions?) going forward, even low/info disclosure issues like this, then I can do that. Though we'd probably want to update our security release doc (1, 2) to emphasize this point or provide some basic guidance around what types of security patches should remain protected until an official security release.