| From 0e6b4a06ad72ac68ec41bab2063f8d167e8e277e Mon Sep 17 00:00:00 2001 |
| From: Matthew Booth <mbooth@redhat.com> |
| Date: Thu, 10 Dec 2015 16:34:19 +0000 |
| Subject: [PATCH 2/3] Fix format conversion in libvirt snapshot |
| |
| The libvirt driver was calling images.convert_image during snapshot to |
| convert snapshots to the intended output format. However, this |
| function does not take the input format as an argument, meaning it |
| implicitly does format detection. This opened an exploit for setups |
| using raw storage on the backend, including raw on filesystem, LVM, |
| and RBD (Ceph). An authenticated user could write a qcow2 header to |
| their instance's disk which specified an arbitrary backing file on the |
| host. When convert_image ran during snapshot, this would then write |
| the contents of the backing file to glance, which is then available to |
| the user. If the setup uses an LVM backend this conversion runs as |
| root, meaning the user can exfiltrate any file on the host, including |
| raw disks. |
| |
| This change adds an input format to convert_image. |
| |
| Partial-Bug: #1524274 |
| |
| Change-Id: If73e73718ecd5db262ed9904091024238f98dbc0 |
| (cherry picked from commit 840644d619e9560f205016eafc8799565ffd6d8c) |
| --- |
| nova/tests/unit/virt/libvirt/test_driver.py | 5 +++-- |
| nova/tests/unit/virt/libvirt/test_utils.py | 3 ++- |
| nova/virt/images.py | 26 ++++++++++++++++++++++++-- |
| nova/virt/libvirt/imagebackend.py | 19 ++++++++++++++----- |
| 4 files changed, 43 insertions(+), 10 deletions(-) |
| |
| diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py |
| index 22ef56d..6fd8728 100644 |
| --- a/nova/tests/unit/virt/libvirt/test_driver.py |
| +++ b/nova/tests/unit/virt/libvirt/test_driver.py |
| @@ -14985,7 +14985,7 @@ class LibvirtVolumeSnapshotTestCase(test.NoDBTestCase): |
| self.mox.VerifyAll() |
| |
| |
| -def _fake_convert_image(source, dest, out_format, |
| +def _fake_convert_image(source, dest, in_format, out_format, |
| run_as_root=True): |
| libvirt_driver.libvirt_utils.files[dest] = '' |
| |
| @@ -15127,7 +15127,8 @@ class LVMSnapshotTests(_BaseSnapshotTests): |
| |
| mock_volume_info.assert_has_calls([mock.call('/dev/nova-vg/lv')]) |
| mock_convert_image.assert_called_once_with( |
| - '/dev/nova-vg/lv', mock.ANY, disk_format, run_as_root=True) |
| + '/dev/nova-vg/lv', mock.ANY, 'raw', disk_format, |
| + run_as_root=True) |
| |
| def test_raw(self): |
| self._test_lvm_snapshot('raw') |
| diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py |
| index 6773bea..6f75a92 100644 |
| --- a/nova/tests/unit/virt/libvirt/test_utils.py |
| +++ b/nova/tests/unit/virt/libvirt/test_utils.py |
| @@ -594,7 +594,8 @@ disk size: 4.4M |
| target = 't.qcow2' |
| self.executes = [] |
| expected_commands = [('qemu-img', 'convert', '-O', 'raw', |
| - 't.qcow2.part', 't.qcow2.converted'), |
| + 't.qcow2.part', 't.qcow2.converted', |
| + '-f', 'qcow2'), |
| ('rm', 't.qcow2.part'), |
| ('mv', 't.qcow2.converted', 't.qcow2')] |
| images.fetch_to_raw(context, image_id, target, user_id, project_id, |
| diff --git a/nova/virt/images.py b/nova/virt/images.py |
| index 5b9374b..e2b5b91 100644 |
| --- a/nova/virt/images.py |
| +++ b/nova/virt/images.py |
| @@ -66,9 +66,31 @@ def qemu_img_info(path): |
| return imageutils.QemuImgInfo(out) |
| |
| |
| -def convert_image(source, dest, out_format, run_as_root=False): |
| +def convert_image(source, dest, in_format, out_format, run_as_root=False): |
| """Convert image to other format.""" |
| + if in_format is None: |
| + raise RuntimeError("convert_image without input format is a security" |
| + "risk") |
| + _convert_image(source, dest, in_format, out_format, run_as_root) |
| + |
| + |
| +def convert_image_unsafe(source, dest, out_format, run_as_root=False): |
| + """Convert image to other format, doing unsafe automatic input format |
| + detection. Do not call this function. |
| + """ |
| + |
| + # NOTE: there is only 1 caller of this function: |
| + # imagebackend.Lvm.create_image. It is not easy to fix that without a |
| + # larger refactor, so for the moment it has been manually audited and |
| + # allowed to continue. Remove this function when Lvm.create_image has |
| + # been fixed. |
| + _convert_image(source, dest, None, out_format, run_as_root) |
| + |
| + |
| +def _convert_image(source, dest, in_format, out_format, run_as_root): |
| cmd = ('qemu-img', 'convert', '-O', out_format, source, dest) |
| + if in_format is not None: |
| + cmd = cmd + ('-f', in_format) |
| utils.execute(*cmd, run_as_root=run_as_root) |
| |
| |
| @@ -123,7 +145,7 @@ def fetch_to_raw(context, image_href, path, user_id, project_id, max_size=0): |
| staged = "%s.converted" % path |
| LOG.debug("%s was %s, converting to raw" % (image_href, fmt)) |
| with fileutils.remove_path_on_error(staged): |
| - convert_image(path_tmp, staged, 'raw') |
| + convert_image(path_tmp, staged, fmt, 'raw') |
| os.unlink(path_tmp) |
| |
| data = qemu_img_info(staged) |
| diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py |
| index 5e14f61..151ebc4 100644 |
| --- a/nova/virt/libvirt/imagebackend.py |
| +++ b/nova/virt/libvirt/imagebackend.py |
| @@ -477,7 +477,7 @@ class Raw(Image): |
| self.correct_format() |
| |
| def snapshot_extract(self, target, out_format): |
| - images.convert_image(self.path, target, out_format) |
| + images.convert_image(self.path, target, self.driver_format, out_format) |
| |
| @staticmethod |
| def is_file_in_instance_path(): |
| @@ -631,7 +631,16 @@ class Lvm(Image): |
| size, sparse=self.sparse) |
| if self.ephemeral_key_uuid is not None: |
| encrypt_lvm_image() |
| - images.convert_image(base, self.path, 'raw', run_as_root=True) |
| + # NOTE: by calling convert_image_unsafe here we're |
| + # telling qemu-img convert to do format detection on the input, |
| + # because we don't know what the format is. For example, |
| + # we might have downloaded a qcow2 image, or created an |
| + # ephemeral filesystem locally, we just don't know here. Having |
| + # audited this, all current sources have been sanity checked, |
| + # either because they're locally generated, or because they have |
| + # come from images.fetch_to_raw. However, this is major code smell. |
| + images.convert_image_unsafe(base, self.path, self.driver_format, |
| + run_as_root=True) |
| if resize: |
| disk.resize2fs(self.path, run_as_root=True) |
| |
| @@ -678,8 +687,8 @@ class Lvm(Image): |
| lvm.remove_volumes([self.lv_path]) |
| |
| def snapshot_extract(self, target, out_format): |
| - images.convert_image(self.path, target, out_format, |
| - run_as_root=True) |
| + images.convert_image(self.path, target, self.driver_format, |
| + out_format, run_as_root=True) |
| |
| def get_model(self, connection): |
| return imgmodel.LocalBlockImage(self.path) |
| @@ -786,7 +795,7 @@ class Rbd(Image): |
| self.driver.resize(self.rbd_name, size) |
| |
| def snapshot_extract(self, target, out_format): |
| - images.convert_image(self.path, target, out_format) |
| + images.convert_image(self.path, target, 'raw', out_format) |
| |
| @staticmethod |
| def is_shared_block_storage(): |
| -- |
| 2.5.0 |
| |