* [PATCH 0/1] Add deletion safeguard @ 2015-04-17 14:26 Paul Eggleton 2015-04-17 14:26 ` [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't Paul Eggleton 0 siblings, 1 reply; 5+ messages in thread From: Paul Eggleton @ 2015-04-17 14:26 UTC (permalink / raw) To: bitbake-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't 2015-04-17 14:26 [PATCH 0/1] Add deletion safeguard Paul Eggleton @ 2015-04-17 14:26 ` Paul Eggleton 2015-04-20 13:46 ` Trevor Woerner 0 siblings, 1 reply; 5+ messages in thread From: Paul Eggleton @ 2015-04-17 14:26 UTC (permalink / raw) To: bitbake-devel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't 2015-04-17 14:26 ` [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't Paul Eggleton @ 2015-04-20 13:46 ` Trevor Woerner 2015-04-20 13:59 ` Paul Eggleton 0 siblings, 1 reply; 5+ messages in thread From: Trevor Woerner @ 2015-04-20 13:46 UTC (permalink / raw) To: Paul Eggleton, bitbake-devel 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... ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't 2015-04-20 13:46 ` Trevor Woerner @ 2015-04-20 13:59 ` Paul Eggleton 2015-04-20 22:04 ` Richard Purdie 0 siblings, 1 reply; 5+ messages in thread From: Paul Eggleton @ 2015-04-20 13:59 UTC (permalink / raw) To: Trevor Woerner; +Cc: bitbake-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't 2015-04-20 13:59 ` Paul Eggleton @ 2015-04-20 22:04 ` Richard Purdie 0 siblings, 0 replies; 5+ messages in thread From: Richard Purdie @ 2015-04-20 22:04 UTC (permalink / raw) To: Paul Eggleton; +Cc: bitbake-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-04-20 22:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-17 14:26 [PATCH 0/1] Add deletion safeguard Paul Eggleton 2015-04-17 14:26 ` [PATCH 1/1] lib/bb/utils: add safeguard against recursively deleting things we shouldn't Paul Eggleton 2015-04-20 13:46 ` Trevor Woerner 2015-04-20 13:59 ` Paul Eggleton 2015-04-20 22:04 ` Richard Purdie
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.