diff --git a/nova/compute/utils.py b/nova/compute/utils.py index 74873d258a8e..98a1ae919424 100644 --- a/nova/compute/utils.py +++ b/nova/compute/utils.py @@ -59,7 +59,20 @@ LOG = log.getLogger(__name__) def exception_to_dict(fault, message=None): - """Converts exceptions to a dict for use in notifications.""" + """Converts exceptions to a dict for use in notifications. + + :param fault: Exception that occurred + :param message: Optional fault message, otherwise the message is derived + from the fault itself. + :returns: dict with the following items: + + - exception: the fault itself + - message: one of (in priority order): + - the provided message to this method + - a formatted NovaException message + - the fault class name + - code: integer code for the fault (defaults to 500) + """ # TODO(johngarbutt) move to nova/exception.py to share with wrap_exception code = 500 @@ -74,11 +87,17 @@ def exception_to_dict(fault, message=None): # These exception handlers are broad so we don't fail to log the fault # just because there is an unexpected error retrieving the message except Exception: - try: - message = six.text_type(fault) - except Exception: - message = None - if not message: + # In this case either we have a NovaException which failed to format + # the message or we have a non-nova exception which could contain + # sensitive details. Since we're not sure, be safe and set the message + # to the exception class name. Note that we don't guard on + # context.is_admin here because the message is always shown in the API, + # even to non-admin users (e.g. NoValidHost) but only the traceback + # details are shown to users with the admin role. Checking for admin + # context here is also not helpful because admins can perform + # operations on a tenant user's server (migrations, reboot, etc) and + # service startup and periodic tasks could take actions on a server + # and those use an admin context. message = fault.__class__.__name__ # NOTE(dripton) The message field in the database is limited to 255 chars. # MySQL silently truncates overly long messages, but PostgreSQL throws an @@ -92,10 +111,14 @@ def exception_to_dict(fault, message=None): def _get_fault_details(exc_info, error_code): details = '' + # TODO(mriedem): Why do we only include the details if the code is 500? + # Though for non-nova exceptions the code will probably be 500. if exc_info and error_code == 500: - tb = exc_info[2] - if tb: - details = ''.join(traceback.format_tb(tb)) + # We get the full exception details including the value since + # the fault message may not contain that information for non-nova + # exceptions (see exception_to_dict). + details = ''.join(traceback.format_exception( + exc_info[0], exc_info[1], exc_info[2])) return six.text_type(details) diff --git a/nova/tests/functional/test_server_faults.py b/nova/tests/functional/test_server_faults.py new file mode 100644 index 000000000000..67853ed4af7a --- /dev/null +++ b/nova/tests/functional/test_server_faults.py @@ -0,0 +1,114 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from nova import test +from nova.tests import fixtures as nova_fixtures +from nova.tests.functional import fixtures as func_fixtures +from nova.tests.functional import integrated_helpers +from nova.tests.unit.image import fake as fake_image +from nova.tests.unit import policy_fixture + + +class HypervisorError(Exception): + """This is just used to make sure the exception type is in the fault.""" + pass + + +class ServerFaultTestCase(test.TestCase, + integrated_helpers.InstanceHelperMixin): + """Tests for the server faults reporting from the API.""" + + def setUp(self): + super(ServerFaultTestCase, self).setUp() + # Setup the standard fixtures. + fake_image.stub_out_image_service(self) + self.addCleanup(fake_image.FakeImageService_reset) + self.useFixture(nova_fixtures.NeutronFixture(self)) + self.useFixture(func_fixtures.PlacementFixture()) + self.useFixture(policy_fixture.RealPolicyFixture()) + + # Start the compute services. + self.start_service('conductor') + self.start_service('scheduler') + self.compute = self.start_service('compute') + api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( + api_version='v2.1')) + self.api = api_fixture.api + self.admin_api = api_fixture.admin_api + + def test_server_fault_non_nova_exception(self): + """Creates a server using the non-admin user, then reboots it which + will generate a non-NovaException fault and put the instance into + ERROR status. Then checks that fault details are only visible to the + admin user. + """ + # Create the server with the non-admin user. + server = self._build_minimal_create_server_request( + self.api, 'test_server_fault_non_nova_exception', + image_uuid=fake_image.get_valid_image_id(), + networks=[{'port': nova_fixtures.NeutronFixture.port_1['id']}]) + server = self.api.post_server({'server': server}) + server = self._wait_for_state_change(self.admin_api, server, 'ACTIVE') + + # Stop the server before rebooting it so that after the driver.reboot + # method raises an exception, the fake driver does not report the + # instance power state as running - that will make the compute manager + # set the instance vm_state to error. + self.api.post_server_action(server['id'], {'os-stop': None}) + server = self._wait_for_state_change(self.admin_api, server, 'SHUTOFF') + + # Stub out the compute driver reboot method to raise a non-nova + # exception to simulate some error from the underlying hypervisor + # which in this case we are going to say has sensitive content. + error_msg = 'sensitive info' + with mock.patch.object( + self.compute.manager.driver, 'reboot', + side_effect=HypervisorError(error_msg)) as mock_reboot: + reboot_request = {'reboot': {'type': 'HARD'}} + self.api.post_server_action(server['id'], reboot_request) + # In this case we wait for the status to change to ERROR using + # the non-admin user so we can assert the fault details. We also + # wait for the task_state to be None since the wrap_instance_fault + # decorator runs before the reverts_task_state decorator so we will + # be sure the fault is set on the server. + server = self._wait_for_server_parameter( + self.api, server, {'status': 'ERROR', + 'OS-EXT-STS:task_state': None}) + mock_reboot.assert_called_once() + # The server fault from the non-admin user API response should not + # have details in it. + self.assertIn('fault', server) + fault = server['fault'] + self.assertNotIn('details', fault) + # And the sensitive details from the non-nova exception should not be + # in the message. + self.assertIn('message', fault) + self.assertNotIn(error_msg, fault['message']) + # The exception type class name should be in the message. + self.assertIn('HypervisorError', fault['message']) + + # Get the server fault details for the admin user. + server = self.admin_api.get_server(server['id']) + fault = server['fault'] + # The admin can see the fault details which includes the traceback. + self.assertIn('details', fault) + # The details also contain the exception message (which is not in the + # fault message). + self.assertIn(error_msg, fault['details']) + # Make sure the traceback is there by looking for part of it. + self.assertIn('in reboot_instance', fault['details']) + # The exception type class name should be in the message for the admin + # user as well since the fault handling code cannot distinguish who + # is going to see the message so it only sets class name. + self.assertIn('HypervisorError', fault['message']) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 29a80bf6d471..feb474dee4a9 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -4425,7 +4425,10 @@ class ComputeTestCase(BaseTestCase, self.assertEqual('ERROR', msg.priority) payload = msg.payload message = payload['message'] - self.assertNotEqual(-1, message.find("i'm dying")) + # The fault message does not contain the exception value, only the + # class name. + self.assertEqual(-1, message.find("i'm dying")) + self.assertIn('TestingException', message) def test_terminate_usage_notification(self): # Ensure terminate_instance generates correct usage notification. @@ -6840,11 +6843,12 @@ class ComputeTestCase(BaseTestCase, def fake_db_fault_create(ctxt, values): self.assertIn('raise NotImplementedError', values['details']) + self.assertIn('test', values['details']) del values['details'] expected = { 'code': 500, - 'message': 'test', + 'message': 'NotImplementedError', 'instance_uuid': instance['uuid'], 'host': self.compute.host } @@ -6875,12 +6879,14 @@ class ComputeTestCase(BaseTestCase, global raised_exc self.assertIn('raise messaging.RemoteError', values['details']) + self.assertIn('Remote error: test My Test Message\nNone.', + values['details']) del values['details'] expected = { 'code': 500, 'instance_uuid': instance['uuid'], - 'message': 'Remote error: test My Test Message\nNone.', + 'message': 'RemoteError', 'host': self.compute.host } self.assertEqual(expected, values) @@ -6935,7 +6941,7 @@ class ComputeTestCase(BaseTestCase, def fake_db_fault_create(ctxt, values): expected = { 'code': 500, - 'message': 'test', + 'message': 'NotImplementedError', 'details': '', 'instance_uuid': instance['uuid'], 'host': self.compute.host @@ -6971,9 +6977,11 @@ class ComputeTestCase(BaseTestCase, fake_db_fault_create) ctxt = context.get_admin_context() - compute_utils.add_instance_fault_from_exc(ctxt, - instance, - NotImplementedError(message)) + # Use a NovaException because non-nova exceptions will just have the + # class name recorded in the fault message which will not exercise our + # length trim code. + exc = exception.NovaException(message=message) + compute_utils.add_instance_fault_from_exc(ctxt, instance, exc) def test_add_instance_fault_with_message(self): instance = self._create_fake_instance_obj() diff --git a/releasenotes/notes/bug-1837877-cve-fault-message-exposure-5360d794f4976b7c.yaml b/releasenotes/notes/bug-1837877-cve-fault-message-exposure-5360d794f4976b7c.yaml new file mode 100644 index 000000000000..0ed0ea99c15a --- /dev/null +++ b/releasenotes/notes/bug-1837877-cve-fault-message-exposure-5360d794f4976b7c.yaml @@ -0,0 +1,23 @@ +--- +security: + - | + `OSSA-2019-003`_: Nova Server Resource Faults Leak External Exception + Details (CVE-2019-14433) + + This release contains a security fix for `bug 1837877`_ where users + without the admin role can be exposed to sensitive error details in + the server resource fault ``message``. + + There is a behavior change where non-nova exceptions will only record + the exception class name in the fault ``message`` field which is exposed + to all users, regardless of the admin role. + + The fault ``details``, which are only exposed to users with the admin role, + will continue to include the traceback and also include the exception + value which for non-nova exceptions is what used to be exposed in the + fault ``message`` field. Meaning, the information that admins could see + for server faults is still available, but the exception value may be in + ``details`` rather than ``message`` now. + + .. _OSSA-2019-003: https://security.openstack.org/ossa/OSSA-2019-003.html + .. _bug 1837877: https://bugs.launchpad.net/nova/+bug/1837877