libvirt: Block swap volume attempts with encrypted volumes prior to Queens

Prior to Queens any attempt to swap between encrypted volumes would
result in unencrypted data being written to the new volume. This
unencrypted data would then be overwritten the next time the volume was
attached to an instance as Nova no longer identified the volume as
encrypted, resulting in the volume being reformatted.

This stable only change uses limited parts of the following changes to
block all swap_volume attempts with encrypted volumes prior to Queens
where this was resolved by Ica323b87fa85a454fca9d46ada3677f18 and also
blocked when using QEMU to decrypt LUKS volumes by
Ibfa64f18bbd2fb70db7791330ed1a64fe61c1.

Ica323b87fa85a454fca9d46ada3677f18fe50022

The request context is provided to swap_volume in order to look up the
encryption metadata of a volume.

Ibfa64f18bbd2fb70db7791330ed1a64fe61c1355

Attempts to swap from an encrypted volume are blocked with a
NotImplementedError exception raised.

I258127fdcd011ccec721d5ff62eb7f128f130336

Attempts to swap from an unencrypted volume to an encrypted volume are
also blocked with a NotImplementedError exception raised.

Ie02d298cd92d5b5ebcbbcd2b0e8be01f197bfafb

The serial of a volume is used as the id if connection_info for the
volume doesn't contain the volume_id key. Required to avoid bug #1746609.

Conflicts:
        nova/tests/unit/compute/test_compute_mgr.py
        nova/tests/unit/virt/libvirt/test_driver.py

NOTE(lyarwood): Conflict due to cinderv3 support for swap_volume not
being present in stable/ocata via
I4b8bd01f1ffe2640fe7313213bf853d2e1bef9dd.

Closes-bug: #1739593
Change-Id: If12e7860baad2899380f06144a0270784a5466b8
(cherry picked from commit 5b64a1936122eeb35f37a09f9d38159e1a224c58)

10 files changed, 106 insertions, 14 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 2e83f07ae6..66c50e330a 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -5015,8 +5015,8 @@ class ComputeManager(manager.Manager):
                       "old: %(old_cinfo)s",
                       {'new_cinfo': new_cinfo, 'old_cinfo': old_cinfo},
                       instance=instance)
-            self.driver.swap_volume(old_cinfo, new_cinfo, instance, mountpoint,
-                                    resize_to)
+            self.driver.swap_volume(context, old_cinfo, new_cinfo, instance,
+                                    mountpoint, resize_to)
             LOG.debug("swap_volume: Driver volume swap returned, new "
                       "connection_info is now : %(new_cinfo)s",
                       {'new_cinfo': new_cinfo})
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index 6fab2d24ef..bbde4babdf 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -1890,8 +1890,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
         self.assertTrue(uuidutils.is_uuid_like(volume))
         return {}
 
-    def _assert_swap_volume(self, old_connection_info, new_connection_info,
-                            instance, mountpoint, resize_to):
+    def _assert_swap_volume(self, context, old_connection_info,
+                            new_connection_info, instance, mountpoint,
+                            resize_to):
         self.assertEqual(2, resize_to)
 
     @mock.patch.object(cinder.API, 'initialize_connection')
diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py
index db1a692801..0e5f66a082 100755
--- a/nova/tests/unit/virt/libvirt/test_driver.py
+++ b/nova/tests/unit/virt/libvirt/test_driver.py
@@ -14928,6 +14928,26 @@ class LibvirtConnTestCase(test.NoDBTestCase):
         self.assertTrue(instance.cleaned)
         save.assert_called_once_with()
 
+    @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption')
+    def test_swap_volume_native_luks_blocked(self, mock_get_encryption):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+
+        # dest volume is encrypted
+        mock_get_encryption.side_effect = [{}, {'provider': 'luks'}]
+        self.assertRaises(NotImplementedError, drvr.swap_volume, self.context,
+                            {}, {}, None, None, None)
+
+        # src volume is encrypted
+        mock_get_encryption.side_effect = [{'provider': 'luks'}, {}]
+        self.assertRaises(NotImplementedError, drvr.swap_volume, self.context,
+                            {}, {}, None, None, None)
+
+        # both volumes are encrypted
+        mock_get_encryption.side_effect = [{'provider': 'luks'},
+                                           {'provider': 'luks'}]
+        self.assertRaises(NotImplementedError, drvr.swap_volume, self.context,
+                            {}, {}, None, None, None)
+
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete',
                 return_value=True)
     def _test_swap_volume(self, mock_is_job_complete, source_type,
@@ -15104,8 +15124,8 @@ class LibvirtConnTestCase(test.NoDBTestCase):
         conf = mock.MagicMock(source_path='/fake-new-volume')
         get_volume_config.return_value = conf
 
-        conn.swap_volume(old_connection_info, new_connection_info, instance,
-                         '/dev/vdb', 1)
+        conn.swap_volume(self.context, old_connection_info,
+                         new_connection_info, instance, '/dev/vdb', 1)
 
         get_guest.assert_called_once_with(instance)
         connect_volume.assert_called_once_with(new_connection_info, disk_info)
@@ -15122,6 +15142,7 @@ class LibvirtConnTestCase(test.NoDBTestCase):
     def test_swap_volume_driver_source_is_snapshot(self):
         self._test_swap_volume_driver(source_type='snapshot')
 
+    @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption')
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.rebase')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._connect_volume')
@@ -15131,7 +15152,8 @@ class LibvirtConnTestCase(test.NoDBTestCase):
     @mock.patch('nova.virt.libvirt.host.Host.write_instance_config')
     def test_swap_volume_disconnect_new_volume_on_rebase_error(self,
             write_config, get_guest, get_disk, get_volume_config,
-            connect_volume, disconnect_volume, rebase):
+            connect_volume, disconnect_volume, rebase,
+            get_volume_encryption):
         """Assert that disconnect_volume is called for the new volume if an
            error is encountered while rebasing
         """
