issue ------ data is being inserted into the database without any validation. This could lead to potential compromises as malicious data could be stored in the DB to be later read by an administrator. Or this data could be presented back to users at some point. This was found in submitBuildStar.php and updateBuilders.php Code Examples ------------------- updateBuilders.php 41 $who = $_POST['who']; 56 $historyEntry = array( 57 'date' => time(), 58 'action' => $action, 59 'who' => $who, 60 'reason' => $reason, 61 'ip' => $ip); 62 $mongo->tbpl->builders->update( 63 array('name' => $name), 64 array('$set' => array('hidden' => $newHidden), 65 '$push' => array('history' => $historyEntry))); submitBuildStar.php 41 $noteObject = array( 42 'who' => $_POST["who"], 43 'note' => $_POST["note"], 44 'timestamp' => time(), 45 'ip' => $_SERVER['REMOTE_ADDR'] 46 ); 47 $mongo->tbpl->runs->update($run, array('$push' => array('notes' => $noteObject))); suggested remediation --------------------- all data should be validated before being submitted to the database.

(In reply to comment #0) > data is being inserted into the database without any validation. This could > lead to potential compromises as malicious data could be stored in the DB to > be later read by an administrator. Do you have any examples for this? We're saving JSON, which in and of itself is pretty harmless, generally. > Or this data could be presented back to > users at some point. If we do this in a way where something bad can happen, wouldn't that rather be a bug in the code that does the presenting? I'll go through our JavaScript code again to make sure that all strings that end up as HTML are escaped properly, but other than that, I don't see what needs to be done. > This was found in submitBuildStar.php and updateBuilders.php > > > Code Examples > ------------------- > updateBuilders.php > 41 $who = $_POST['who']; > 56 $historyEntry = array( > 57 'date' => time(), > 58 'action' => $action, $action is guaranteed to be either "hide" or "unhide". > 59 'who' => $who, > 60 'reason' => $reason, $reason and $who can be any string (or an array of strings potentially); I don't really see what we could validate here. I agree that it's pretty pointless to allow storing arrays in these fields, even though that shouldn't be able to cause any harm. I can add an is_string($who) validation here, of course. What else is there to validate? Maybe a maximum length so our DB doesn't grow uncontrollably? > 61 'ip' => $ip); > 62 $mongo->tbpl->builders->update( > 63 array('name' => $name), > 64 array('$set' => array('hidden' => $newHidden), > 65 '$push' => array('history' => $historyEntry))); > > submitBuildStar.php > 41 $noteObject = array( > 42 'who' => $_POST["who"], > 43 'note' => $_POST["note"], As above, all of the code should be able to handle arbitrary values here.

(In reply to comment #1) > > Or this data could be presented back to > > users at some point. > > If we do this in a way where something bad can happen, wouldn't that rather > be a bug in the code that does the presenting? > > I'll go through our JavaScript code again to make sure that all strings that > end up as HTML are escaped properly I've done that and found out that we indeed weren't escaping build star strings ("who" and "note") before inserting them into the HTML. Patch is in bug 665787. > $reason and $who can be any string (or an array of strings potentially); ... or even arbitrarily nested arrays: a query with who[names][]=Arpad&who[names][]=Borsos&who[funny]=1 would give $_POST["who"] the value array("names" => array("Arpad", "Borsos") "funny" => "1") I've put up a patch in bug 665792 to only accept strings.

OK, the patch that adds is_string() validation has landed in http://hg.mozilla.org/users/mstange_themasta.com/tbpl-pending-infrasec-review/ and has been deployed on http://tbpl.swatinem.de/ The is_string line is here: http://hg.mozilla.org/users/mstange_themasta.com/tbpl-pending-infrasec-review/annotate/a621fc4bb3de/php/inc/Communication.php#l24 Raymond, is that enough to fix this bug and to finalize the security review, or is there something else that needs to be done?

this looks good to me. closing bug.