Page MenuHomePhabricator

Security review for Extension:ExternalGuidance
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

Wikipedia content gets exposed externally in different ways, and this project intends to make it easy for people to return to the origin, and contribute to it.

Description of how the tool will be used at WMF

Users accessing content that originates from a Wikimedia project would be made aware that it is different than the original content, and will be shown a guidance to make contributions to it.

Dependencies

None.

Has this project been reviewed before?

No.

Working test environment

https://eg.wmflabs.org/wiki/Main_Page

Post-deployment

name of team responsible for tool/project after deployment and primary contact
Language

Details

Related Gerrit Patches:

Event Timeline

Arrbee created this task.Jan 18 2019, 10:15 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 18 2019, 10:15 AM
Arrbee added a project: Security-Team-Reviews.
Arrbee added a subscriber: KartikMistry.
Aklapper updated the task description. (Show Details)Jan 18 2019, 1:00 PM

Just wondering, is there a reason for the doing the new SiteMapper stuff, instead of using core's WikiMap (and related) class? (I haven't looked at WikiMap in a while, so the answer might just be that WikiMap really sucks)

Just wondering, is there a reason for the doing the new SiteMapper stuff, instead of using core's WikiMap (and related) class? (I haven't looked at WikiMap in a while, so the answer might just be that WikiMap really sucks)

WikiMap is mainly for dealing with wikis in a Wiki farm like setup while the SiteMapper has no such assumption. This SiteMapper code is originally taken from ContentTranslation. It helps a lot when you want to treat your local wiki as one of the sister wiki of wikipedia , fetch content from one of those language wiki, use APIs etc, by just having a domain URL template. Since we need a counter part of this in javascript, these templates helps to easity construct api URLs.

To help understanding what the project does,access our labs instance where this extension is deployed using google translate. You should see broken edit links, and page actions removed by a contribution invite banner. Which will take you to one of our wikis, explaining steps to contribute. (Currently that target wiki is same labs wiki due to constraints in testing)

sbassett triaged this task as Normal priority.Feb 4 2019, 3:09 PM

@santhosh -

This is still technically unassigned, but I had a cursory look at it. Some initial findings:

  1. package.lock looks fine from a quick run of security-checker.
  2. I assume any potential privacy (and related) issues w/ the Google MT service are already addressed by WMF's agreement with Google for cxserver.
  3. In SiteMapper::getPageURL, it looks like $title isn't being validated/sanitized and is just replacing $2 within //$1.wikipedia.org/wiki/$2. Given that Html::rawElement is used within specials/SpecialExternalGuidance.php and it's populating $sourcePage from the request via getVal(), I'm thinking there could be a potential for injection here, unless I'm just missing where this is being sent to the parser or sanitized, etc.
  4. I'm not seeing anything egregious within the JS modules under modules/, but I (or whomever officially gets assigned this review) should definitely spend more time reviewing those.
Reedy added a subscriber: Reedy.Feb 5 2019, 8:32 PM

@santhosh -
This is still technically unassigned, but I had a cursory look at it. Some initial findings:

  1. package.lock looks fine from a quick run of security-checker.
  2. I assume any potential privacy (and related) issues w/ the Google MT service are already addressed by WMF's agreement with Google for cxserver.
  3. In SiteMapper::getPageURL, it looks like $title isn't being validated/sanitized and is just replacing $2 within //$1.wikipedia.org/wiki/$2. Given that Html::rawElement is used within specials/SpecialExternalGuidance.php and it's populating $sourcePage from the request via getVal(), I'm thinking there could be a potential for injection here, unless I'm just missing where this is being sent to the parser or sanitized, etc.
  4. I'm not seeing anything egregious within the JS modules under modules/, but I (or whomever officially gets assigned this review) should definitely spend more time reviewing those.

