diff --git a/glance/async/flows/base_import.py b/glance/async/flows/base_import.py index 26a09e58c6..fb0c04502a 100644 --- a/glance/async/flows/base_import.py +++ b/glance/async/flows/base_import.py @@ -30,6 +30,7 @@ from taskflow import retry from taskflow import task from taskflow.types import failure +from glance.async import utils from glance.common import exception from glance.common.scripts.image_import import main as image_import from glance.common.scripts import utils as script_utils @@ -156,6 +157,7 @@ class _ImportToFS(task.Task): # place that other tasks can consume as well. stdout, stderr = putils.trycmd('qemu-img', 'info', '--output=json', path, + prlimit=utils.QEMU_IMG_PROC_LIMITS, log_errors=putils.LOG_ALL_ERRORS) except OSError as exc: with excutils.save_and_reraise_exception(): diff --git a/glance/async/flows/convert.py b/glance/async/flows/convert.py index 8893533049..6512fc25a6 100644 --- a/glance/async/flows/convert.py +++ b/glance/async/flows/convert.py @@ -73,12 +73,23 @@ class _Convert(task.Task): _Convert.conversion_missing_warned = True return + image_obj = self.image_repo.get(image_id) + src_format = image_obj.disk_format + # TODO(flaper87): Check whether the image is in the desired # format already. Probably using `qemu-img` just like the # `Introspection` task. + + # NOTE(hemanthm): We add '-f' parameter to the convert command here so + # that the image format need not be inferred by qemu utils. This + # shields us from being vulnerable to an attack vector described here + # https://bugs.launchpad.net/glance/+bug/1449062 + dest_path = os.path.join(CONF.task.work_dir, "%s.converted" % image_id) - stdout, stderr = putils.trycmd('qemu-img', 'convert', '-O', - conversion_format, file_path, dest_path, + stdout, stderr = putils.trycmd('qemu-img', 'convert', + '-f', src_format, + '-O', conversion_format, + file_path, dest_path, log_errors=putils.LOG_ALL_ERRORS) if stderr: diff --git a/glance/async/flows/introspect.py b/glance/async/flows/introspect.py index 8784d4f302..af1be3b7ea 100644 --- a/glance/async/flows/introspect.py +++ b/glance/async/flows/introspect.py @@ -48,6 +48,7 @@ class _Introspect(utils.OptionalTask): try: stdout, stderr = putils.trycmd('qemu-img', 'info', '--output=json', file_path, + prlimit=utils.QEMU_IMG_PROC_LIMITS, log_errors=putils.LOG_ALL_ERRORS) except OSError as exc: # NOTE(flaper87): errno == 2 means the executable file diff --git a/glance/async/utils.py b/glance/async/utils.py index c411975a25..3a515dfe86 100644 --- a/glance/async/utils.py +++ b/glance/async/utils.py @@ -13,7 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_concurrency import processutils as putils from oslo_log import log as logging +from oslo_utils import units from taskflow import task from glance import i18n @@ -22,6 +24,14 @@ from glance import i18n LOG = logging.getLogger(__name__) _LW = i18n._LW +# NOTE(hemanthm): As reported in the bug #1449062, "qemu-img info" calls can +# be exploited to craft DoS attacks by providing malicious input. The process +# limits defined here are protections against such attacks. This essentially +# limits the CPU time and address space used by the process that executes +# "qemu-img info" command to 2 seconds and 1 GB respectively. +QEMU_IMG_PROC_LIMITS = putils.ProcessLimits(cpu_time=2, + address_space=1 * units.Gi) + class OptionalTask(task.Task): diff --git a/glance/tests/unit/async/flows/test_convert.py b/glance/tests/unit/async/flows/test_convert.py index 4e46b70c7e..a3246ce03b 100644 --- a/glance/tests/unit/async/flows/test_convert.py +++ b/glance/tests/unit/async/flows/test_convert.py @@ -94,6 +94,12 @@ class TestImportTask(test_utils.BaseTestCase): rm_mock.return_value = None image_convert.execute(image, 'file:///test/path.raw') + # NOTE(hemanthm): Asserting that the source format is passed + # to qemu-utis to avoid inferring the image format. This + # shields us from an attack vector described at + # https://bugs.launchpad.net/glance/+bug/1449062/comments/72 + self.assertIn('-f', exc_mock.call_args[0]) + def test_convert_revert_success(self): image_convert = convert._Convert(self.task.task_id, self.task_type, @@ -178,3 +184,15 @@ class TestImportTask(test_utils.BaseTestCase): self.assertEqual([], os.listdir(self.work_dir)) self.assertEqual('qcow2', image.disk_format) self.assertEqual(10737418240, image.virtual_size) + + # NOTE(hemanthm): Asserting that the source format is passed + # to qemu-utis to avoid inferring the image format when + # converting. This shields us from an attack vector described + # at https://bugs.launchpad.net/glance/+bug/1449062/comments/72 + # + # A total of three calls will be made to 'execute': 'info', + # 'convert' and 'info' towards introspection, conversion and + # OVF packaging respectively. We care about the 'convert' call + # here, hence we fetch the 2nd set of args from the args list. + convert_call_args, _ = exc_mock.call_args_list[1] + self.assertIn('-f', convert_call_args) diff --git a/glance/tests/unit/async/flows/test_import.py b/glance/tests/unit/async/flows/test_import.py index 7221e853d2..1c8d6bcc08 100644 --- a/glance/tests/unit/async/flows/test_import.py +++ b/glance/tests/unit/async/flows/test_import.py @@ -27,6 +27,7 @@ from taskflow.types import failure import glance.async.flows.base_import as import_flow from glance.async import taskflow_executor +from glance.async import utils as async_utils from glance.common.scripts.image_import import main as image_import from glance.common.scripts import utils as script_utils from glance.common import utils @@ -86,6 +87,14 @@ class TestImportTask(test_utils.BaseTestCase): task_time_to_live=task_ttl, task_input=task_input) + def _assert_qemu_process_limits(self, exec_mock): + # NOTE(hemanthm): Assert that process limits are being applied + # on "qemu-img info" calls. See bug #1449062 for more details. + kw_args = exec_mock.call_args[1] + self.assertIn('prlimit', kw_args) + self.assertEqual(async_utils.QEMU_IMG_PROC_LIMITS, + kw_args.get('prlimit')) + def test_import_flow(self): self.config(engine_mode='serial', group='taskflow_executor') @@ -127,6 +136,8 @@ class TestImportTask(test_utils.BaseTestCase): self.image.image_id), self.image.locations[0]['url']) + self._assert_qemu_process_limits(tmock) + def test_import_flow_missing_work_dir(self): self.config(engine_mode='serial', group='taskflow_executor') self.config(work_dir=None, group='task') @@ -235,6 +246,7 @@ class TestImportTask(test_utils.BaseTestCase): self.assertTrue(rmock.called) self.assertIsInstance(rmock.call_args[1]['result'], failure.Failure) + self._assert_qemu_process_limits(tmock) image_path = os.path.join(self.test_dir, self.image.image_id) @@ -283,6 +295,8 @@ class TestImportTask(test_utils.BaseTestCase): executor.begin_processing, self.task.task_id) + self._assert_qemu_process_limits(tmock) + image_path = os.path.join(self.test_dir, self.image.image_id) tmp_image_path = os.path.join(self.work_dir, @@ -392,6 +406,7 @@ class TestImportTask(test_utils.BaseTestCase): image_path = os.path.join(self.work_dir, image_id) tmp_image_path = os.path.join(self.work_dir, image_path) self.assertTrue(os.path.exists(tmp_image_path)) + self._assert_qemu_process_limits(tmock) def test_delete_from_fs(self): delete_fs = import_flow._DeleteFromFS(self.task.task_id, diff --git a/glance/tests/unit/async/flows/test_introspect.py b/glance/tests/unit/async/flows/test_introspect.py index 7df66c40e9..869f99de4f 100644 --- a/glance/tests/unit/async/flows/test_introspect.py +++ b/glance/tests/unit/async/flows/test_introspect.py @@ -21,6 +21,7 @@ from oslo_concurrency import processutils from oslo_config import cfg from glance.async.flows import introspect +from glance.async import utils as async_utils from glance import domain import glance.tests.utils as test_utils @@ -89,6 +90,13 @@ class TestImportTask(test_utils.BaseTestCase): image_create.execute(image, '/test/path.qcow2') self.assertEqual(10737418240, image.virtual_size) + # NOTE(hemanthm): Assert that process limits are being applied on + # "qemu-img info" calls. See bug #1449062 for more details. + kw_args = exc_mock.call_args[1] + self.assertIn('prlimit', kw_args) + self.assertEqual(async_utils.QEMU_IMG_PROC_LIMITS, + kw_args.get('prlimit')) + def test_introspect_no_image(self): image_create = introspect._Introspect(self.task.task_id, self.task_type, diff --git a/releasenotes/notes/add-processlimits-to-qemu-img-c215f5d90f741d8a.yaml b/releasenotes/notes/add-processlimits-to-qemu-img-c215f5d90f741d8a.yaml new file mode 100644 index 0000000000..5c471d43dd --- /dev/null +++ b/releasenotes/notes/add-processlimits-to-qemu-img-c215f5d90f741d8a.yaml @@ -0,0 +1,13 @@ +--- +security: + - All ``qemu-img info`` calls are now run under resource + limitations that limit the CPU time and address space + usage of the process running the command to 2 seconds + and 1 GB respectively. This addresses the bug + https://bugs.launchpad.net/glance/+bug/1449062 + + Current usage of "qemu-img" is limited to Glance tasks. + In the Mitaka release, tasks by default will only be + available to admin users. In general, we recommend that + tasks only be exposed to trusted users, even in releases + prior to Mitaka.