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.

Details

Related Gerrit Patches:
mediawiki/extensions/LiquidThreads : masterReduce maximum limit from 5000 to 50
mediawiki/extensions/LiquidThreads : wmf/1.29.0-wmf.9Sanitise page limit
mediawiki/extensions/LiquidThreads : masterSanitise page limit
mediawiki/extensions/LiquidThreads : REL1_23Sanitise page limit
mediawiki/extensions/LiquidThreads : REL1_27Sanitise page limit
mediawiki/extensions/LiquidThreads : REL1_28Sanitise page limit

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 Medium 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 closed this task as Resolved.Jan 31 2017, 2:02 PM
Reedy claimed this task.
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".

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

sbassett moved this task from Patch pending review to Done on the Security board.