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

3 files changed, 36 insertions, 17 deletions

diff --git a/nova/tests/test_virt.py b/nova/tests/test_virt.py
index 360e4bfbf..1a1e4cd40 100644
--- a/nova/tests/test_virt.py
+++ b/nova/tests/test_virt.py
@@ -99,22 +99,6 @@ class TestVirtDisk(test.TestCase):
 
         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):
@@ -165,3 +149,32 @@ class TestVirtDisk(test.TestCase):
         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 5bf4468c1..6362d8ff5 100644
--- a/nova/tests/test_xenapi.py
+++ b/nova/tests/test_xenapi.py
@@ -677,9 +677,13 @@ class XenAPIVMTestCase(stubs.XenAPITestBase):
             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,
diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py
index 373c4fa52..5d3c9c6c9 100644
--- a/nova/virt/disk/api.py
+++ b/nova/virt/disk/api.py
@@ -363,7 +363,9 @@ def _join_and_check_path_within_fs(fs, *args):
     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