592512 - use unique resultfile for 'cfx run', not always /tmp/harness_result

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Brian Warner [:warner :bwarner]]

python-lib/cuddlefish/runner.py (around line 248) uses the following code to
come up with a "resultfile" name: a file into which the spawned
firefox/xulrunner can write its test results for later processing:

  resultfile = os.path.join(tempfile.gettempdir(), 'harness_result')

This always uses "/tmp/harness_result", with two unfortunate consequences:

 1: two users on the same host cannot run "cfx test" or "cfx run" at the same
    time, since they'll overwrite each others resultfiles
 2: it enables a "symlink attack", in which an attacker does something like
    "ln -s /tmp/harness_result ~you/.ssh/id_dsa" and tricks you into
    corrupting or deleting your private key, even though they cannot touch
    that file directly. Other forms of this attack are even worse, such as if
    the contents of the resultfile are interpreted by some other tool in a
    troubling way (think .rhosts). Sometimes this attack requires winning a
    race, in which the attacker keeps re-creating the symlink in a tight
    loop, but that's pretty easy in practice.

Both problems are fixed by using unguessable/unique tempfile names. (it is
also a good idea to put these tempfiles in the user's home directory, rather
than in a shared world-writable /tmp directory).

runner.py should be changed to use:

 f = tempfile.NamedTemporaryFile(prefix="harness-result-", delete=False)
 f.close()
 resultfile = f.name
 ... # run xulrunner, tell it to write into resultfile
 ... # read from resultfile
 os.unlink(resultfile)

The code that manages "harness_log" should be changed in the same way.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 2

[Brian Warner [:warner :bwarner]]

Attachment: use unique tempfile for results and logfile

Comment 3

[Brian Warner [:warner :bwarner]]

Oh, BTW, this patch is also in my '592512-resultfile' branch on github.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 531202 [details] [diff] [review]
use unique tempfile for results and logfile

Review of attachment 531202 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 5

[Brian Warner [:warner :bwarner]]

Great, landed in https://github.com/mozilla/addon-sdk/commit/214de059d9b168ea83fc0d604d433f882c1b192a

Comment 6

[Brian Warner [:warner :bwarner]]

Whoops, it turns out that my patch introduced a dependency upon python2.6 (since NamedTemporaryFile didn't acquire a delete= argument until py2.6), making tests fail on our py2.5 buildbots. Reverted in https://github.com/mozilla/addon-sdk/commit/3ec8553ceaffbc2e8ac858864bea295e98bf08dd . I'll work on a py2.5-safe patch.

Comment 7

[Brian Warner [:warner :bwarner]]

Attachment: py2.5-safe patch

Also visible as https://github.com/mozilla/addon-sdk/pull/169

This version is good for pythons all the way back to 2.3 .

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 533104 [details] [diff] [review]
py2.5-safe patch

Review of attachment 533104 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 9

[gilma gamez lopez]

Comment on attachment 533104 [details] [diff] [review]
py2.5-safe patch

Review of attachment 533104 [details] [diff] [review]:
-----------------------------------------------------------------

https://wiki.mozilla.org/User:Gilma.gamez

Comment 10

[Brian Warner [:warner :bwarner]]

Great, landed in https://github.com/mozilla/addon-sdk/commit/e74ca6441a8fdeb44052b1ac138b8f6751781699 .