Page MenuHomePhabricator

Get security reviews for PageAssessment extension done
Closed, ResolvedPublic

Description

Security reviews before getting extension deployed to production

Event Timeline

Niharika created this task.Dec 3 2015, 3:05 PM
Niharika claimed this task.
Niharika raised the priority of this task from to Normal.
Niharika updated the task description. (Show Details)
Niharika added a project: Community-Tech.
Niharika added subscribers: DannyH, kaldari, Niharika, Aklapper.
kaldari set Security to None.

Hi @NiharikaKohli, I looked at the master branch of https://gerrit.wikimedia.org/r/p/mediawiki/extensions/PageAssessments.git, and looks like there isn't much there yet. Or maybe this is just a really small extension? We'll probably need to do the review this week or early next week if we're going to complete it this quarter, so let us know if it's currently ready, or if there's a commit in gerrit we should review?

Also, can you point to any extra documentation we can look at to get an overview of what the extension does, and if you have a development version of the extension installed anywhere that we can look at?

Hi @NiharikaKohli, I looked at the master branch of https://gerrit.wikimedia.org/r/p/mediawiki/extensions/PageAssessments.git, and looks like there isn't much there yet. Or maybe this is just a really small extension? We'll probably need to do the review this week or early next week if we're going to complete it this quarter, so let us know if it's currently ready, or if there's a commit in gerrit we should review?

Hi Chris, thanks for looking into this. It's a small enough extension but there are two major commits yet to be merged in. I'm hopeful to have it complete by end of this week, or if not, Monday. I'll ping you to let you know when they're ready for review.

Also, can you point to any extra documentation we can look at to get an overview of what the extension does, and if you have a development version of the extension installed anywhere that we can look at?

There isn't any documentation yet. In brief, this extension takes article assessment data for wikiprojects as input via a parser function and stores it in a table. There's also a logging table to log every update.

Hi @csteipp, I have a patch in Gerrit: https://gerrit.wikimedia.org/r/259522 that's up for security reviews.

Description: This extension takes article assessment data for wikiprojects as input via a parser function and stores it in ExtensionData until parsing is complete because there can be multiple assessment functions on the same page. Then it fetches them and inserts, deletes or updates the database records according to the new data.
There's also a logging table to log every update.

Thanks!

Hi @NiharikaKohli,

I reviewed the extension with gerrit 259522 today. Two major blockers for deploying this:

  • PageAssessmentsBody::getAllProjects - sql injection in the way you're constructing the query with the page title. The page title Foo" OR 1="1 is completely valid.
  • The way that you're handling logging in a dedicated database table is concerning. As it is, someone can put something private/defamatory in one of the function parameters, and that is written directly into the log. If the revision is later suppressed, although the value will be deleted in page_assessments, it will stay in page_assessments_log. Since we typically replicate tables like this to toollabs, that would leave us with data in the database that we can't suppress. You have several options to address this:
    • Log to a filesystem log instead of DB table. It looks like you aren't displaying that information anywhere in the extension, so if the data is just for debugging, then I'd recommend just logging it. Logs are rotated after 90 days, so you're automatically compliant with our privacy policy.
    • Use the normal mediawiki logging functions (where we have deletion/suppression built in)
    • If you really need to have the logs in a db table and you can't use mediawiki's logging, you can build in your own deletion/suppression support, but that will be both a lot of work and something you'll need to work with the Security Team pretty closely on to make sure we get right.

@csteipp: Thanks for the review!

@NiharikaKohli: My suggestion is to drop the logging entirely for now since it isn't strictly necessary for an MVP. We can always add logging later after we figure out how to handle suppression.

DannyH moved this task from Bug backlog to Archive on the Community-Tech board.Jan 19 2016, 9:04 PM

This task is a bit difficult to follow. I see Chris' note about two major security blockers. Were those addressed?

This task is a bit difficult to follow. I see Chris' note about two major security blockers. Were those addressed?

Yes. https://gerrit.wikimedia.org/r/#/c/259522/13..16/PageAssessmentsBody.php

@NiharikaKohli: In the future, it's a good idea to list all the bugs that are affected by a gerrit commit in the commit summary. You can do so by just separating them with commas: "Bug: XXXXX, XXXXX, XXXX"

Legoktm added a subscriber: Legoktm.Feb 3 2016, 1:15 AM

@NiharikaKohli: In the future, it's a good idea to list all the bugs that are affected by a gerrit commit in the commit summary. You can do so by just separating them with commas: "Bug: XXXXX, XXXXX, XXXX"

Err, don't do that. Use individual Bug headers for each bug, so Bug: XXXXX\nBug: XXXXX, etc. :)