Page MenuHomePhabricator

LiquidThreads denial of service due to unvalidated limit parameter
Closed, ResolvedPublic

Description

There was an incident at translatewiki.net. I was not around, but some people suspected HHVM has crashed, which it has done before.

I started investigating the logs and I found things such as this:

2017-01-13 13:20:44 translatewiki.net translatewiki_net-bw_: [f8ad881d0def7b56970ebdcc] /w/i.php?title=Support&offset=20160623103144&limit=20999999.1%20union%20select%20unhex(hex(version()))%20--%20and%201%3D1   MWException from line 168 of /srv/mediawiki/tags/2017-01-04_09:30:19/extensions/SyntaxHighlight_GeSHi/SyntaxHighlight_GeSHi.class.php: Unexpected output from Pygments encountered
2017-01-13 13:20:02 translatewiki.net translatewiki_net-bw_: [5181b65680301d1e34af5d73] /w/i.php?title=Support&offset=20160623103144&limit=201111111111111%22%20UNION%20SELECT%20CHAR(45,120,49,45,81,4
5),CHAR(45,120,50,45,81,45),CHAR(45,120,51,45,81,45),CHAR(45,120,52,45,81,45),CHAR(45,120,53,45,81,45),CHAR(45,120,54,45,81,45),CHAR(45,120,55,45,81,45),CHAR(45,120,56,45,81,45),CHAR(45,120,57,45,81,
45),CHAR(45,120,49,48,45,81,45),CHAR(45,120,49,49,45,81,45),CHAR(45,120,49,50,45,81,45),CHAR(45,120,49,51,45,81,45),CHAR(45,120,49,52,45,81,45),CHAR(45,120,49,53,45,81,45),CHAR(45,120,49,54,45,81,45)
,CHAR(45,120,49,55,45,81,45),CHAR(45,120,49,56,45,81,45),CHAR(45,120,49,57,45,81,45),CHAR(45,120,50,48,45,81,45),CHAR(45,120,50,49,45,81,45),CHAR(45,120,50,50,45,81,45),CHAR(45,120,50,51,45,81,45),CH
AR(45,120,50,52,45,81,45),CHAR(45,120,50,53,45,81,45),CHAR(45,120,50,54,45,81,45),CHAR(45,120,50,55,45,81,45),CHAR(45,120,50,56,45,81,45),CHAR(45,120,50,57,45,81,45),CHAR(45,120,51,48,45,81,45),CHAR(
45,120,51,49,45,81,45),CHAR(45,120,51,50,45,81,45),CHAR(45,120,51,51,45,81,45),CHAR(45,120,51,52,45,81,45),CHAR(45,120,51,53,45,81,45),CHAR(45,120,51,54,45,81,45),CHAR(45,120,51,55,45,81,45),CHAR(45,
120,51,56,45,81,45),CHAR(45,120,51,57,45,81,45)%20--%20/*%20order%20by%20%22as%20/*   MWException from line 168 of /srv/mediawiki/tags/2017-01-04_09:30:19/extensions/SyntaxHighlight_GeSHi/SyntaxHighl
ight_GeSHi.class.php: Unexpected output from Pygments encountered
\nFatal error: request has exceeded memory limit in /srv/mediawiki/tags/2017-01-04_09:30:19/includes/OutputPage.php on line 1506
2017-01-13 15:21:31 translatewiki.net translatewiki_net-bw_: [1af36d12a5fc0dea8bec34c9] /w/i.php?title=Support&offset=20160623103144&limit=20999999.1%20and(select%201%20from(select%20count(*),concat(
(select%20(select%20(SELECT%20distinct%20concat(0x7e,0x27,%27ololo%27,0x27,0x7e)%20FROM%20information_schema.schemata%20LIMIT%201))%20from%20information_schema.tables%20limit%200,1),floor(rand(0)*2))
x%20from%20information_schema.tables%20group%20by%20x)a)%20and%201=1%20   MWException from line 168 of /srv/mediawiki/tags/2017-01-04_09:30:19/extensions/SyntaxHighlight_GeSHi/SyntaxHighlight_GeSHi.c
lass.php: Unexpected output from Pygments encountered

I knew the warning about Pygments already, and I believe it is unrelated.

I tried some of the urls myself, to see if there is an SQL injection, but those were timing out. Then I made MediaWiki to log into error log all the queries it does. I got about 90M of those, over 100k queries etc. With grepping I found no evidence of an SQL injection, but these requests take many minutes to complete even though nginx gives a time out. Most likely the attacker exhausted our resources bringing the site down.

Lqt does not do any validation in the limit parameter:

class LqtDiscussionPager extends IndexPager {
	function __construct( $article, $orderType ) {
		$this->article = $article;
		$this->orderType = $orderType;

		parent::__construct();

		$this->mLimit = $this->getPageLimit();
	}

	function getPageLimit() {
		$article = $this->article;

		global $wgRequest;
		$requestedLimit = $wgRequest->getVal( 'limit', null );
		if ( $requestedLimit ) {
			return $requestedLimit;
		}

		if ( $article->exists() ) {
			$pout = $article->getParserOutput();
			$setLimit = $pout->getProperty( 'lqt-page-limit' );
			if ( $setLimit ) return $setLimit;
		}

		global $wgLiquidThreadsDefaultPageLimit;
		return $wgLiquidThreadsDefaultPageLimit;
	}

My conclusion is that this is only a denial of service issue, but I would like someone else to double check.

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 13 2017, 7:20 PM

I didn't review the code in detail either, but it looks like the limit is cast to a number later. Instead of $this->mLimit = $this->getPageLimit();, it should be doing $this->setLimit( $this->getPageLimit() ). which explicitly does a cast and limits the range of values.

Reedy added a subscriber: Reedy.Jan 24 2017, 6:49 PM
diff --git a/pages/TalkpageView.php b/pages/TalkpageView.php
index 368681d..11bab19 100644
--- a/pages/TalkpageView.php
+++ b/pages/TalkpageView.php
@@ -439,7 +439,7 @@ class LqtDiscussionPager extends IndexPager {
 
                parent::__construct();
 
-               $this->mLimit = $this->getPageLimit();
+               $this->setLimit( $this->getPageLimit() );
        }
 
        function getPageLimit() {

Guess we should probably deploy to WMF wikis before pushing it out straight into gerrit

dpatrick triaged this task as Normal priority.Jan 24 2017, 10:02 PM
Reedy added a comment.Jan 30 2017, 9:38 PM

@Nikerabbit I'm going to deploy this to WMF wikis tonight, and then put it on gerrit tomorrow if you want to get it deployed to twn around a similar time as it goes onto gerrit

From 6531168a01f4336a9e4dd5b8ed0f6480829dae69 Mon Sep 17 00:00:00 2001
From: Reedy <reedy@wikimedia.org>
Date: Mon, 30 Jan 2017 21:37:01 +0000
Subject: [PATCH] Sanitise page limit

Bug: T155265
Change-Id: I9ed476fe208ea39e9dbdcfa671a8614ac8e7116a
---
 pages/TalkpageView.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pages/TalkpageView.php b/pages/TalkpageView.php
index 368681d..11bab19 100644
--- a/pages/TalkpageView.php
+++ b/pages/TalkpageView.php
@@ -439,7 +439,7 @@ class LqtDiscussionPager extends IndexPager {
 
                parent::__construct();
 
-               $this->mLimit = $this->getPageLimit();
+               $this->setLimit( $this->getPageLimit() );
        }
 
        function getPageLimit() {
-- 
2.9.3

Deployed to the cluster now

Deployed to the cluster now

Thanks @Reedy.

@Nikerabbit I'm going to deploy this to WMF wikis tonight, and then put it on gerrit tomorrow if you want to get it deployed to twn around a similar time as it goes onto gerrit

I have been running my own patch on translatewiki.net since the day I reported this bug.

This should be marked as resolved and made public?

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Jan 31 2017, 2:02 PM
Reedy closed this task as Resolved.
Reedy claimed this task.

Change 335321 had a related patch set uploaded (by Reedy):
Sanitise page limit

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

Change 335322 had a related patch set uploaded (by Reedy):
Sanitise page limit

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

Change 335323 had a related patch set uploaded (by Reedy):
Sanitise page limit

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

Change 335322 merged by jenkins-bot:
Sanitise page limit

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

Change 335321 merged by jenkins-bot:
Sanitise page limit

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

Change 335323 merged by Reedy:
Sanitise page limit

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

Change 335447 had a related patch set uploaded (by Reedy):
Sanitise page limit

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

Change 335447 merged by jenkins-bot:
Sanitise page limit

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

Kghbln added a subscriber: Kghbln.Feb 24 2017, 5:06 PM

I think news about such fixes should go to "mediawiki-l" too like it has similarly been do for Math, etc. recently. I just stumbled upon this by pure chance when browsing the twn news page.

Change 356330 had a related patch set uploaded (by Nikerabbit; owner: Nikerabbit):
[mediawiki/extensions/LiquidThreads@master] Reducde maximum limit from 5000 to 50

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

Change 356330 merged by jenkins-bot:
[mediawiki/extensions/LiquidThreads@master] Reduce maximum limit from 5000 to 50

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