1182994 - Treeherder SQL injection in API 'offset' parameter due to Datasource's LIMIT/OFFSET handling

Status: RESOLVED FIXED

(Tree Management :: Treeherder: API, defect, P1)

Description

[David Chan [:dchan]]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36

Steps to reproduce:

The Treeherder API constructs raw SQL queries for certain endpoints. The data is not properly escaped resulting in SQL injection. 

STR:
1. Visit 
https://treeherder.mozilla.org/api/project/mozilla-central/bug-job-map/?offset=%27abcd


Actual results:

SQL Error indicating successful injection
{"detail": "(1064, \"You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''abcd,10' at line 1\")"}


Expected results:

No error

This specific bug exists due to the code not properly casting the GET param to an int. [1] There may be more vulnerable API endpoints which call jobs_execute() in [2]

[1] - https://github.com/mozilla/treeherder/blob/12f80ce4486497e33131faeaae19429f1e00ff4e/treeherder/webapp/api/bug.py#L74
[2] -  https://github.com/mozilla/treeherder/blob/12f80ce4486497e33131faeaae19429f1e00ff4e/treeherder/model/derived/jobs.py#L209

Comment 1

[David Chan [:dchan]]

I tried to use the web-bounty form at https://bugzilla.mozilla.org/form.web.bounty, but received a permissions error for setting sec-bounty

Comment 2

[Ed Morley [:emorley]]

Thank you for reporting this! :-)

The problem (or at least this one) is here in the datasource package we're using:
https://github.com/jeads/datasource/blob/aab1f00d9533cb043c7e6074a0f01e02814fba2a/datasource/bases/RDBSHub.py#L332

Yet another reason we need to ditch datasource in lieu of using Django's ORM.

Comment 3

[David Chan [:dchan]]

Another vulnerable URL

- https://treeherder.mozilla.org/api/project/mozilla-central/artifact/?offset=%27abcd

My local testing on Django shows that the MySQL adapter allows multiple queries / commands in execute(). Mozilla's setup may be different and not suffer from this which would limit the impact. Otherwise, the vulnerable SQL query looks like

SELECT ... FROM ... LIMIT [sqi], 10;

and by setting [sqli] to
1; CREATE / SELECT / UPDATE ... ;#

you will be able to execute arbitrary queries as the db user, possibly obtaining a webshell through 'SELECT ... INTO OUTFILE /somewhere/in/webroot' . I used the the following to test on my local install

1; CREATE TABLE foo (bar INT);#

then checked for the existence of foo.

Comment 4

[David Chan [:dchan]]

(In reply to Ed Morley [:emorley] from comment #2)
> Thank you for reporting this! :-)
> 
> The problem (or at least this one) is here in the datasource package we're
> using:
> https://github.com/jeads/datasource/blob/
> aab1f00d9533cb043c7e6074a0f01e02814fba2a/datasource/bases/RDBSHub.py#L332
> 
> Yet another reason we need to ditch datasource in lieu of using Django's ORM.

Ah, thanks for the additional information. I was under the assumption that you were using the Django functions for raw sql. You can ignore most of comment 3 :)

Comment 5

[Ed Morley [:emorley]]

Well, we should of course be casting the API param too - but I think datasource should handle this - otherwise who would ever want to use it over stored procedures :-/

What worries me is how many other nuggets like this are lurking in datasource/our use of it :-S

Comment 6

[Ed Morley [:emorley]]

Attachment: Datasource: Fix handling of limit and offset parameters

* Correctly use the 'offset' param instead of 'limit', since the wrong parameter was being substituted.
* Cast the limit and offset parameters to int, to prevent SQL injection if those parameters were not sanitised in the app. Note: This intentionally removes the ability to pass a limit string of say "100,200" to Datasource - since IMO the offset parameter makes this redundant (and now the offset param is fixed we can actually use it).

We'll need to make Treeherder switch to the latest datasource at the same time as changing the limit params to instead be a combination of limit+offset.

Comment 7

[Ed Morley [:emorley]]

Attachment: Treeherder: Cast API offset parameter to int

(This got r+ over IRC from mdoglio)

Comment 8

[Ed Morley [:emorley]]

Comment on attachment 8632727 [details] [review]
Treeherder: Cast API offset parameter to int

Treeherder workaround (/something we should probably have been doing anyway) checked in:
https://github.com/mozilla/treeherder/commit/b05b6f38ceced08a9582bd1263d1f0cb2f8b3f67

And deployed to stage+prod.

[~/src/treeherder]$ curl https://treeherder.mozilla.org/api/project/mozilla-central/bug-job-map/?offset=%27abcd
{"detail": "invalid literal for int() with base 10: \"'abcd\""}
[~/src/treeherder]$ curl https://treeherder.mozilla.org/api/project/mozilla-central/artifact/?offset=%27abcd
{"detail": "invalid literal for int() with base 10: \"'abcd\""}

Comment 9

[Ed Morley [:emorley]]

If it helps for determination of exploitability, of the four API endpoints patched in comment 8, one does not use it's offset_id param, and the other three are all queries that run under the read_host DB config:
https://github.com/mozilla/treeherder/blob/0f3d1b2bcf8700070e7e99892bf1990528811bd0/treeherder/model/sql/jobs.json#L903
https://github.com/mozilla/treeherder/blob/0f3d1b2bcf8700070e7e99892bf1990528811bd0/treeherder/model/sql/jobs.json#L389
https://github.com/mozilla/treeherder/blob/0f3d1b2bcf8700070e7e99892bf1990528811bd0/treeherder/model/sql/jobs.json#L664

...which uses the DB user 'th_user' on treeherder-stage-ro-vip.db.scl3.mozilla.com / treeherder-ro-vip.metrics.scl3.mozilla.com:
https://github.com/mozilla/treeherder/blob/2d087edaeab07b1fe680d211413a37090792f309/treeherder/model/models.py#L262

That user 'only' has the following permissions:
Select_priv, Insert_priv, Update_priv, Delete_priv

Though I guess we could reduce that down further, to just select? (the non-read host queries use the user th_admin)

Comment 10

[Ed Morley [:emorley]]

Attachment: Update to datasource v0.9's and use its fixed 'offset' parameter

I've created a new Datasource release with the Datasource fixes for limit/offset.

This PR makes Treeherder use that new release, as well as switching uses of the 'limit' parameter to use limit+offset, now that Datasource no longer supports passing a comma delimited string to 'limit.

Comment 11

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #10)
> I've created a new Datasource release with the Datasource fixes for
> limit/offset.

https://github.com/jeads/datasource/releases/tag/v0.9
https://github.com/jeads/datasource/compare/v0.8...v0.9

Comment 12

[Ed Morley [:emorley]]

As far as I'm aware the only other users of the datasource package are Bugherder & Datazilla - I've inspected both, and neither pass `limit` (or `offset`) to datasource (they use LIMITs in the json sql statements instead, which is filtered by mysql-python) so are not affected by the same issue.

Comment 13

[Ed Morley [:emorley]]

Comment on attachment 8632752 [details] [review]
Update to datasource v0.9's and use its fixed 'offset' parameter

https://github.com/mozilla/treeherder/commit/044adae263586164877d644c416fba174b91ca11
https://github.com/mozilla/treeherder/commit/c16772d5d6e2c558baf5b440ffb78658dab48c00