From 60167d0b3721027c764e5daccf9354b12c5320dc Mon Sep 17 00:00:00 2001 From: Ben Nemec Date: Thu, 21 Aug 2014 22:20:50 +0000 Subject: [PATCH] Address some potential security issues in lockutils Adds a note to the lock_path help text explaining how to secure the target directory. Also opens lock files in append mode so there is no possibility of overwriting a file due to a malicious symlink. Change-Id: I77b72b20088fe66b573c23bd1fd98376c2b0f168 --- oslo/concurrency/lockutils.py | 9 +++++++-- tests/unit/test_lockutils.py | 9 +++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/oslo/concurrency/lockutils.py b/oslo/concurrency/lockutils.py index 23bdc44..4976f15 100644 --- a/oslo/concurrency/lockutils.py +++ b/oslo/concurrency/lockutils.py @@ -40,7 +40,9 @@ util_opts = [ help='Enables or disables inter-process locks.'), cfg.StrOpt('lock_path', default=os.environ.get("OSLO_LOCK_PATH"), - help='Directory to use for lock files.') + help='Directory to use for lock files. For security, the ' + 'specified directory should only be writable by the user ' + 'running the processes that need locking.') ] @@ -81,7 +83,10 @@ class _FileLock(object): fileutils.ensure_tree(basedir) LOG.info(_LI('Created lock path: %s'), basedir) - self.lockfile = open(self.fname, 'w') + # Open in append mode so we don't overwrite any potential contents of + # the target file. This eliminates the possibility of an attacker + # creating a symlink to an important file in our lock_path. + self.lockfile = open(self.fname, 'a') while True: try: diff --git a/tests/unit/test_lockutils.py b/tests/unit/test_lockutils.py index 86593b1..1715050 100644 --- a/tests/unit/test_lockutils.py +++ b/tests/unit/test_lockutils.py @@ -427,6 +427,15 @@ class FileBasedLockingTestCase(test_base.BaseTestCase): thread.join() self.assertEqual(call_list, ['other', 'other', 'main', 'main']) + def test_non_destructive(self): + lock_file = os.path.join(self.lock_dir, 'not-destroyed') + with open(lock_file, 'w') as f: + f.write('test') + with lockutils.lock('not-destroyed', external=True, + lock_path=self.lock_dir): + with open(lock_file) as f: + self.assertEqual(f.read(), 'test') + class LockutilsModuleTestCase(test_base.BaseTestCase):