Following up on number 3... I added some use statements in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ExternalGuidance/+/488111/ which is causing Phan to complain about the $out->addHTML( Html::rawElement( blocks as Scott has highlighted above

Obviously the patch doesn't add them, but it does expose them as it gives Phan some more context as to the types (as they didn't exist in the namespace, they're in the global namespace)

Change 488314 had a related patch set uploaded (by Santhosh; owner: Santhosh):
[mediawiki/extensions/ExternalGuidance@master] SpecialExternalGuidance: Validate the language codes and title parameters

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

  1. In SiteMapper::getPageURL, it looks like $title isn't being validated/sanitized and is just replacing $2 within //$1.wikipedia.org/wiki/$2. Given that Html::rawElement is used within specials/SpecialExternalGuidance.php and it's populating $sourcePage from the request via getVal(), I'm thinking there could be a potential for injection here, unless I'm just missing where this is being sent to the parser or sanitized, etc.

In https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ExternalGuidance/+/488314 I added validation for language codes and source page param before all these code execution. That should take care of this issue.

Change 488314 merged by jenkins-bot:
[mediawiki/extensions/ExternalGuidance@master] SpecialExternalGuidance: Validate the language codes and title parameters

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

santhosh moved this task from Backlog to In Review on the ExternalGuidance board.Feb 7 2019, 5:45 AM

@sbassett, @Reedy Are you planning to take another look on this? Or are we done? All the patches related to your comments are merged now. Thanks!

@santhosh - I think we'd like to (or at least I'd like to) review this code a bit more. We didn't have our weekly security review scrum during All-hands or the week after, so this is still kind of in limbo at the moment though we wanted to get you some initial feedback. We should be able to schedule a full review by next Tuesday (2/12) when we have our next scrum. Is there a more specific launch date other than "February 2019" you had in mind that would make this more pressing?

@santhosh - I think we'd like to (or at least I'd like to) review this code a bit more. We didn't have our weekly security review scrum during All-hands or the week after, so this is still kind of in limbo at the moment though we wanted to get you some initial feedback. We should be able to schedule a full review by next Tuesday (2/12) when we have our next scrum. Is there a more specific launch date other than "February 2019" you had in mind that would make this more pressing?

We plan to deploy extension on 02/12.

sbassett added a comment.EditedFeb 11 2019, 3:32 PM

@santhosh, @KartikMistry -

These changes look good and phan-taint-check likes them. I still want to review the aforementioned JS a bit more in depth, but I personally don't think that should stop your deployment timeline of 2/12.

sbassett claimed this task.Feb 12 2019, 3:42 PM

Update: I did have a more in-depth look at the JavaScript under modules/ yesterday - again, I didn't find anything particularly concerning. I still wanted to pen-test it a bit further (was having some vagrant issues yesterday) so if I find anything from that, I'll post it here. Additionally, I see that the extension was deployed to simple.wikipedia.org and id.wikipedia.org earlier today, per T213076.

@sbassett we are doing tests now, and sometime early next week will have more clarity in terms of the timeline for deployment on enwiki. Approximately, a two week window is what we are looking at for the next deployment. Hope this is helpful for your task prioritization.

@Arrbee - that's good to know, thanks. Again, I think this code is looking pretty good overall with @santhosh's recent patch set. I was merely planning to perform some additional (kind of optional) analysis and pen-testing of the code for good measure. It shouldn't really be anything that prevents a full production deployment.

sbassett closed this task as Resolved.EditedFeb 28 2019, 7:55 PM

All-

I've completed my security review. Everything looks good to me, though a new dev dependency issue was found: a ReDoS in braces < 2.3.1. Here's the dep tree:

─┬ stylelint-config-wikimedia@0.5.0
  └─┬ stylelint@9.6.0
    └─┬ micromatch@2.3.11
      └── braces@1.8.5

npm classifies this as a low severity issue, so it might be best to just open an issue for the project. Certainly nothing that would halt any future deployments of ExternalGuidance.

Update: filed.

I'll go ahead and resolve this task now.

KartikMistry moved this task from In Review to Done on the ExternalGuidance board.Mar 1 2019, 8:02 AM
sbassett moved this task from Frozen to Archive on the Security-Team-Reviews board.