This review supersedes bug 658098 as there have been many changes in the meantime. Bug 658098 was only about the new file php/starcomment.php; this one is about all the files in the php directory. The last TBPL security review was in bug 571551 comment 18 / bug 573823. > A quick intro to what this app does. TBPL is a developer dashboard that correlates Mercurial repository pushes with results from automated tests. The focus of the changes that need to be reviewed is the switch away from Tinderbox. tinderbox.mozilla.org provided several services that need to be provided by tbpl itself now: - Run data used to come from http://tinderbox.mozilla.org/Firefox/json.js but now is imported from http://build.mozilla.org/builds/builds-4hr.js.gz - Parsed logs were provided by http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1306925550.1306926464.16768.gz and now by http://tbpl.swatinem.de/php/getParsedLog.php?id=4913049 - Similar for run notes ("stars") and the list of hidden builders per tree. By default we still use Tinderbox for the data; in order to use the new Buildbot based backend, you'll have to specify usebuildbot=1 in the URL: http://tbpl.swatinem.de/?usebuildbot=1 > Where is the source code located? http://hg.mozilla.org/users/mstange_themasta.com/tbpl-pending-infrasec-review/ The source code that's currently running on http://tbpl.mozilla.org/ and that's missing all the to-be-reviewed php changes is here: http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/ The changes in tbpl-pending-infrasec-review are listed here: http://hg.mozilla.org/users/mstange_themasta.com/tbpl-pending-infrasec-review/pushloghtml > Is there a stage server running that we can also test against? If so, > please indicate what machine the web server is running on. The code is running on a staging server here: http://tbpl.swatinem.de/ Note that this server doesn't use php/starcomment.php (which the security review in bug 658098 is about) because it doesn't have access to the elastic search server at elasticsearch1.metrics.sjc1.mozilla.com and because we're not running a local ES instance on it. However, starcomment.php can be tested on the staging server http://brasstacks.mozilla.com/tbpl/ as noted in bug 658098. > Where would you like the bugs filed in bugzilla? Please specify > the product, component and if anyone specific should be copied > on the bugs. Please use Webtools -> Tinderboxpushlog, no cc necessary. > Please describe if this app will be connecting to any internal or external > services or if it is able to interact with the OS. Yes to all. Internal services are a MongoDB server and the file system, external services are the servers build.mozilla.org and ftp.mozilla.org. These files access the MongoDB: - dataimport/import-buildbot-data.py (read/write) - php/getBuilderHistory.php (read) - php/getBuilders.php (read) - php/getRevisionBuilds.php (read) - php/submitBuildStar.php (read/write) - php/updateBuilders.php (read/write) - php/inc/HiddenBuilders.php (read) - php/inc/RunForLog.php (read) The contents of the MongoDB are mostly non-secret, except for the IPs stored by submitBuildStar.php in db.tbpl.runs.notes.ip and updateBuilders.php in db.tbpl.builders.history.ip. These files access the file system: - php/inc/ParallelFileGenerating.php - php/inc/GzipUtils.php - php/inc/RawGzLogDownloader.php - php/getTinderboxSummary.php - leak-analysis/index.php In the scripts in php/inc/, file names are generated in RawGzLogDownloader::getLines, AnnotatedSummaryGenerator::ensureAnnotatedSummaryExists, LogParser::ensureExcerptExists and ParsedLogGenerator::ensureLogExists and only make use hardcoded strings and numbers (run IDs) from the database. I haven't recently looked at php/getTinderboxSummary.php and leak-analysis/index.php, but I think most of those files has already been security reviewed the last time. build.mozilla.org is accessed by dataimport/import-buildbot-data.py. ftp.mozilla.org is accessed in php/inc/RawGzLogDownloader.php. > Does this app support logins or multiple roles? If so, we'll need > test accounts created for each available role. It supports a very rudimentary password check for the administration page, but that's it. > What is the worst case scenario that could happen with this system, > data or connected systems? (This is used to help understand the > criticality of this server.) If the server went down, Firefox developers wouldn't be able to see the results of their checkins and the tree would have to be closed until the server was back up or until another tbpl instance would be set up on a different server. If somebody gained read access to the database, the IP addresses saved by submitBuildStar.php and updateBuilders.php would be exposed. If somebody gained write access to the database, he'd probably be able to take over the whole server since the PHP scripts don't validate the database contents (they're trusted). If the whole data on the server were lost, it would be a minor nuisance but not critical. Run data could be reimported from build.mozilla.org, but build notes (which are very transient) and hidden builder configuration would have to be re-added manually. If the sheriff password (see below) were cracked, the attacker could toggle hidden states of builders, which is easy to undo. > Does this website contain an administration page? There's a UI for changing "hidden builders" which asks for the "sheriff password" on submission: Go to http://tbpl.swatinem.de/?usebuildbot=1, click on "Tree Info" and then on "Open tree admin panel", double click a builder in the list and click "Save changes...". The sheriff password on tbpl.swatinem.de is "yeah" and stored as clear text in php/sheriff-password.php. There's no session id or any form of cookies used. This tree admin panel is designed to supersede the current Tinderbox admin panel on http://tinderbox.mozilla.org/admintree.cgi?tree=Firefox . > If so, have the admin page blockers (listed here) all been addressed? No, none of them have. The sheriff password doesn't have much power, and I don't think it's worth increasing the security compared to the current Tinderbox admin panel. The sheriff password for http://tinderbox.mozilla.org/admintree.cgi?tree=Firefox hasn't been changed since 2008-06-19, it's always sent as plain text and doesn't have any brute force protection as far as I know. > This review will be scheduled amongst other requested reviews. > What is the urgency or needed completion date of this review? Not urgent, but the switch away from Tinderbox has been a long time coming and it would be really nice to get it finished soon. It would also address a Tinderbox bug that annoys Firefox developers every once in a while (bug 590526).

