[PATCH 0/1] Add deletion safeguard

[PATCH 0/1] Add deletion safeguard

[Paul Eggleton]

Try to guard against any repeat of bug 7620:

  https://bugzilla.yoctoproject.org/show_bug.cgi?id=7620

It won't protect usage of shutil.rmtree(), but there's not a lot
we can do about that and we use both of the functions that are protected
extensively throughout BitBake/OE.


The following change since commit e2879c60e905d7566091d40eab330372fa001313:

  bitbake-user-manual: Cleaned up parallelism note and formatted user input (2015-04-17 13:28:31 +0100)

is available in the git repository at:

  git://git.yoctoproject.org/poky-contrib paule/rm-safeguard
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=paule/rm-safeguard

Paul Eggleton (1):
  lib/bb/utils: add safeguard against recursively deleting things we shouldn't

 lib/bb/tests/utils.py | 17 +++++++++++++++++
 lib/bb/utils.py       | 21 +++++++++++++++++++++
 2 files changed, 38 insertions(+)

-- 
2.1.0

[PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't

[Paul Eggleton]

Add some very basic safeguard against recursively deleting paths such
as / and /home in the event of bugs or user mistakes.

Addresses [YOCTO #7620].

Signed-off-by: Paul Eggleton <paul.eggleton@linux.intel.com>
---
 lib/bb/tests/utils.py | 17 +++++++++++++++++
 lib/bb/utils.py       | 21 +++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/lib/bb/tests/utils.py b/lib/bb/tests/utils.py
index 507de2d..6e09858 100644
--- a/lib/bb/tests/utils.py
+++ b/lib/bb/tests/utils.py
@@ -21,6 +21,7 @@
 
 import unittest
 import bb
+import os
 
 class VerCmpString(unittest.TestCase):
 
@@ -88,3 +89,19 @@ class VerCmpString(unittest.TestCase):
 
         # Check that clearly invalid operator raises an exception
         self.assertRaises(bb.utils.VersionStringException, bb.utils.vercmp_string_op, '0', '0', '$')
+
+
+class Path(unittest.TestCase):
+    def test_unsafe_delete_path(self):
+        checkitems = [('/', True),
+                      ('//', True),
+                      ('///', True),
+                      (os.getcwd().count(os.sep) * ('..' + os.sep), True),
+                      (os.environ.get('HOME', '/home/test'), True),
+                      ('/home/someone', True),
+                      ('/home/other/', True),
+                      ('/home/other/subdir', False),
+                      ('', False)]
+        for arg1, correctresult in checkitems:
+            result = bb.utils._check_unsafe_delete_path(arg1)
+            self.assertEqual(result, correctresult, '_check_unsafe_delete_path("%s") != %s' % (arg1, correctresult))
diff --git a/lib/bb/utils.py b/lib/bb/utils.py
index 5ac9bcf..c97f3ef 100644
--- a/lib/bb/utils.py
+++ b/lib/bb/utils.py
@@ -601,11 +601,30 @@ def build_environment(d):
         if export:
             os.environ[var] = d.getVar(var, True) or ""
 
+def _check_unsafe_delete_path(path):
+    """
+    Basic safeguard against recursively deleting something we shouldn't. If it returns True,
+    the caller should raise an exception with an appropriate message.
+    NOTE: This is NOT meant to be a security mechanism - just a guard against silly mistakes
+    with potentially disastrous results.
+    """
+    extra = ''
+    # HOME might not be /home/something, so in case we can get it, check against it
+    homedir = os.environ.get('HOME', '')
+    if homedir:
+        extra = '|%s' % homedir
+    if re.match('(/|//|/home|/home/[^/]*%s)$' % extra, os.path.abspath(path)):
+        return True
+    return False
+
 def remove(path, recurse=False):
     """Equivalent to rm -f or rm -rf"""
     if not path:
         return
     if recurse:
+        for name in glob.glob(path):
+            if _check_unsafe_delete_path(path):
+                raise Exception('bb.utils.remove: called with dangerous path "%s" and recurse=True, refusing to delete!' % path)
         # shutil.rmtree(name) would be ideal but its too slow
         subprocess.call(['rm', '-rf'] + glob.glob(path))
         return
@@ -619,6 +638,8 @@ def remove(path, recurse=False):
 def prunedir(topdir):
     # Delete everything reachable from the directory named in 'topdir'.
     # CAUTION:  This is dangerous!
+    if _check_unsafe_delete_path(topdir):
+        raise Exception('bb.utils.prunedir: called with dangerous path "%s", refusing to delete!' % topdir)
     for root, dirs, files in os.walk(topdir, topdown = False):
         for name in files:
             os.remove(os.path.join(root, name))
-- 
2.1.0

Re: [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't

[Trevor Woerner]

On 15-04-17 10:26 AM, Paul Eggleton wrote:
> Add some very basic safeguard against recursively deleting paths such
> as / and /home in the event of bugs or user mistakes.
>

I liked Nicolas Dechesne's idea of only allowing deletion in paths that 
include TMPDIR or TOPDIR. Is there any reason bitbake would need to 
delete (potentially dangerous) paths anywhere else?

With a list-based approach the list will keep growing indefinitely. 
Inevitably an ex-Solaris person will come along (for example) and have 
their /home directories in /export/home and need a patch for that, etc, 
etc...

Re: [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't

[Paul Eggleton]

On Monday 20 April 2015 09:46:19 Trevor Woerner wrote:
> On 15-04-17 10:26 AM, Paul Eggleton wrote:
> > Add some very basic safeguard against recursively deleting paths such
> > as / and /home in the event of bugs or user mistakes.
> 
> I liked Nicolas Dechesne's idea of only allowing deletion in paths that
> include TMPDIR or TOPDIR. Is there any reason bitbake would need to
> delete (potentially dangerous) paths anywhere else?

Unfortunately this would be problematic for people who decide to split out 
directories managed by the build system to different paths (which they often do 
when part of the system has to be shared over NFS, or to put part of TMPDIR on 
a larger disk, or spread between SSDs and rotational media, etc.). Another 
issue is that the functions in question don't actually have access to d (the 
datastore) and thus can't actually get the value of these variables, at least 
with the way the code is currently structured.

> With a list-based approach the list will keep growing indefinitely.
> Inevitably an ex-Solaris person will come along (for example) and have
> their /home directories in /export/home and need a patch for that, etc,
> etc...

It's not all-encompassing, I agree. I'm all for improving it but unfortunately 
restricting to TMPDIR/TOPDIR will break for a lot of users.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't

[Richard Purdie]

On Mon, 2015-04-20 at 14:59 +0100, Paul Eggleton wrote:
> On Monday 20 April 2015 09:46:19 Trevor Woerner wrote:
> > On 15-04-17 10:26 AM, Paul Eggleton wrote:
> > > Add some very basic safeguard against recursively deleting paths such
> > > as / and /home in the event of bugs or user mistakes.
> > 
> > I liked Nicolas Dechesne's idea of only allowing deletion in paths that
> > include TMPDIR or TOPDIR. Is there any reason bitbake would need to
> > delete (potentially dangerous) paths anywhere else?
> 
> Unfortunately this would be problematic for people who decide to split out 
> directories managed by the build system to different paths (which they often do 
> when part of the system has to be shared over NFS, or to put part of TMPDIR on 
> a larger disk, or spread between SSDs and rotational media, etc.). Another 
> issue is that the functions in question don't actually have access to d (the 
> datastore) and thus can't actually get the value of these variables, at least 
> with the way the code is currently structured.
> 
> > With a list-based approach the list will keep growing indefinitely.
> > Inevitably an ex-Solaris person will come along (for example) and have
> > their /home directories in /export/home and need a patch for that, etc,
> > etc...
> 
> It's not all-encompassing, I agree. I'm all for improving it but unfortunately 
> restricting to TMPDIR/TOPDIR will break for a lot of users.

I liked his idea too however we use these functions against SSTATE_DIR
and DL_DIR which are often not in TMPDIR/TOPDIR. Yes, we could start
making a list of exceptions but I don't think it will scale well, not
withstanding the other technical issues Paul mentions.

Cheers,

Richard