Skip to contentSkip to conversation

Add deletion safeguard

PATCH In review Paul Eggleton 3 participants 5 messages Apr 17, 2015 – Apr 20, 2015

lore
Share & tools
lore
#0 lore raw

cover Add deletion safeguard

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(+)
Signature · 1 line
2.1.0
#1 lore raw

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

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

Addresses [YOCTO #7620].

---
 lib/bb/tests/utils.py | 17 +++++++++++++++++
 lib/bb/utils.py       | 21 +++++++++++++++++++++
 2 files changed, 38 insertions(+)
lib/bb/tests/utils.py +17 -0
index 507de2d..6e09858 100644
@@ -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))
lib/bb/utils.py +21 -0
index 5ac9bcf..c97f3ef 100644
@@ -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))
Signature · 1 line
2.1.0
#2 lore raw

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

Quoted 3 lines · Paul Eggleton

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...

#3 lore raw

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

Quoted 7 lines · On Monday 20 April 2015 09:46:19 Trevor Woerner wrote:
Quoted 2 lines · Paul Eggleton

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.

Quoted 4 lines

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

Signature · 2 lines
Paul Eggleton
Intel Open Source Technology Centre
#4 lore raw

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

Quoted 17 lines · Paul Eggleton
Quoted 7 lines · On Monday 20 April 2015 09:46:19 Trevor Woerner wrote:
Quoted 2 lines · Paul Eggleton

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.

Quoted 7 lines
Quoted 4 lines

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