@@ -15139,12 +15161,13 @@ class LibvirtConnTestCase(test.NoDBTestCase):
         instance = objects.Instance(**self.test_instance)
         guest = libvirt_guest.Guest(mock.MagicMock())
         get_guest.return_value = guest
+        get_volume_encryption.return_value = {}
         exc = fakelibvirt.make_libvirtError(fakelibvirt.libvirtError,
               'internal error', error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR)
         rebase.side_effect = exc
 
         self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume,
-                          mock.sentinel.old_connection_info,
+                          self.context, mock.sentinel.old_connection_info,
                           mock.sentinel.new_connection_info,
                           instance, '/dev/vdb', 0)
         connect_volume.assert_called_once_with(
@@ -15153,6 +15176,7 @@ class LibvirtConnTestCase(test.NoDBTestCase):
         disconnect_volume.assert_called_once_with(
                 mock.sentinel.new_connection_info, 'vdb')
 
+    @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption')
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete')
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.abort_job')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
@@ -15163,7 +15187,8 @@ class LibvirtConnTestCase(test.NoDBTestCase):
     @mock.patch('nova.virt.libvirt.host.Host.write_instance_config')
     def test_swap_volume_disconnect_new_volume_on_pivot_error(self,
             write_config, get_guest, get_disk, get_volume_config,
-            connect_volume, disconnect_volume, abort_job, is_job_complete):
+            connect_volume, disconnect_volume, abort_job, is_job_complete,
+            get_volume_encryption):
         """Assert that disconnect_volume is called for the new volume if an
            error is encountered while pivoting to the new volume
         """
@@ -15171,13 +15196,14 @@ class LibvirtConnTestCase(test.NoDBTestCase):
         instance = objects.Instance(**self.test_instance)
         guest = libvirt_guest.Guest(mock.MagicMock())
         get_guest.return_value = guest
+        get_volume_encryption.return_value = {}
         exc = fakelibvirt.make_libvirtError(fakelibvirt.libvirtError,
               'internal error', error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR)
         is_job_complete.return_value = True
         abort_job.side_effect = [None, exc]
 
         self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume,
