[MERGE] OSX's mkdtemp assigns wrong group by default

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Guillermo Gonzalez wrote:
> Hi,
...

>>
>>>     john> dir = mkdtemp()
>>>     john> if sys.platform == 'darwin':
>>>     john>   # OS X creates temp dirs with the 'wheel' group, which users are not likely
>>>     john>   # to be in, and this prevents us from setting the sgid bit
>>>     john>   os.chown(dir, os.getuid(), os.getgid())
>>>
>>> Conditional logic in tests ? >-/

Well, one of your alternatives was to detect that the platform is 'darwin' and
raise either UnavailableFeature or TestNotApplicable. That is *still*
conditional logic in the test. I honestly don't see what it is such a big
problem. You are just changing where the logic is.

>>>
>>>     john> I would certainly be more comfortable modifying the one
>>>     john> test that needs group, rather than everytime someone
>>>     john> creates a temp dir.
>>>
>>> But then we risk writing tests suffering from the same problem.
>> And then we fix specific tests that need it, but I honestly think that
>> actively chowning mkdtemp would lead to security holes which is worse than
>> having a test have a bit of platform specific logic.
>> If we have other tests crop up, then we can revisit it. But a single test
>> needing this does not necessitate doing it everywhere.
>>
> 
> I wasn't aware of possible security holes related to this, at the time
> I added it.
> 
> I'm thinking that a possible solution, to avoid conditionals in the
> tests, could be adding an argument to osutils.mkdtemp, i.e:
> osutils.mkdtemp(override_ug_id=False)
> 
> This would allow a specific test to override uid/guid, and also
> keeping the current behaviour in all other test.
> 
> any thoughts?
> 
> Cheers,
> 
> --
> Guillermo

So, the only places where bzr now creates temp files outside the test suite is:

1) btree code, when spilling extra pages to disk. (Uses NamedTemporaryFile)
2) TransformPreview, uses mkdtemp to save a place to work in, I think because
it doesn't want to pollute .bzr/checkout/limbo (especially as it can do
multiple stacking of previews, at least, Aaron *wants* to do that.)
3) diff.py, uses mkstemp to spawn an external tool, and mkdtemp to build up a
full tree, to then give to an external tool
4) bzrlib.remote, uses tempfile.NamedTemporaryFile to spool the result of
GetRepositoryTarball, though that code path is no longer actively used. (Also
used mkdtemp during *creation* of said tarball.)

so the (2) and (4) are using mkdtemp() in such a way that someone could
possibly inject bogus data into it if we weren't careful. chown(getgid())
doesn't seem particularly dangerous, since it has the access rights of the
current process. It just doesn't seem specifically necessary.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIuutDJdeBCYSNAAMRAlgDAJ0QEkTLQSlUEoFmR7EB5wzdPbYY4wCePQ1G
ncdxLlzUWWV9nIjlhYc828c=
=QsBC
-----END PGP SIGNATURE-----