321234 - graph.cgi code execution

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect)

Description

[georgi - hopefully not receiving bugspam]

it is possible to (ab)use webtools/graph/graph.cgi for code execution on the
web server with the kind help of gnuplot(1).

#man gnuplot
...
Shell escapes and command line substitution.
...

tested only locally:
Shell escapes and command line substitution.
http://localhost/cgi-bin/graph.cgi?testname=test1&tbox=tbox&units=%60touch%20/tmp/fuub1l2%60

|testname| and |tbox| should be valid values, easily obtained via mouse
clicking.
the command in |units| enclosed in backticks (`) gets executed for me.

Comment 1

[Daniel Veditz [:dveditz]]

moving to "webtools security" group

Comment 2

[Dave Miller [:justdave]]

This is hosted at http://build-graphs.mozilla.org/graph.cgi on axolotl,
which I've now disabled (chmod 000).

Comment 3

[Dave Miller [:justdave]]

After poking around a bit, this appears to be most-closely related to Tinderbox, so that's probably the best place to put this bug.

Comment 4

[Dave Miller [:justdave]]

timeless is apparently working on fixing this.  He says there's much scarier things in the code than just the issue Georgi found.  Per timeless' request, I dropped an .htaccess file into the directory which limits access to collect.cgi (the submission engine that the tinderboxes submit to via http when they complete a build) to the known tinderboxes which have been hitting it over the last month (which amounts to our internal build farms and one outside host [Brad's]).

Comment 5

[georgi - hopefully not receiving bugspam]

(In reply to comment #4)
> timeless is apparently working on fixing this.  He says there's much scarier

much scarier than code execution - you have suid scripts on production?

> Per timeless' request, I
> dropped an .htaccess file into the directory which limits access to collect.cgi

collect.cgi seems to allow creation of files on the web server via open() with non validated input.

Comment 6

[georgi - hopefully not receiving bugspam]

http://build-graphs.mozilla.org/graph/rawdata.cgi is still online.

check bug 321469

Comment 7

[timeless]

Attachment: first draft at requiring sane input

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 206844 [details] [diff] [review]
first draft at requiring sane input

graph.cgi doesn't work if you don't pass it any params.  The tests here don't allow for the params to be blank (which is legal since it gives you a menu to pick from if you leave them out).

Comment 9

[Michael Morgan [:morgamic]]

Attachment: New patch without syntax errors. It also leaves GET pruning out of collect.cgi.

collect.cgi's patch has syntax errors (missing ;), and was also too restrictive for $data and $value.  Certain build systems would fail to update their respective files in graphs/db/.  Since collect.cgi is protected, it should be okay to leave it relatively open.  The alternative would be to check all Tinderbox configurations and make sure the regexp is not excluding any system.

Comment 10

[Chase Phillips]

Comment on attachment 209785 [details] [diff] [review]
New patch without syntax errors.  It also leaves GET pruning out of collect.cgi.

Patch looks good.  Since your heads in this space right now, it would be valuable for you to specify examples or a description of the patterns you're looking for with the regular expressions as comments in the code.

Comment 11

[Michael Morgan [:morgamic]]

Wanted to note that the patch was a mix of timeless's patch w/ the ;'s plus some additional redundant checking of the inputs.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 209785 [details] [diff] [review]
New patch without syntax errors.  It also leaves GET pruning out of collect.cgi.

r=dveditz
I'm not so sure thinking collect.cgi is "proteected" is a good reason to skip sanitizing its arguments, that's relying too much on every tinderbox being secure. But let's get this much in and running.

dbaron is at a conference, pick someone else if you need 3 reviews.

Comment 13

[Chase Phillips]

Comment on attachment 209785 [details] [diff] [review]
New patch without syntax errors.  It also leaves GET pruning out of collect.cgi.

A third review may be sought sooner than when dbaron gets back, but it would still be useful to have his review on this patch.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 209785 [details] [diff] [review]
New patch without syntax errors.  It also leaves GET pruning out of collect.cgi.

I've never seen this code, but r=dbaron.  A few comments, though:

According to "man perlre", \w is 'alphanumeric plus "_"', so there shouldn't be any need to use \d or _ along with it.

Does "." need to be escaped inside character classes in perl?

+$TBOX = '' unless $TBOX =~ m/^(?:[-_\.\w\d]+)$/;
why not just use m/^[-_\.\w\d]+$/ ?
(occurs twice, I think)

I think the regexp for SHOWPOINT should use \d+ rather than \d* and should have no ? after the ,.

Comment 15

[timeless]

. doesn't need to be escape in perl character classes.

the (?:) stuff was from me building alternates and repeats, and is indeed unnecessary in this case.

http://build-graphs.mozilla.org/graph/graph.cgi?tbox=comet.mozilla.org&testname=xulwinopen&autoscale=1&size=&days=7&units=&ltype=&points=&showpoint=2006:06:03:18:25:46&avg=1 happens to work today, but if you want to tighten the restriction, we can do it as you described.

Comment 16

[Reed Loden [:reed]]

Attachment: what I will check-in

It seems nobody ever checked this in, so I'm doing that. I addressed several of dbaron's nits, but since this service is being replaced by vlad's new graph server, I didn't spend that much time on it.

Comment 17

[Reed Loden [:reed]]

Checking in collect.cgi;
/cvsroot/mozilla/webtools/graph/collect.cgi,v  <--  collect.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in compare-graphs.cgi;
/cvsroot/mozilla/webtools/graph/compare-graphs.cgi,v  <--  compare-graphs.cgi
new revision: 1.2; previous revision: 1.1
done
Checking in graph.cgi;
/cvsroot/mozilla/webtools/graph/graph.cgi,v  <--  graph.cgi
new revision: 1.20; previous revision: 1.19
done
Checking in multiquery.cgi;
/cvsroot/mozilla/webtools/graph/multiquery.cgi,v  <--  multiquery.cgi
new revision: 1.15; previous revision: 1.14
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/graph/query.cgi,v  <--  query.cgi
new revision: 1.41; previous revision: 1.40
done
Checking in rawdata.cgi;
/cvsroot/mozilla/webtools/graph/rawdata.cgi,v  <--  rawdata.cgi
new revision: 1.6; previous revision: 1.5
done