-                          mock.sentinel.old_connection_info,
+                          self.context, mock.sentinel.old_connection_info,
                           mock.sentinel.new_connection_info,
                           instance, '/dev/vdb', 0)
         connect_volume.assert_called_once_with(
diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py
index e576fcd79d..7bacb2df68 100644
--- a/nova/tests/unit/virt/test_block_device.py
+++ b/nova/tests/unit/virt/test_block_device.py
@@ -1085,3 +1085,28 @@ class TestDriverBlockDevice(test.NoDBTestCase):
         # can't assert_not_called if the method isn't in the spec.
         self.assertFalse(hasattr(test_eph, 'refresh_connection_info'))
         self.assertFalse(hasattr(test_swap, 'refresh_connection_info'))
+
+
+class TestGetVolumeId(test.NoDBTestCase):
+
+    def test_get_volume_id_none_found(self):
+        self.assertIsNone(driver_block_device.get_volume_id(None))
+        self.assertIsNone(driver_block_device.get_volume_id({}))
+        self.assertIsNone(driver_block_device.get_volume_id({'data': {}}))
+
+    def test_get_volume_id_found_volume_id_no_serial(self):
+        self.assertEqual(uuids.volume_id,
+                         driver_block_device.get_volume_id(
+                             {'data': {'volume_id': uuids.volume_id}}))
+
+    def test_get_volume_id_found_no_volume_id_serial(self):
+        self.assertEqual(uuids.serial,
+                         driver_block_device.get_volume_id(
+                             {'serial': uuids.serial}))
+
+    def test_get_volume_id_found_both(self):
+        # volume_id is taken over serial
+        self.assertEqual(uuids.volume_id,
+                         driver_block_device.get_volume_id(
+                             {'serial': uuids.serial,
+                              'data': {'volume_id': uuids.volume_id}}))
diff --git a/nova/tests/unit/virt/test_virt_drivers.py b/nova/tests/unit/virt/test_virt_drivers.py
index bc23e19b1a..b2bb38b8a2 100644
--- a/nova/tests/unit/virt/test_virt_drivers.py
+++ b/nova/tests/unit/virt/test_virt_drivers.py
@@ -488,7 +488,7 @@ class _VirtDriverTestCase(_FakeDriverBackendTestCase):
                                           instance_ref,
                                           '/dev/sda'))
         self.assertIsNone(
-            self.connection.swap_volume({'driver_volume_type': 'fake',
+            self.connection.swap_volume(None, {'driver_volume_type': 'fake',
                                          'data': {}},
                                         {'driver_volume_type': 'fake',
                                          'data': {}},
diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py
index 1da9d09912..edcf5c5496 100644
--- a/nova/virt/block_device.py
+++ b/nova/virt/block_device.py
@@ -570,3 +570,13 @@ def is_block_device_mapping(bdm):
     return (bdm.source_type in ('image', 'volume', 'snapshot', 'blank')
             and bdm.destination_type == 'volume'
             and is_implemented(bdm))
+
+
+def get_volume_id(connection_info):
+    if connection_info:
+        # Check for volume_id in 'data' and if not there, fallback to
+        # the 'serial' that the DriverVolumeBlockDevice adds during attach.
+        volume_id = connection_info.get('data', {}).get('volume_id')
+        if not volume_id:
+            volume_id = connection_info.get('serial')
+        return volume_id
diff --git a/nova/virt/driver.py b/nova/virt/driver.py
index fc15519c4e..6736296501 100644
--- a/nova/virt/driver.py
+++ b/nova/virt/driver.py
@@ -455,10 +455,11 @@ class ComputeDriver(object):
         """Detach the disk attached to the instance."""
         raise NotImplementedError()
 
-    def swap_volume(self, old_connection_info, new_connection_info,
+    def swap_volume(self, context, old_connection_info, new_connection_info,
                     instance, mountpoint, resize_to):
         """Replace the volume attached to the given `instance`.
 
+        :param context: The request context.
         :param dict old_connection_info:
             The volume for this connection gets detached from the given
             `instance`.
diff --git a/nova/virt/fake.py b/nova/virt/fake.py
index 1a4d5bec68..846c076b16 100644
--- a/nova/virt/fake.py
+++ b/nova/virt/fake.py
@@ -298,7 +298,7 @@ class FakeDriver(driver.ComputeDriver):
         except KeyError:
             pass
 
-    def swap_volume(self, old_connection_info, new_connection_info,
+    def swap_volume(self, context, old_connection_info, new_connection_info,
                     instance, mountpoint, resize_to):
         """Replace the disk attached to the instance."""
         instance_name = instance.name
diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py
index 2c56ea0269..07b669dd87 100644
--- a/nova/virt/libvirt/driver.py
+++ b/nova/virt/libvirt/driver.py
@@ -1194,6 +1194,16 @@ class LibvirtDriver(driver.ComputeDriver):
                                                     **encryption)
         return encryptor
 
+    def _get_volume_encryption(self, context, connection_info):
+        """Get the encryption metadata dict if it is not provided
+        """
+        encryption = {}
+        volume_id = driver_block_device.get_volume_id(connection_info)
+        if volume_id:
+            encryption = encryptors.get_encryption_metadata(context,
+                            self._volume_api, volume_id, connection_info)
+        return encryption
+
     def _check_discard_for_attach_volume(self, conf, instance):
         """Perform some checks for volumes configured for discard support.
 
@@ -1342,9 +1352,19 @@ class LibvirtDriver(driver.ComputeDriver):
         finally:
             self._host.write_instance_config(xml)
 
-    def swap_volume(self, old_connection_info,
+    def swap_volume(self, context, old_connection_info,
                     new_connection_info, instance, mountpoint, resize_to):
 
+        # NOTE(lyarwood): Bug #1739593 uncovered a nasty data corruption
+        # issue that was fixed in Queens by Ica323b87fa85a454fca9d46ada3677f18.
+        # Given the size of the bugfix it was agreed not to backport the change
+        # to earlier stable branches and to instead block swap volume attempts.
+        if (self._get_volume_encryption(context, old_connection_info) or
+            self._get_volume_encryption(context, new_connection_info)):
+            raise NotImplementedError(_("Swap volume is not supported when "
+                "using encrypted volumes. For more details see "
+                "https://bugs.launchpad.net/nova/+bug/1739593."))
+
         guest = self._host.get_guest(instance)
 
         disk_dev = mountpoint.rpartition("/")[2]
diff --git a/releasenotes/notes/bug-1739593-cve-2017-18191-25fe48d336d8cf13.yaml b/releasenotes/notes/bug-1739593-cve-2017-18191-25fe48d336d8cf13.yaml
new file mode 100644
index 0000000000..915b34053e
--- /dev/null
+++ b/releasenotes/notes/bug-1739593-cve-2017-18191-25fe48d336d8cf13.yaml
@@ -0,0 +1,9 @@
+---
+prelude: >
+    This release includes fixes for security vulnerabilities.
+security:
+  - |
+    [CVE-2017-18191] Swapping encrypted volumes can lead to data loss and a
+    possible compute host DOS attack.
+
+    * `Bug 1739593 <https://bugs.launchpad.net/nova/+bug/1739593>`_