Page MenuHomePhabricator

Slow query in ApiQueryBacklinksprop when using an empty xxnamespace parameter
Closed, ResolvedPublic

Description

Abstract

While developing a bot I've just figure out that a query on the transcludedin API module with a non-numeric tinamespace / gtinamespace throws an error 500 after 60 seconds of execution on major Wikimedia Foundation sites.

Also note that other modules defined in the ApiQueryBacklinksprop may be affected.

Proof of concept

Normal usage examples

Notes

As far I can see this is not a problem related to all API modules with a namespace filter e.g. allpages quits immediately as expected:

Here some MediaWiki versions that I think are not affected:

  • <=1.24 (has not transcludedin generator)
  • 1.27.4
  • 1.29.1

Now I really have no time to further investigate because tomorrow morning I'm in an office.

Definition

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 1 2018, 12:20 AM
valerio.bozzolan renamed this task from Potential DOS on transcludedin MediaWIki generator API through wrong namespace? to Potential denial of service through transcludedin MediaWIki generator API using a wrong namespace.Oct 1 2018, 12:24 AM
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan renamed this task from Potential denial of service through transcludedin MediaWIki generator API using a wrong namespace to Denial of service through transcludedin MediaWIki generator API using a wrong namespace.Oct 1 2018, 12:28 AM
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan renamed this task from Denial of service through transcludedin MediaWIki generator API using a wrong namespace to Denial of service through the "transcludedin" MediaWiki generator API module using a wrong namespace.Oct 1 2018, 12:47 AM
valerio.bozzolan renamed this task from Denial of service through the "transcludedin" MediaWiki generator API module using a wrong namespace to Denial of service through the "transcludedin" MediaWiki API module using a wrong namespace.Oct 1 2018, 12:59 AM
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan renamed this task from Denial of service through the "transcludedin" MediaWiki API module using a wrong namespace to Denial of service through some MediaWiki API modules (at least some defined in ApiQueryBacklinksprop) using a wrong namespace.Oct 1 2018, 1:04 AM
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan updated the task description. (Show Details)
Anomie edited subscribers, added: Anomie; removed: MediaWiki-API.

That is a lot of changes to the description. The general guidance on https://en.wikipedia.org/wiki/Template:Uw-preview may be relevant, although in Phabricator previewing happens automatically if you have JavaScript enabled.

Anomie claimed this task.Oct 1 2018, 2:03 PM

The "wrong namespace" bit is misleading. Specifying only invalid values is equivalent to specifying an empty array.

Without tinamespace, the query generated looks like

SELECT  /*! STRAIGHT_JOIN */ tl_from,tl_namespace AS `bl_namespace`,tl_title AS `bl_title`,page_id,page_title,page_namespace,page_is_redirect  FROM `templatelinks` FORCE INDEX (tl_namespace),`page`    WHERE (tl_from = page_id) AND ((tl_namespace = '0' AND tl_title = 'A'))  ORDER BY tl_from LIMIT 11

With an empty tinamespace, the query looks like

SELECT  /*! STRAIGHT_JOIN */ tl_from,tl_namespace AS `bl_namespace`,tl_title AS `bl_title`,page_id,page_title,page_namespace,page_is_redirect  FROM `templatelinks` FORCE INDEX (tl_backlinks_namespace),`page`    WHERE (tl_from = page_id) AND ((tl_namespace = '0' AND tl_title = 'A'))  ORDER BY tl_from LIMIT 11

In other words, the index forcing is wrong because the code isn't taking the possibility of an empty list of namespaces into account.

Anomie renamed this task from Denial of service through some MediaWiki API modules (at least some defined in ApiQueryBacklinksprop) using a wrong namespace to Slow query in ApiQueryBacklinksprop when using an empty xxnamespace parameter.Oct 1 2018, 2:45 PM
Anomie removed a project: Security.
Anomie changed the visibility from "Custom Policy" to "Public (No Login Required)".
Restricted Application added a project: Security. · View Herald TranscriptOct 1 2018, 2:45 PM

This doesn't seem to be much of a DoS, just a bad query.

Change 463772 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] API: Handle empty xxnamespace parameter in ApiQueryBacklinksprop

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

That is a lot of changes to the description. The general guidance on https://en.wikipedia.org/wiki/Template:Uw-preview may be relevant, although in Phabricator previewing happens automatically if you have JavaScript enabled.

Sorry for the multiple edits. I've immediately published every single additional note... too much ASAP.

This doesn't seem to be much of a DoS, just a bad query.

Well, a page that runs a 60 seconds query is a very appreciated DOS vector.

Bawolff added a subscriber: Bawolff.Oct 2 2018, 9:37 PM

Well, a page that runs a 60 seconds query is a very appreciated DOS vector.

The extent that that is an "appreciable" DOS vector is going to depend on your setup. In the Wikimedia context, it isn't really (due to mitigations at the DB layer). Of course, its still a bad thing that should be fixed, but its not bad enough that it meets our usual definition of significant DOS.

Legoktm added a subscriber: Legoktm.

This seems like it would be easy to backport, so I'm boldly adding those release tags.

Change 463772 merged by jenkins-bot:
[mediawiki/core@master] API: Handle empty xxnamespace parameter in ApiQueryBacklinksprop

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

Anomie moved this task from Needs Review to Done on the MediaWiki-API board.Oct 3 2018, 3:24 PM
Krinkle moved this task from Backlog to Core on the MW-1.31-release board.Oct 20 2018, 5:48 AM
kchapman closed this task as Resolved.Dec 18 2018, 3:25 PM
kchapman added a subscriber: kchapman.

Marking as resolved. If anyone cares about backporting they can go ahead and do that.