979884 - xss on tbpl in rev param

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Date: Tue, 4 Mar 2014 06:57:33 +0530
Message-ID: <CA+LKR=v+Nz6R_=[e-mail]>
Subject: XSS vulnerability on tbpl.mozilla.org
From: Deepankar Arora <[e-mail]>
To: [e-mail]
-----//-----
Hi,

tbpl.mozilla.org suffers from a cross-site scripting vulnerability due to failure to sanitize the user input.

Proof of concept:
https://tbpl.mozilla.org/?tree=Try&rev="><img%20src%3Dx%20onerror%3Dalert%282%29%3B>

As it shows, the value of rev parameter is not filtered and the script is executed.

Thanks,
Deepankar Arora

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

confirmed

Comment 2

[codeinjector007]

Any update on this one?

Comment 3

[Raymond Forbes[:rforbes]]

tbpl is not on the list of eligible systems.  we do not use *.mozilla.org cookies, that helps re mediate any issues with cookie stealing from this domain.

Comment 4

[cmtalbert]

Ed Morley or David Burns will look into this and get us a fix.

Comment 5

[Ed Morley [:emorley]]

Looking now. (Didn't get the bugmail for this bug being filed, even though I'm watching this component, since I don't have s-s-webtools permissions)

Comment 6

[Ed Morley [:emorley]]

To add to comment 3, the worst that I can come up with for this so far (other than people using a JS 0-day, but I doubt anyone would click on a link that long) is stealing the sheriff password using for controlling the visibility of jobs, presuming someone has it autofill by their password manager (eg https://tbpl.mozilla.org/?tree=Try&rev=%22%3E%3Cimg%20src%3Dx%20onerror%3Dalert%28%24%28%22%23password%22%29.val%28%29%29%3B%3E ; and any changes using said password are reversible & the password is a shared one that 100+ people have access to already). Other than caching a nickname for commenting, the "match server timezone" pref & the tree MRU list, TBPL doesn't store any other data in localstorage or cookies.

Comment 7

[Ed Morley [:emorley]]

(TBPL doesn't have a persistent login for which session state can be stolen)

Comment 8

[Ed Morley [:emorley]]

Attachment: Cleanup1: Don't check for legacy parameters if the query string was empty

(Breaking these up a bit to make the diffs clearer)

Comment 9

[Ed Morley [:emorley]]

Attachment: Cleanup2: Remove the leading '?' earlier

Currently we do unnecessary work if document.location.search == "?". By stripping the leading '?' early we avoid this and more importantly IMO make this block easier to read.

Comment 10

[Ed Morley [:emorley]]

Attachment: Cleanup3: Assign the parameter & value since we'll need them later

Comment 11

[Ed Morley [:emorley]]

Attachment: Main fix: Discard query string parameters that contain invalid characters

All over the UI we treat the revision as trusted & don't escape it, even though the params weren't validated. We got away with this until now, since the only place those params ended up were as part of the hgwebjson fetch, which would fail if passed invalid params, which would mean we don't proceed with the rest of the app load & didn't have a pushlog response to handle. However with bug 721152 a failed pushlog load resulted in us falling back to the provided revision & proceeding with app load.

I think the best solution for now is just to sanitise all params to avoid unintended side-effects by seemingly innocuous changes (I'd thought the change in bug 721152 was safe, since we didn't perform any validation in the $.ajax() success case either).

The only param for which we may later need to tweak the regexp is 'pusher' - though I processed the last 5000 Try pushes to check that all email addresses used passed the current regexp, which they did.

Comment 12

[Ed Morley [:emorley]]

For clarity, [\w\-.+@] expands to:
A-Za-z0-9_-.+@

Comment 13

[Ed Morley [:emorley]]

Joel, in this bug I'm adding some fairly strict validation of query string parameters. For the &fromchange=/&tochange= queries that you perform, what syntax do you use? The regexp I've chosen (comment 12) will handle dates that use a hyphen delimiter, but not forward slash. In addition, colon isn't in the list, so times won't work. Is this an issue?

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

thanks Ed for checking with me.  My common use cases of tbpl are these type of examples:
* https://tbpl.mozilla.org/?tree=Fx-Team&fromchange=1af4ac38f025&tochange=67e72ea05f66&jobname=Ubuntu%20HW%2012.04%20fx-team%20talos%20svgr
* https://tbpl.mozilla.org/?tree=Fx-Team&rev=67e72ea05f66
* https://tbpl.mozilla.org/?tree=Fx-Team&startdate=2014-04-04&enddate=2014-04-06&jobname=fx-team%20talos%20svgr

fromchange/tochange is used in the first item and it is only revisions.  I have never used a / unless it was part of a jobname or branchname (i don't think it is)

Comment 15

[Ed Morley [:emorley]]

Removing bug 835471 from blocks, since tinderbox != tinderboxpushlog. Tinderboxpushlog (aka TBPL) was formerly a consumer of tinderbox (hence the name), but now consumes buildbot data (amongst others) instead.

(In reply to Joel Maher (:jmaher) from comment #14)
> I have never used a / unless it was part of a jobname or branchname (i don't
> think it is)

Yeah the params supported (either the tree name or the buildbot equivalent) don't have forward slashes:
https://hg.mozilla.org/webtools/tbpl/file/tip/js/Config.js#l38
(The keys under treeInfo and the value of buildbotBranch for each of them; primaryRepo is only used for pushlog paths etc)

Comment 16

[Ed Morley [:emorley]]

Thank you for the reviews :-)

remote:   https://hg.mozilla.org/webtools/tbpl/rev/5942837aeff2
remote:   https://hg.mozilla.org/webtools/tbpl/rev/4c5c08bd255b
remote:   https://hg.mozilla.org/webtools/tbpl/rev/c06adeebefe8
remote:   https://hg.mozilla.org/webtools/tbpl/rev/42d5a3629505

Comment 17

[Ed Morley [:emorley]]

In production :-)