use of temporary directory can lead to leak-ins

[Nish Aravamudan]

Not sure of the right terminology, but since we use the equivalent of mktemp -d without a template, if there are already, e.g., tarballs in /tmp, then they will get used inadvertently (and not be checked).

It seems like the right approach is something different than we have now, but I'm not sure what yet.

[Christian Ehrhardt  (paelzer)]

If context of the execution dir is breaking us we would want/need fully context depleted environments - like containers. But that pretty fast gets pretty heavy as we already see on build.

I first thought for taballs and such alone which do ../ we could just make a two level tmp dirs.
But we have hit examples of /tmp being used directly (non relative).

I wonder if we should use mount namespace execution like [1].
I'd think of the following on entry
1. make a new empty tmp dir via mktemp
2. make that the /tmp via mount namespaces
3. actual execution

If we identify more dirs we can add them on that mechanism as well.
Should be more efficient than full system containers each time, especially for dependencies just staying as is as long as we don't touch andthing in PATH and ld path.

[1]: https://blog.famzah.net/2014/06/04/private-tmp-mount-per-process-in-linux/

[Nish Aravamudan (nacc)]

On Thu, Jan 11, 2018 at 10:53 PM, ChristianEhrhardt
<email address hidden> wrote:
> If context of the execution dir is breaking us we would want/need fully
> context depleted environments - like containers. But that pretty fast
> gets pretty heavy as we already see on build.

Yeah, we don't quite need that. What we need is an 'empty' location.
The problem was that using an empty location directlly at /tmp, where
.. is used means that stuff in /tmp is seen.

So what we need to do is use `mktemp -d -t gitubuntu.XXXXXX` or so. I
have some patches that start doing this.

> I first thought for taballs and such alone which do ../ we could just make a two level tmp dirs.
> But we have hit examples of /tmp being used directly (non relative).

Well, the intention is to clean this up directly.

> I wonder if we should use mount namespace execution like [1].
> I'd think of the following on entry
> 1. make a new empty tmp dir via mktemp
> 2. make that the /tmp via mount namespaces
> 3. actual execution

The problem is we have to get stuff out of the namespace into the
'top' namespace. I don't think that's trivial to do? Especially when
both might be /tmp?

> If we identify more dirs we can add them on that mechanism as well.
> Should be more efficient than full system containers each time, especially for dependencies just staying as is as long as we don't touch andthing in PATH and ld path.
>
> [1]: https://blog.famzah.net/2014/06/04/private-tmp-mount-per-process-
> in-linux/

Let's see if I can get well-defined semantics going first without namespaces.

-Nish

[Bryce Harrington (bryce)]

I'm not certain which specific part of the code this bug is referring to needing fixed, but here's the current state of mktemp usage in the usd-importer repo:

usd-importer$ grep -sirI mktemp .
./bin/git-merge-changelogs:file_base=`mktemp git-mergechangelogs.XXXXXXXXXX`
./bin/git-merge-changelogs:file_a=`mktemp git-mergechangelogs.XXXXXXXXXX`
./bin/git-merge-changelogs:file_b=`mktemp git-mergechangelogs.XXXXXXXXXX`
./bin/git-reconstruct-changelog:tempfile=`mktemp --tmpdir git-reconstruct-changelog.XXXXXXXXXX`
./bin/bzr-am:tempdir=`mktemp -d`
./bin/git-dsc-commit:tmpdir=`mktemp --tmpdir -d git-dsc-commit.XXXXXXXXXX`
./snap-wrappers/wrappers/git-merge-changelogs:file_base=`mktemp git-mergechangelogs.XXXXXXXXXX`
./snap-wrappers/wrappers/git-merge-changelogs:file_a=`mktemp git-mergechangelogs.XXXXXXXXXX`
./snap-wrappers/wrappers/git-merge-changelogs:file_b=`mktemp git-mergechangelogs.XXXXXXXXXX`
./snap-wrappers/wrappers/git-reconstruct-changelog:tempfile=`mktemp --tmpdir git-reconstruct-changelog.XXXXXXXXXX`
./snap-wrappers/wrappers/git-ubuntu-self-test:tmp_dir=$(mktemp -d -t "ci-$(date +%Y%m%d)-XXXXXXXXXX")

Only the bzr-am script uses a non-templated form of mktemp. That should be an easy fix, although maybe bzr-am is less relevant. In any case probably a non-issue as far as leaks go.

I improved git-ubuntu-self-test in this respect, via commit 992daf18c8b8fec2638bcddf75c660958930674a, which previously had been just doing a $(mktemp) - I needed a few other tmp things so put the lot into a tmp directory that gets trap-clean'ed up at script exit.

So, I suspect the original issue of leaking is now no longer a problem. However it's probably worth a deeper review, and verifying that there is proper error checking on the mktemp call and corresponding cleanups.