Adding constraints around qemu-img calls

* All "qemu-img info" calls are now run under resource limitations that
  limit CPU time to 2 seconds and address space usage to 1 GB. This
  helps avoid any DoS attacks via malicious images.
* All "qemu-img convert" calls now specify the import format so that it
  does not have to be inferred by qemu-img.

SecurityImpact

(Hemanth did all the work on this, I'm just doing the backport.)

Co-authored-by: Hemanth Makkapati <hemanth.makkapati@rackspace.com>
Closes-Bug: #1449062
(cherry picked from commit 69a9b659fd)

Change-Id: I65f30b85439a8811545b0ca590555528631954df
This commit is contained in:
Brian Rosmaita 2016-09-27 16:11:17 -04:00
parent 1f93da1a33
commit 58311904a7
8 changed files with 80 additions and 2 deletions

View File

@ -30,6 +30,7 @@ from taskflow import retry
from taskflow import task from taskflow import task
from taskflow.types import failure from taskflow.types import failure
from glance.async import utils
from glance.common import exception from glance.common import exception
from glance.common.scripts.image_import import main as image_import from glance.common.scripts.image_import import main as image_import
from glance.common.scripts import utils as script_utils 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. # place that other tasks can consume as well.
stdout, stderr = putils.trycmd('qemu-img', 'info', stdout, stderr = putils.trycmd('qemu-img', 'info',
'--output=json', path, '--output=json', path,
prlimit=utils.QEMU_IMG_PROC_LIMITS,
log_errors=putils.LOG_ALL_ERRORS) log_errors=putils.LOG_ALL_ERRORS)
except OSError as exc: except OSError as exc:
with excutils.save_and_reraise_exception(): with excutils.save_and_reraise_exception():

View File

@ -73,12 +73,23 @@ class _Convert(task.Task):
_Convert.conversion_missing_warned = True _Convert.conversion_missing_warned = True
return 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 # TODO(flaper87): Check whether the image is in the desired
# format already. Probably using `qemu-img` just like the # format already. Probably using `qemu-img` just like the
# `Introspection` task. # `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) dest_path = os.path.join(CONF.task.work_dir, "%s.converted" % image_id)
stdout, stderr = putils.trycmd('qemu-img', 'convert', '-O', stdout, stderr = putils.trycmd('qemu-img', 'convert',
conversion_format, file_path, dest_path, '-f', src_format,
'-O', conversion_format,
file_path, dest_path,
log_errors=putils.LOG_ALL_ERRORS) log_errors=putils.LOG_ALL_ERRORS)
if stderr: if stderr:

View File

@ -48,6 +48,7 @@ class _Introspect(utils.OptionalTask):
try: try:
stdout, stderr = putils.trycmd('qemu-img', 'info', stdout, stderr = putils.trycmd('qemu-img', 'info',
'--output=json', file_path, '--output=json', file_path,
prlimit=utils.QEMU_IMG_PROC_LIMITS,
log_errors=putils.LOG_ALL_ERRORS) log_errors=putils.LOG_ALL_ERRORS)
except OSError as exc: except OSError as exc:
# NOTE(flaper87): errno == 2 means the executable file # NOTE(flaper87): errno == 2 means the executable file

View File

@ -13,7 +13,9 @@
# License for the specific language governing permissions and limitations # License for the specific language governing permissions and limitations
# under the License. # under the License.
from oslo_concurrency import processutils as putils
from oslo_log import log as logging from oslo_log import log as logging
from oslo_utils import units
from taskflow import task from taskflow import task
from glance import i18n from glance import i18n
@ -22,6 +24,14 @@ from glance import i18n
LOG = logging.getLogger(__name__) LOG = logging.getLogger(__name__)
_LW = i18n._LW _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): class OptionalTask(task.Task):

View File

