Page MenuHomePhabricator

Security review of Wikibase-Quality-Constraints - v1 branch
Closed, ResolvedPublic

Description

Please do a security review of Wikibase-Quality-Constraints
Thank you very much!

Gerrit:

Project

Repo: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/WikidataQualityConstraints
Evaluated: f655fc3ea04d4c8b37f5a61a6b74ce258b024bb0
Dependencies:

  • wikibase/wikibase, wikibase/data-model, wikibase/quality
  • composer/installers

Review

Did we work with the team during the design phase? No

Data-flow diagram:

Architecture issues

  • (T101303) The update process is a manual ETL, and has to be run by a deployer for each data update. It's not clear why the database and tools need to live on the cluster.
  • The design behind FormatChecker makes the safety of the processing rely on the CSV not being hostile. Unless there's a way to indicate what needs to be in the regex and you build a known good regex, I'm not sure that check can be run in production. Also noted below as T101467.

Hardening

  • (T101305) Escape SQL input in the functions that interact with the DB
  • SpecialConstraintReport should use an HTMLForm, which will add csrf protection
  • (T101306) Use escaped() instead of text() when inserting messages into raw html
  • (T101308) Escape $entityId->getSerialization()
  • Document which function parameters are expected to be sanitized HTML, and when functions return HTML. If the safety of the returned HTML relies on an input parameter being properly sanitized, document that assumption.
  • (T101468 / T101469) CommonsLinkChecker: proxying user-controlled values is dangerous-- need to url encode any user controlled values, and the url needs to be https://. Need to check with ops that the url is accessible, I think you need to use the outbound proxy to access commons like that. Also, get_headers doesn't validate tls certificate, you need to use something that does.
  • UniqueValueChecker - if this isn't working, the class should be removed.

Vulnerabilities

  • T101341
  • (T101467) FormatChecker executes a regex from the Contstraint table-- stored regex DoS.

Non-security

  • The result messages are all hardcoded English-- those should be translatable.
  • The performance characteristics of this do not look good; you may want to work with @aaron to find better ways to do some of these things

Related Objects

Event Timeline

Tamslo raised the priority of this task from to Needs Triage.
Tamslo updated the task description. (Show Details)

@Tamslo, For each of these reviews (T99352, T99358, and T99355), can you link to the code repository for each? And can you provide a data-flow diagram (https://www.mediawiki.org/wiki/Security_for_developers/Architecture#Threat_modeling) for each as well? If you don't have them already, we can probably schedule an hour to draw them together at some point.

Tamslo updated the task description. (Show Details)

@csteipp Done. We don't have such a diagram so far, scheduling an hour to draw them would be nice :)

I'm done with the initial review. All the blockers need to get resolved before this is closed.

csteipp moved this task from Incoming to In Progress on the Security-Team board.
csteipp moved this task from In Progress to Waiting on the Security-Team board.

Short clarification: Please review branch v1 on gerrit.

csteipp renamed this task from Security review of Wikibase-Quality-Constraints to Security review of Wikibase-Quality-Constraints - v1 branch.Jun 22 2015, 8:24 PM
csteipp closed this task as Resolved.
csteipp claimed this task.
csteipp removed a subtask: Restricted Task.

OK, blockers have all been resolved for v1. We will need another review before violations are deployed.