Page MenuHomePhabricator

Security Review For viewing Special:Homepage as rendered for other users
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

Special:Homepage is a special page we are planning to deploy to our (Growth team's) target wikis. You can see mockups here or interact with the work-in-progress code on beta labs if you're logged in.

On the homepage, users can view their impact, get help, set an email address or send verification email if not verified, create their user page, view a tutorial, and contact their mentor.

We are asking for security review and guidance only for a specific feature of the homepage, which is to allow a privileged user to be able to view Special:Homepage as it renders for another user (some notes also exist here). We currently do something similar to this for the impact module of the homepage (see example on beta), but now we want to expand this to the entire Special:Homepage.

There is no code to review for the above feature yet, creating this task to organize discussion about the approach, which is described below.

Proposed approach

The implementation we are proposing is:

  1. Create privileged group on the wikis we are deploying to (Czech, Korean, Vietnamese, possibly Arabic)
  2. Add users who should be able to view Special:Homepage as others to that group (currently would probably just be members . of the Growth-Team, but perhaps the ambassadors who work with us in these wikis as well)
  3. If privileged users go to Special:Homepage/{username}, in SpecialHomepage.php code we would:
    1. Check if the context user is in the privileged group and is requesting to view someone else's page
    2. Create a derivative context based on loading username from the parameters
    3. Set a flag on the server side that we're in view-as-other mode
    4. Set a flag in client-side code that we're in view-as-other mode, so that event logging is switched off for example
    5. Ensure that user email does not display in the account completion module if we are in "view-as-other" mode.
      1. Question: Do we need to hide the account email verification status when in view-as-other mode?
    6. The user's mentor is stored as a preference on a per user basis. Should this not be shown?
    7. Two modules (help, and mentor) allow the user to post a question to the help desk and their mentor respectively, we need to ensure that these modules are in read-only mode. This probably has to be done on the client side.

Update: We can also consider striking the privileged users bit, and develop this feature such that any user could view any other user's Special:Homepage, as long as any user-specific private information is removed from the output.

Description of how the tool will be used at WMF

Mainly @MMiller_WMF will be using the view-as-other feature to verify that the Homepage renders as we intend it to.

Dependencies

none

Has this project been reviewed before?

I think so, but I can't find the task.

Working test environment

It's on beta labs, or you can use the growthexperiments role in Vagrant.

Post-deployment

Growth-Team, @Catrope, @SBisson, @kostajh

Event Timeline

kostajh created this task.Mar 26 2019, 3:46 PM
Restricted Application added subscribers: revi, Aklapper. · View Herald TranscriptMar 26 2019, 3:46 PM
sbassett triaged this task as Low priority.Mar 26 2019, 5:18 PM

Also, if you all would like to do a quick call or IRC chat to get additional clarifications, please let me know!

sbassett claimed this task.Mar 26 2019, 5:24 PM

@kostajh - Yes, maybe it would be good to have a quick chat. I'm specifically curious if you'd like us to review everything committed since T207798 or if there are a more specific set of commits we should focus on for the features described above.

kostajh updated the task description. (Show Details)Mar 26 2019, 6:06 PM

I'm specifically curious if you'd like us to review everything committed since T207798 or if there are a more specific set of commits we should focus on for the features described above.

There's no code specific to the view-Special:Homepage-as-some-other-user feature to review just yet. If the "Proposed approach" section seems like it would be OK, we can start coding and then tag you for review after.

FWIW, given how much press https://arstechnica.com/information-technology/2018/09/50-million-facebook-accounts-breached-by-an-access-token-harvesting-attack/ has been getting, view-as-other-people features make me nervous.

The possibility here that most immediately comes to mind would be if the wiki has a special page with getCacheTTL set to 0 (e.g. Like with Extension:Petition or any special page when $wgMiserMode is set to false. There are no such special pages in core when miser mode is enabled), and that the homepage will render some some stuff where $parser->getUser() gets the other user, thus leaking CSRF tokens. In practise, this probably currently can't happen (Primarily because, wfMessage() wouldn't use your custom context at all, if you go via $this->msg [where this is the SpecialPage], it uses the DerivitiveContext, but not the User part, and the user is still coming from $wgUser [in MessageCache]. If you're using $this->getOutput->addWikiTextAsInterface [or a different OutputPage parsing method. Which the current extension does not use, but it is a common method people call in SpecialPages], this does use the context from RequestContext, and could output csrf tokens in the special page transclusion case. However, DerivativeContext will generally use the OutputPage from the parent context unless explicitly overridden. This means that OutputPage will still have the parent context instead of the DerivativeContext we got the OutputPage from. This borderline seems like a bug to me, but does mean that $myDerrivContext->getOutput()->addWikiTextAsInterface(...) will be using the User and other Context stuff from the parent context and not $myDerrivContext. So it can't leak tokens that way, but that's mostly relying on a lot of implementation details. Of course there's also the possibility of something else using the DerivativeContext in an unsafe way I didn't think of

thanks for your review!

FWIW, given how much press https://arstechnica.com/information-technology/2018/09/50-million-facebook-accounts-breached-by-an-access-token-harvesting-attack/ has been getting, view-as-other-people features make me nervous.

yes, acknowledged.

this does use the context from RequestContext, and could output csrf tokens in the special page transclusion case.

We are not proposing to allow transclusion of this page although we have talked about transclusion for one of its modules, Special:Impact


I'm not sure I fully understood everything in your comment but my takeaway is that using DerivativeContext is not necessarily safe to prevent leaking sensitive information for the "other" when in view-as-other mode

Note that we use DerivateContext to render Special:Impact for another user (see the code here), but from what you're saying it sounds like for view-as-other mode, we should *not* inject a IContextSource to the Impact class (and presumably also the other modules, if we implemented view-as-other for them), but instead provide an ability to set or inject User, MessageLocalizer, Language, and Config classes.

That said, could you please look at the Special:Impact code and the Impact class to see if anything there strikes you as unsafe?

Hey @kostajh - just to follow up on this a bit more, I'd like to propose this to the Security-Team and provide a concept review. This will probably involve some measurement of risk for the "view another user's homepage" feature (based upon @Bawolff's concerns and any other potential issues) and the ownership of said risk by the growth team.

kostajh updated the task description. (Show Details)Apr 2 2019, 8:24 PM
kostajh added a subscriber: kaldari.

@sbassett -- in a conversation with @JBennett the other week, we talked about separating out the ability to view any user's Impact module via Special:Impact from the ability to see their whole homepage. Seeing their Impact module is our higher priority, and the thing we want to make sure is okay soonest. @kostajh can fill in details about the distinction if needed.

@MMiller_WMF, @JTannerWMF - the Security-Team discussed this today. We had a couple of initial questions for you:

  1. For how long is this feature intended to exist (specifically the read-only view of the Special:Impact page)? A month? A year? Indefinitely?
  2. Do you (and any additional privileged users) have 2FA enabled on all wikis where this feature would be enabled?

Finally, we have a few suggestions for some additional technical controls for the Growth Team to implement regarding this feature, which are currently under WMF-Legal review. Once that review is complete (and they potentially prescribe additional controls) we can provide them here for you to review, along with our analysis of the overall risk. I'd hope we could get this turned around in a week or less for you.

Myself, @sbassett @Catrope @kaldari and @kostajh had a follow up meeting about this yesterday. Scott is planning to talk to John to get an okay and risk assessment on this project.

The following things were discussed:

  • The Task recommendation module is being put on hold and should be considered for this release
  • We are looking for feedback on Special:Homepage and the Impact Module (mentioned in the description of this ticket)
  • We made clear the Special:Homepage will only show already public information and a minimal amount of it

When asked for examples of the Impact Module the following were provided:
https://en.wikipedia.beta.wmflabs.org/wiki/Special:Impact/Krenair for example, or https://en.wikipedia.beta.wmflabs.org/wiki/Special:Impact

Additionally, this one is about the view-homepage-as-other-user task, which we just put in the fridge. https://phabricator.wikimedia.org/T217281 But in there, we split off the Special:Impact part of it, and that's discussed at https://phabricator.wikimedia.org/T217281#5041219 and is implemented in this patch https://gerrit.wikimedia.org/r/#/c/497841/

@JTannerWMF - just to clarify your first bullet point - "The Task recommendation module is being put on hold and should NOT be considered for this release", correct?

A few small clarifications:

  • The "view Special:Homepage of another user" feature is on hold and should not be considered by Security team for now
  • We are looking for feedback on viewing and interacting with Special:Homepage and viewing Special:Impact for yourself or for another user (Special:Impact/{Username}). Both can be tested on beta labs.
  • Special:Homepage mostly shows publicly accessible information to the user who is viewing it, but the user can also see:
    • their email confirmation status (no email, or confirmation pending)
    • their mentor (saved as a user ID in preferences)
    • Whether the user has visited the tutorial page defined for the wiki

And yes @sbassett the task recommendation module hasn't been developed or scoped so there's nothing to review there yet.

sbassett added a comment.EditedApr 15 2019, 3:23 PM

@JTannerWMF, @kostajh, @Catrope - the Security-Team just had a chat about this. We're fine with the Special:Impact administrative view by itself. We will classify that as an "informational" risk, which wouldn't require an entry within our risk register. If/when the Growth Team is ready to proceed with the administrative view of the Special:Homepage feature (as initially proposed) we can pick up where we left off on that specific feature. If this sounds good and there are no further questions from the Growth Team on this, I can go ahead and resolve this task.

(note: the Special:Impact view does not require any additional assessment from the Security-Team or WMF-Legal for now)

sbassett closed this task as Resolved.Apr 16 2019, 7:30 PM
sbassett moved this task from Frozen to Archive on the Security-Team-Reviews board.