diff --git a/glance/async/flows/base_import.py b/glance/async/flows/base_import.py index 0993a47dbb..ff3a8fd642 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 @@ -154,6 +155,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 0bb9e26f47..ceaa4116b1 100644 --- a/glance/async/flows/convert.py +++ b/glance/async/flows/convert.py @@ -69,12 +69,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 3be75cda14..f702a2bd9d 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 14ae5c9fbb..6a5054ecd8 100644 --- a/glance/async/utils.py +++ b/glance/async/utils.py @@ -13,8 +13,10 @@ # 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 encodeutils +from oslo_utils import units from taskflow import task from glance.i18n import _LW @@ -22,6 +24,14 @@ from glance.i18n import _LW LOG = logging.getLogger(__name__) +# 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 6400dd8b51..b3c3848b2a 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 47343dffa7..bbc708d22a 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, @@ -393,6 +407,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..fa38f923ad --- /dev/null +++ b/releasenotes/notes/add-processlimits-to-qemu-img-c215f5d90f741d8a.yaml @@ -0,0 +1,12 @@ +--- +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, + which by default (since the Liberty release) are only + available to admin users. We continue to recommend that + tasks only be exposed to trusted users