CVE-2012-3447: Permissions, Privileges, and Access Controls (CWE-264)
value: virt/disk/api.py in OpenStack Compute (Nova) 2012.1.x before 2012.1.2 and Folsom before Folsom-3 allows remote authenticated users to overwrite arbitrary files via a symlink attack on a file in an image that uses a symlink that is only readable by root.  NOTE: this vulnerability exists because of an incomplete fix for CVE-2012-3361.
CWE description: Weaknesses in this category are related to the management of permissions, privileges, and other security features that are used to perform access control.
Published: 2012-08-20T18:55Z
Repository: https://github.com/openstack/nova
Language: Python
Files changed: nova/tests/test_virt.py, nova/tests/test_xenapi.py, nova/virt/disk/api.py
Commit date: 2012-08-07 20:02:06 +0100
Commit message: Prohibit file injection writing to host filesystem

This is a refinement of the previous fix in commit 2427d4a9,
which does the file name canonicalization as the root user.
This is required so that guest images could not for example,
protect malicious symlinks in a directory only readable by root.

Fixes bug: 1031311, CVE-2012-3447
Change-Id: I7f7cdeeffadebae7451e1e13f73f1313a7df9c5c

Diff: diff --git a/nova/tests/test_virt.py b/nova/tests/test_virt.py
index 360e4bfbf6..1a1e4cd40d 100644
--- a/nova/tests/test_virt.py
+++ b/nova/tests/test_virt.py
@@ -97,26 +97,10 @@ class TestVirtDisk(test.TestCase):
             self.executes.append(cmd)
             return None, None
 
         self.stubs.Set(utils, 'execute', fake_execute)
 
-    def test_check_safe_path(self):
-        ret = disk_api._join_and_check_path_within_fs('/foo', 'etc',
-                                                      'something.conf')
-        self.assertEquals(ret, '/foo/etc/something.conf')
-
-    def test_check_unsafe_path(self):
-        self.assertRaises(exception.Invalid,
-                          disk_api._join_and_check_path_within_fs,
-                          '/foo', 'etc/../../../something.conf')
-
-    def test_inject_files_with_bad_path(self):
-        self.assertRaises(exception.Invalid,
-                          disk_api._inject_file_into_fs,
-                          '/tmp', '/etc/../../../../etc/passwd',
-                          'hax')
-
     def test_lxc_destroy_container(self):
 
         def proc_mounts(self, mount_point):
             mount_points = {
                 '/mnt/loop/nopart': '/dev/loop0',
@@ -163,5 +147,34 @@ class TestVirtDisk(test.TestCase):
                              ]
         # It's not worth trying to match the last timeout command
         self.executes.pop()
 
         self.assertEqual(self.executes, expected_commands)
+
+
+class TestVirtDiskPaths(test.TestCase):
+    def setUp(self):
+        super(TestVirtDiskPaths, self).setUp()
+
+        real_execute = utils.execute
+
+        def nonroot_execute(*cmd_parts, **kwargs):
+            kwargs.pop('run_as_root', None)
+            return real_execute(*cmd_parts, **kwargs)
+
+        self.stubs.Set(utils, 'execute', nonroot_execute)
+
+    def test_check_safe_path(self):
+        ret = disk_api._join_and_check_path_within_fs('/foo', 'etc',
+                                                      'something.conf')
+        self.assertEquals(ret, '/foo/etc/something.conf')
+
+    def test_check_unsafe_path(self):
+        self.assertRaises(exception.Invalid,
+                          disk_api._join_and_check_path_within_fs,
+                          '/foo', 'etc/../../../something.conf')
+
+    def test_inject_files_with_bad_path(self):
+        self.assertRaises(exception.Invalid,
+                          disk_api._inject_file_into_fs,
+                          '/tmp', '/etc/../../../../etc/passwd',
+                          'hax')
diff --git a/nova/tests/test_xenapi.py b/nova/tests/test_xenapi.py
index 5bf4468c14..6362d8ff59 100644
--- a/nova/tests/test_xenapi.py
+++ b/nova/tests/test_xenapi.py
@@ -675,13 +675,17 @@ class XenAPIVMTestCase(stubs.XenAPITestBase):
                 'dns-nameservers 192.168.1.3 192.168.1.4',
                 ''])
             self._tee_executed = True
             return '', ''
 
+        def _readlink_handler(cmd_parts, **kwargs):
+            return os.path.realpath(cmd_parts[2]), ''
+
         fake_utils.fake_execute_set_repliers([
             # Capture the tee .../etc/network/interfaces command
             (r'tee.*interfaces', _tee_handler),
+            (r'readlink -nm.*', _readlink_handler),
         ])
         self._test_spawn(IMAGE_MACHINE,
                          IMAGE_KERNEL,
                          IMAGE_RAMDISK,
                          check_injection=True)
diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py
index 373c4fa522..5d3c9c6c9c 100644
--- a/nova/virt/disk/api.py
+++ b/nova/virt/disk/api.py
@@ -361,11 +361,13 @@ def _join_and_check_path_within_fs(fs, *args):
     Join the supplied path components and make sure that the
     resulting path we are injecting into is within the
     mounted guest fs.  Trying to be clever and specifying a
     path with '..' in it will hit this safeguard.
     '''
-    absolute_path = os.path.realpath(os.path.join(fs, *args))
+    absolute_path, _err = utils.execute('readlink', '-nm',
+                                        os.path.join(fs, *args),
+                                        run_as_root=True)
     if not absolute_path.startswith(os.path.realpath(fs) + '/'):
         raise exception.Invalid(_('injected file path not valid'))
     return absolute_path
 
 

Vulnerable code:     def test_check_safe_path(self):
        ret = disk_api._join_and_check_path_within_fs('/foo', 'etc',
                                                      'something.conf')
        self.assertEquals(ret, '/foo/etc/something.conf')

    def test_check_unsafe_path(self):
        self.assertRaises(exception.Invalid,
                          disk_api._join_and_check_path_within_fs,
                          '/foo', 'etc/../../../something.conf')

    def test_inject_files_with_bad_path(self):
        self.assertRaises(exception.Invalid,
                          disk_api._inject_file_into_fs,
                          '/tmp', '/etc/../../../../etc/passwd',
                          'hax')

    absolute_path = os.path.realpath(os.path.join(fs, *args))
Fixed code: 

class TestVirtDiskPaths(test.TestCase):
    def setUp(self):
        super(TestVirtDiskPaths, self).setUp()

        real_execute = utils.execute

        def nonroot_execute(*cmd_parts, **kwargs):
            kwargs.pop('run_as_root', None)
            return real_execute(*cmd_parts, **kwargs)

        self.stubs.Set(utils, 'execute', nonroot_execute)

    def test_check_safe_path(self):
        ret = disk_api._join_and_check_path_within_fs('/foo', 'etc',
                                                      'something.conf')
        self.assertEquals(ret, '/foo/etc/something.conf')

    def test_check_unsafe_path(self):
        self.assertRaises(exception.Invalid,
                          disk_api._join_and_check_path_within_fs,
                          '/foo', 'etc/../../../something.conf')

    def test_inject_files_with_bad_path(self):
        self.assertRaises(exception.Invalid,
                          disk_api._inject_file_into_fs,
                          '/tmp', '/etc/../../../../etc/passwd',
                          'hax')
        def _readlink_handler(cmd_parts, **kwargs):
            return os.path.realpath(cmd_parts[2]), ''

            (r'readlink -nm.*', _readlink_handler),
    absolute_path, _err = utils.execute('readlink', '-nm',
                                        os.path.join(fs, *args),
                                        run_as_root=True)