(In reply to comment #0) > > Where would you like the bugs filed in bugzilla? Please specify > > the product, component and if anyone specific should be copied > > on the bugs. > > Please use Webtools -> Tinderboxpushlog, no cc necessary. In case you file followups as restricted security bugs, make sure to cc me and mstange. (or is it maybe possible in bugzilla to make every security bug for that product visible to us?)

Fixing this bug would obsolete bug 659724, which is currently closing m-c.

two things I am confused about with this source. I can't find db configuration info. I see where a new mongo instance is instantiated but now where Mongo() is defined or the db config. Also, there are several files that interact with the filesystem. They take a $filesystem variable. I don't see anything using these methods so I am not sure where $filesystem is coming from. When interacting with the filesystem it is very important that we properly sanitize/validate the data to make sure we are not allowing malicious users to interact with the filesystem. The files in question are: LogParser.php, AnnotatedSummaryGenerator.php. GzipUtils.php, ParllelFileGenerating.php, ParsedLogGenerator.php, RawGzLogDownloaader.php

(In reply to comment #4) > two things I am confused about with this source. I can't find db > configuration info. I see where a new mongo instance is instantiated but > now where Mongo() is defined or the db config. Mongo is defined by the MongoDB PHP driver, which is documented here: http://www.php.net/manual/en/book.mongo.php Calling new Mongo() without any configuration info uses the standard host and port which are localhost and 27017, as documented here: http://www.php.net/manual/en/mongo.construct.php > Also, there are several files that interact with the filesystem. They take > a $filesystem variable. I don't see a $filesystem variable anywhere, so I'm assuming you mean $filename. > I don't see anything using these methods so I am > not sure where $filesystem is coming from. Let's look at LogParser::generate($filename). This method is called from ParallelFileGenerating::ensureFileExists($filename, $generator). ensureFileExists is called from many places, but only one caller passes a LogParser instance as the $generator argument, and that caller is in LogParser::ensureExcerptExists. So ensureExcerptExists is the place where the $filename comes from: > $excerptFilename = "../cache/excerpt/".$this->run['_id']."_".$this->lineFilter->getType().".txt.gz"; > ParallelFileGenerating::ensureFileExists($excerptFilename, $this); The other files you mentioned work similarly: They have an ensure*Exists method which chooses a filename and calls ParallelFileGenerating::ensureFileExists with that filename and $this as $generator; and that method passes the filename to the generate method. > When interacting with the > filesystem it is very important that we properly sanitize/validate the data > to make sure we are not allowing malicious users to interact with the > filesystem. Right. Quoting from comment 0: > In the scripts in php/inc/, file names are generated [...] and only make > use of hardcoded strings and numbers (run IDs) from the database.

(In reply to comment #1) > > Please use Webtools -> Tinderboxpushlog, no cc necessary. > > In case you file followups as restricted security bugs, make sure to cc me > and mstange. Raymond, I forgot about security sensitive bugs in comment 0, but Arpad is right: In order to view security sensitive bugs, we need to be CCed. Can you CC us on bug 664099 and bug 664100, please?

ok, this is complete. I will close this bug when the vulnerabilities have been dealt with

Raymond, please see comment 6.

Markus, are you still blocked on being CCed on bug 664099 and bug 664100 ?

No, Gavin has CCed me in the meantime (thanks!).

this has been comleted and verified. closing now.

Please may someone with webtools-security CC me on bug 664099 (or ideally open it up presuming it was fixed years ago).