@ -94,6 +94,12 @@ class TestImportTask(test_utils.BaseTestCase):
rm_mock.return_value = None rm_mock.return_value = None
image_convert.execute(image, 'file:///test/path.raw') 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): def test_convert_revert_success(self):
image_convert = convert._Convert(self.task.task_id, image_convert = convert._Convert(self.task.task_id,
self.task_type, self.task_type,
@ -178,3 +184,15 @@ class TestImportTask(test_utils.BaseTestCase):
self.assertEqual([], os.listdir(self.work_dir)) self.assertEqual([], os.listdir(self.work_dir))
self.assertEqual('qcow2', image.disk_format) self.assertEqual('qcow2', image.disk_format)
self.assertEqual(10737418240, image.virtual_size) 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)

View File

@ -27,6 +27,7 @@ from taskflow.types import failure
import glance.async.flows.base_import as import_flow import glance.async.flows.base_import as import_flow
from glance.async import taskflow_executor 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.image_import import main as image_import
from glance.common.scripts import utils as script_utils from glance.common.scripts import utils as script_utils
from glance.common import utils from glance.common import utils
@ -86,6 +87,14 @@ class TestImportTask(test_utils.BaseTestCase):
task_time_to_live=task_ttl, task_time_to_live=task_ttl,
task_input=task_input) 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): def test_import_flow(self):
self.config(engine_mode='serial', self.config(engine_mode='serial',
group='taskflow_executor') group='taskflow_executor')
@ -127,6 +136,8 @@ class TestImportTask(test_utils.BaseTestCase):
self.image.image_id), self.image.image_id),
self.image.locations[0]['url']) self.image.locations[0]['url'])
self._assert_qemu_process_limits(tmock)
def test_import_flow_missing_work_dir(self): def test_import_flow_missing_work_dir(self):
self.config(engine_mode='serial', group='taskflow_executor') self.config(engine_mode='serial', group='taskflow_executor')
self.config(work_dir=None, group='task') self.config(work_dir=None, group='task')
@ -235,6 +246,7 @@ class TestImportTask(test_utils.BaseTestCase):
self.assertTrue(rmock.called) self.assertTrue(rmock.called)
self.assertIsInstance(rmock.call_args[1]['result'], self.assertIsInstance(rmock.call_args[1]['result'],
failure.Failure) failure.Failure)
self._assert_qemu_process_limits(tmock)
image_path = os.path.join(self.test_dir, image_path = os.path.join(self.test_dir,
self.image.image_id) self.image.image_id)
@ -283,6 +295,8 @@ class TestImportTask(test_utils.BaseTestCase):
executor.begin_processing, executor.begin_processing,
self.task.task_id) self.task.task_id)
self._assert_qemu_process_limits(tmock)
image_path = os.path.join(self.test_dir, image_path = os.path.join(self.test_dir,
self.image.image_id) self.image.image_id)
tmp_image_path = os.path.join(self.work_dir, 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) image_path = os.path.join(self.work_dir, image_id)
tmp_image_path = os.path.join(self.work_dir, image_path) tmp_image_path = os.path.join(self.work_dir, image_path)
self.assertTrue(os.path.exists(tmp_image_path)) self.assertTrue(os.path.exists(tmp_image_path))
self._assert_qemu_process_limits(tmock)
def test_delete_from_fs(self): def test_delete_from_fs(self):
delete_fs = import_flow._DeleteFromFS(self.task.task_id, delete_fs = import_flow._DeleteFromFS(self.task.task_id,

View File

@ -21,6 +21,7 @@ from oslo_concurrency import processutils
from oslo_config import cfg from oslo_config import cfg
from glance.async.flows import introspect from glance.async.flows import introspect
from glance.async import utils as async_utils
from glance import domain from glance import domain
import glance.tests.utils as test_utils import glance.tests.utils as test_utils
@ -89,6 +90,13 @@ class TestImportTask(test_utils.BaseTestCase):
image_create.execute(image, '/test/path.qcow2') image_create.execute(image, '/test/path.qcow2')
self.assertEqual(10737418240, image.virtual_size) 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): def test_introspect_no_image(self):
image_create = introspect._Introspect(self.task.task_id, image_create = introspect._Introspect(self.task.task_id,
self.task_type, self.task_type,

View File

@ -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.