Page MenuHomePhabricator

Security review for PageNotice extension
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

Displays a wikitext message at the top and bottom of every page in a namespaces and/or specific pages.

NOTE: On Wikimedia sites, we would operate this extension with $wgPageNoticeDisablePerPageNotices set to true. The extension would only be used to provide notices on a per-namespace basis.

Description of how the tool will be used at WMF

To be deployed initially at English Wiktionary, and then at other wikis as requested by individual communities.

Dependencies

None.

Has this project been reviewed before?

No.

Working test environment

The extension is trivial to install on your local development instance of MediaWiki.

Post-deployment

Will be maintained by @TTO as required.

Event Timeline

TTO created this task.Aug 3 2019, 3:13 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 3 2019, 3:13 AM
Erutuon added a subscriber: Erutuon.Aug 3 2019, 3:25 AM
sbassett triaged this task as Low priority.Aug 8 2019, 4:30 PM

Change 529168 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/PageNotice@master] Adding phan-taint-check support via extra field

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

sbassett added subscribers: Reedy, sbassett.EditedAug 8 2019, 8:59 PM

Hey @TTO and @Victar -

I just had a quick look at master - no need to security-protect this task. Some thoughts:

  1. I added support in composer.json for phan-taint-check in this patch set. Feel free to merge if you'd like. While it's not a requirement to run phan-taint-check in CI, it's proven to be a fairly valuable, automated security tool.
  2. The quibble, phan and secheck dockers are happy.
  3. Dependencies seem fine within package.json and composer.json.
  4. I didn't complete any exhaustive performance checks. The extension is pretty simple though and I'm not seeing anything that should obviously impact performance from a security standpoint. Of course I'd defer to the Performance-Team as the subject matter experts here. And possibly recommend running fresnel against the entire extension.
  5. As an optional hardening measure, the values of $name and $ns could probably be further validated when building $header, $nsheader, $footer and $nsfooter. But $ns gets type-forced other places and msg() should be fairly robust at handling arbitrary values.
  6. A possible defacement attack might be if someone were able to alter pages under the MediaWiki messages namespace. They could then expose malicious content to a large number of users. These are well-protected on most wikis; I just wanted to mention this here for completeness' sake.

The Security-Team has our security reviews scrum weekly on Tuesdays, so I'll leave this task open until at least then (August 13th) in case @Reedy or anybody else wanted to weigh in.

sbassett claimed this task.Aug 8 2019, 8:59 PM
sbassett moved this task from Backlog to Awaiting remediation on the Security-Team-Reviews board.

Change 529168 merged by jenkins-bot:
[mediawiki/extensions/PageNotice@master] Adding phan-taint-check support via extra field

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

TTO added a comment.Sun, Aug 18, 3:05 AM

Re number 6, there are already many avenues to deface the wiki if you have edit access to the MediaWiki: namespace. Editing MediaWiki:Sitenotice would have a similar effect as creating a page notice. I don't think the fact that this extension adds another avenue is of any special concern.

The Security-Team has our security reviews scrum weekly on Tuesdays, so I'll leave this task open until at least then (August 13th) in case @Reedy or anybody else wanted to weigh in.

Any other comments?

sbassett closed this task as Resolved.Mon, Aug 19, 2:01 PM

Re number 6, there are already many avenues to deface the wiki if you have edit access to the MediaWiki: namespace. Editing MediaWiki:Sitenotice would have a similar effect as creating a page notice. I don't think the fact that this extension adds another avenue is of any special concern.

Yep, that's fine.

Any other comments?

Not from me. The Security-Team would assign a low risk for the deployment of this extension for now. I'll go ahead and resolve this task for now.

TTO added a comment.Fri, Aug 23, 5:59 AM

Thanks. To be 100% clear, does that mean that this extension has successfully passed its security review?

Hey @TTO -

The Security-Team is trying to get away from providing a "pass" or "thumbs up" for code during security reviews, as it assumes a level of accountability on our part which we cannot sustain. So we are adopting the more standard system of risk classification and risk ownership for our security reviews. This entails us performing a risk analysis during the review process and then assigning and communicating a level of risk to the requesters/owners of the code. The levels of risk we're using within our analyses are:

Risk Level
Critical
High
Medium
Low

These would have different implications around the likelihood of an extension getting deployed, etc. but obviously a low (or no) risk as the outcome of a security review would be considered fairly safe.