| Submitter | Constantin Musca |
|---|---|
| Date | Sept. 12, 2012, 11:58 a.m. |
| Message ID | <1347451098-16659-1-git-send-email-constantinx.musca@intel.com> |
| Download | mbox | patch |
| Permalink | /patch/36421/ |
| State | Accepted |
| Commit | 16dbf505c4fdd9fe1820d950ab05c8ea99ad7505 |
| Headers | show |
Comments
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
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
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
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
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
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" > >
Patch
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"
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(+)