[PATCH] patch.bbclass: Use one TMPDIR per patching process

[PATCH] patch.bbclass: Use one TMPDIR per patching process

[Constantin Musca]

We must use one TMPDIR per process (/tmp/${PID}) so that the patching
processes don't generate the same temp file name (the "patch" program
uses the TMPDIR environment variable for deciding where to create the
temp files).

[YOCTO #3070]

Signed-off-by: Constantin Musca <constantinx.musca@intel.com>
---
 meta/classes/patch.bbclass |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/meta/classes/patch.bbclass b/meta/classes/patch.bbclass
index a724972..d010438 100644
--- a/meta/classes/patch.bbclass
+++ b/meta/classes/patch.bbclass
@@ -139,6 +139,13 @@ python patch_do_patch() {
     path = os.getenv('PATH')
     os.putenv('PATH', d.getVar('PATH', True))
 
+    import shutil
+    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
+    if os.path.exists(process_tmpdir):
+        shutil.rmtree(process_tmpdir)
+    os.makedirs(process_tmpdir)
+    os.environ['TMPDIR'] = process_tmpdir
+
     for patch in src_patches(d):
         _, _, local, _, _, parm = bb.decodeurl(patch)
 
@@ -161,11 +168,15 @@ python patch_do_patch() {
         try:
             patchset.Import({"file":local, "strippath": parm['striplevel']}, True)
         except Exception as exc:
+            shutil.rmtree(process_tmpdir)
             bb.fatal(str(exc))
         try:
             resolver.Resolve()
         except bb.BBHandledException as e:
+            shutil.rmtree(process_tmpdir)
             bb.fatal(str(e))
+
+    shutil.rmtree(process_tmpdir)
 }
 patch_do_patch[vardepsexclude] = "PATCHRESOLVE"
 
-- 
1.7.9.5

Re: [PATCH] patch.bbclass: Use one TMPDIR per patching process

[Enrico Scholz]

Constantin Musca
<constantinx.musca-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:

> +    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
> +    if os.path.exists(process_tmpdir):
> +        shutil.rmtree(process_tmpdir)
> +    os.makedirs(process_tmpdir)

ooohhhh... this violates trivial rules regarding secure generation of
tempfiles. Better use 'mkdtemp()' from the 'tempfile' module.


Enrico

Re: [PATCH] patch.bbclass: Use one TMPDIR per patching process

[Richard Purdie]

On Fri, 2012-09-14 at 13:28 +0200, Enrico Scholz wrote:
> Constantin Musca
> <constantinx.musca-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
> 
> > +    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
> > +    if os.path.exists(process_tmpdir):
> > +        shutil.rmtree(process_tmpdir)
> > +    os.makedirs(process_tmpdir)
> 
> ooohhhh... this violates trivial rules regarding secure generation of
> tempfiles. Better use 'mkdtemp()' from the 'tempfile' module.

The problem is that the internal temp directory creation inside patch
can be broken. We *really* don't want to start building patch-native so
this workaround gives patch a fighting chance of not conflicting with
other instances of itself. Its only being used as a prefix, not as the
full directory path name so it isn't quite as insecure as it would first
appear.

I'm fine if we want to use the mkdtemp approach though and further
randomise this. I'd also suggest any updated version adds a comment to
the code about *why* we need a separate TMPDIR and which versions of
patch have this problem.

Cheers,

Richard

Re: [PATCH] patch.bbclass: Use one TMPDIR per patching process

[Enrico Scholz]

Richard Purdie <richard.purdie@linuxfoundation.org> writes:

>> > +    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
>> > +        shutil.rmtree(process_tmpdir)

> Its only being used as a prefix, not as the full directory path name
> so it isn't quite as insecure as it would first appear.

It *is* insecure as it would first appear.  'shutil.rmtree()' does not
traverse the directory in a secure way so that an attacker could:

1. touch /tmp/<2-32767>/a
2. mkdir /tmp/<2-32767>/Z
3. wait for an inotify which triggers on deletion of the 'a' files
4. rmdir /tmp/$dir/Z
5. ln -s /home/<user> /tmp/$dir/Z

When steps 4+5 are executed between

| $ strace python -c 'import shutil; shutil.rmtree("/tmp/2");'
| ...
| lstat("/tmp/2/Z", {st_mode=S_IFDIR|0755, st_size=60, ...}) = 0
| <<<< steps 4+5 here >>>>
| openat(AT_FDCWD, "/tmp/2/Z", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3
| getdents(3, /* 3 entries */, 32768)     = 72
| ...
| unlink("/tmp/2/Z/foo")                  = 0

user writable directories will be removed.

There have been established some rules regarding secure tmpfile/dir
generation in the last 10-20 years which should never be violated.


Beside the obvious security issues, build will be aborted when somebody
else creates a /tmp/<number> file and <number> matches the bitbake pid.


Enrico

Re: [PATCH] patch.bbclass: Use one TMPDIR per patching process

[Richard Purdie]

On Fri, 2012-09-14 at 14:24 +0200, Enrico Scholz wrote:
> Richard Purdie <richard.purdie@linuxfoundation.org> writes:
> 
> >> > +    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
> >> > +        shutil.rmtree(process_tmpdir)
> 
> > Its only being used as a prefix, not as the full directory path name
> > so it isn't quite as insecure as it would first appear.
> 
> It *is* insecure as it would first appear.  'shutil.rmtree()' does not
> traverse the directory in a secure way so that an attacker could:
> 
> 1. touch /tmp/<2-32767>/a
> 2. mkdir /tmp/<2-32767>/Z
> 3. wait for an inotify which triggers on deletion of the 'a' files
> 4. rmdir /tmp/$dir/Z
> 5. ln -s /home/<user> /tmp/$dir/Z
> 
> When steps 4+5 are executed between
> 
> | $ strace python -c 'import shutil; shutil.rmtree("/tmp/2");'
> | ...
> | lstat("/tmp/2/Z", {st_mode=S_IFDIR|0755, st_size=60, ...}) = 0
> | <<<< steps 4+5 here >>>>
> | openat(AT_FDCWD, "/tmp/2/Z", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3
> | getdents(3, /* 3 entries */, 32768)     = 72
> | ...
> | unlink("/tmp/2/Z/foo")                  = 0
> 
> user writable directories will be removed.
> 
> There have been established some rules regarding secure tmpfile/dir
> generation in the last 10-20 years which should never be violated.
> 
> 
> Beside the obvious security issues, build will be aborted when somebody
> else creates a /tmp/<number> file and <number> matches the bitbake pid.

Firstly, I agree that we need to fix this and I know Constantin is
working on a patch. 

I would point out that the build process is likely full of such races
though. We execute an absolute *ton* of code, much of which is part of
upstream projects and which we don't directly control (think of all the
configure scripts and makefiles). I'd therefore suggest that builds be
considered insecure in themselves and run in environments appropriate to
that.

So build time security, I make *no* claims to and I find it hard to get
worked up about this lest it create some illusion builds are "secure".
Runtime security of the build output, very different question,
naturally.

Cheers,

Richard

Re: [PATCH] patch.bbclass: Use one TMPDIR per patching process

[Enrico Scholz]

Richard Purdie <richard.purdie@linuxfoundation.org> writes:

> I would point out that the build process is likely full of such races
> though. 

Yes; I know.  But there is really no excuse to introduce insecure tmpfile
creation; especially because safe techniques are well known, available
and cheap.

All the build tools (gcc, make, autoconf) got patches in the last years
to avoid such races.


Enrico

Re: [PATCH] patch.bbclass: Use one TMPDIR per patching process

[Saul Wold]

On 09/12/2012 04:58 AM, Constantin Musca wrote:
> We must use one TMPDIR per process (/tmp/${PID}) so that the patching
> processes don't generate the same temp file name (the "patch" program
> uses the TMPDIR environment variable for deciding where to create the
> temp files).
>
> [YOCTO #3070]
>
> Signed-off-by: Constantin Musca <constantinx.musca@intel.com>
> ---
>   meta/classes/patch.bbclass |   11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/meta/classes/patch.bbclass b/meta/classes/patch.bbclass
> index a724972..d010438 100644
> --- a/meta/classes/patch.bbclass
> +++ b/meta/classes/patch.bbclass
> @@ -139,6 +139,13 @@ python patch_do_patch() {
>       path = os.getenv('PATH')
>       os.putenv('PATH', d.getVar('PATH', True))
>
> +    import shutil
> +    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
> +    if os.path.exists(process_tmpdir):
> +        shutil.rmtree(process_tmpdir)
> +    os.makedirs(process_tmpdir)
> +    os.environ['TMPDIR'] = process_tmpdir
> +
>       for patch in src_patches(d):
>           _, _, local, _, _, parm = bb.decodeurl(patch)
>   On 09/12/2012 04:58 AM, Constantin Musca wrote:> We must use one TMPDIR per process (/tmp/${PID}) so that the patching
> processes don't generate the same temp file name (the "patch" program
> uses the TMPDIR environment variable for deciding where to create the
> temp files).
>
> [YOCTO #3070]
>
> Signed-off-by: Constantin Musca <constantinx.musca@intel.com>
> ---
>   meta/classes/patch.bbclass |   11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/meta/classes/patch.bbclass b/meta/classes/patch.bbclass
> index a724972..d010438 100644
> --- a/meta/classes/patch.bbclass
> +++ b/meta/classes/patch.bbclass
> @@ -139,6 +139,13 @@ python patch_do_patch() {
>       path = os.getenv('PATH')
>       os.putenv('PATH', d.getVar('PATH', True))
>
> +    import shutil
> +    process_tmpdir = os.path.join('/tmp', str(os.getpid()))
> +    if os.path.exists(process_tmpdir):
> +        shutil.rmtree(process_tmpdir)
> +    os.makedirs(process_tmpdir)
> +    os.environ['TMPDIR'] = process_tmpdir
> +
>       for patch in src_patches(d):
>           _, _, local, _, _, parm = bb.decodeurl(patch)
>
> @@ -161,11 +168,15 @@ python patch_do_patch() {
>           try:
>               patchset.Import({"file":local, "strippath": parm['striplevel']}, True)
>           except Exception as exc:
> +            shutil.rmtree(process_tmpdir)
>               bb.fatal(str(exc))
>           try:
>               resolver.Resolve()
>           except bb.BBHandledException as e:
> +            shutil.rmtree(process_tmpdir)
>               bb.fatal(str(e))
> +
> +    shutil.rmtree(process_tmpdir)
>   }
>   patch_do_patch[vardepsexclude] = "PATCHRESOLVE"
>
>

This initial version was merged into OE-Core

Thanks
	Sau!

> @@ -161,11 +168,15 @@ python patch_do_patch() {
>           try:
>               patchset.Import({"file":local, "strippath": parm['striplevel']}, True)
>           except Exception as exc:
> +            shutil.rmtree(process_tmpdir)
>               bb.fatal(str(exc))
>           try:
>               resolver.Resolve()
>           except bb.BBHandledException as e:
> +            shutil.rmtree(process_tmpdir)
>               bb.fatal(str(e))
> +
> +    shutil.rmtree(process_tmpdir)
>   }
>   patch_do_patch[vardepsexclude] = "PATCHRESOLVE"
>
>