Browse Source

Replace non-nova server fault message

The server fault "message" is always shown in the API
server response, regardless of policy or user role.

The fault "details" are only shown to users with the
admin role when the fault code is 500.

The problem with this is for non-nova exceptions, the
fault message is a string-ified version of the exception
(see nova.compute.utils.exception_to_dict) which can
contain sensitive information which the non-admin owner
of the server can see.

This change adds a functional test to recreate the issue
and a change to exception_to_dict which for the non-nova
case changes the fault message by simply storing the
exception type class name. Admins can still see the fault
traceback in the "details" key of the fault dict in the
server API response. Note that _get_fault_details is
changed so that the details also includes the exception
value which is what used to be in the fault message for
non-nova exceptions. This is necessary so admins can still
get the exception message with the traceback details.

Note that nova exceptions with a %(reason)s replacement
variable could potentially be leaking sensitive details as
well but those would need to be cleaned up on a case-by-case
basis since we don't want to change the behavior of all
fault messages otherwise users might not see information
like NoValidHost when their server goes to ERROR status
during scheduling.

SecurityImpact: This change contains a fix for CVE-2019-14433.

NOTE(mriedem): The functional test imports change here
because Idaed39629095f86d24a54334c699a26c218c6593 is not
in Rocky so the PlacementFixture comes from nova_fixtures.

Change-Id: I5e0a43ec59341c9ac62f89105ddf82c4a014df81
Closes-Bug: #1837877
(cherry picked from commit 298b337a16)
(cherry picked from commit 6765188116)
tags/18.2.2
Matt Riedemann 1 month ago
parent
commit
e0b91a5b1e

+ 32
- 9
nova/compute/utils.py View File

@@ -54,7 +54,20 @@ LOG = log.getLogger(__name__)
54 54
 
55 55
 
56 56
 def exception_to_dict(fault, message=None):
57
-    """Converts exceptions to a dict for use in notifications."""
57
+    """Converts exceptions to a dict for use in notifications.
58
+
59
+    :param fault: Exception that occurred
60
+    :param message: Optional fault message, otherwise the message is derived
61
+        from the fault itself.
62
+    :returns: dict with the following items:
63
+
64
+        - exception: the fault itself
65
+        - message: one of (in priority order):
66
+                   - the provided message to this method
67
+                   - a formatted NovaException message
68
+                   - the fault class name
69
+        - code: integer code for the fault (defaults to 500)
70
+    """
58 71
     # TODO(johngarbutt) move to nova/exception.py to share with wrap_exception
59 72
 
60 73
     code = 500
@@ -69,11 +82,17 @@ def exception_to_dict(fault, message=None):
69 82
     # These exception handlers are broad so we don't fail to log the fault
70 83
     # just because there is an unexpected error retrieving the message
71 84
     except Exception:
72
-        try:
73
-            message = six.text_type(fault)
74
-        except Exception:
75
-            message = None
76
-    if not message:
85
+        # In this case either we have a NovaException which failed to format
86
+        # the message or we have a non-nova exception which could contain
87
+        # sensitive details. Since we're not sure, be safe and set the message
88
+        # to the exception class name. Note that we don't guard on
89
+        # context.is_admin here because the message is always shown in the API,
90
+        # even to non-admin users (e.g. NoValidHost) but only the traceback
91
+        # details are shown to users with the admin role. Checking for admin
92
+        # context here is also not helpful because admins can perform
93
+        # operations on a tenant user's server (migrations, reboot, etc) and
94
+        # service startup and periodic tasks could take actions on a server
95
+        # and those use an admin context.
77 96
         message = fault.__class__.__name__
78 97
     # NOTE(dripton) The message field in the database is limited to 255 chars.
79 98
     # MySQL silently truncates overly long messages, but PostgreSQL throws an
@@ -87,10 +106,14 @@ def exception_to_dict(fault, message=None):
87 106
 
88 107
 def _get_fault_details(exc_info, error_code):
89 108
     details = ''
109
+    # TODO(mriedem): Why do we only include the details if the code is 500?
110
+    # Though for non-nova exceptions the code will probably be 500.
90 111
     if exc_info and error_code == 500:
91
-        tb = exc_info[2]
92
-        if tb:
93
-            details = ''.join(traceback.format_tb(tb))
112
+        # We get the full exception details including the value since
113
+        # the fault message may not contain that information for non-nova
114
+        # exceptions (see exception_to_dict).
115
+        details = ''.join(traceback.format_exception(
116
+            exc_info[0], exc_info[1], exc_info[2]))
94 117
     return six.text_type(details)
95 118
 
96 119
 

+ 113
- 0
nova/tests/functional/test_server_faults.py View File

@@ -0,0 +1,113 @@
1
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
2
+# not use this file except in compliance with the License. You may obtain
3
+# a copy of the License at
4
+#
5
+#      http://www.apache.org/licenses/LICENSE-2.0
6
+#
7
+# Unless required by applicable law or agreed to in writing, software
8
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
9
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
10
+# License for the specific language governing permissions and limitations
11
+# under the License.
12
+
13
+import mock
14
+
15
+from nova import test
16
+from nova.tests import fixtures as nova_fixtures
17
+from nova.tests.functional import integrated_helpers
18
+from nova.tests.unit.image import fake as fake_image
19
+from nova.tests.unit import policy_fixture
20
+
21
+
22
+class HypervisorError(Exception):
23
+    """This is just used to make sure the exception type is in the fault."""
24
+    pass
25
+
26
+
27
+class ServerFaultTestCase(test.TestCase,
28
+                          integrated_helpers.InstanceHelperMixin):
29
+    """Tests for the server faults reporting from the API."""
30
+
31
+    def setUp(self):
32
+        super(ServerFaultTestCase, self).setUp()
33
+        # Setup the standard fixtures.
34
+        fake_image.stub_out_image_service(self)
35
+        self.addCleanup(fake_image.FakeImageService_reset)
36
+        self.useFixture(nova_fixtures.NeutronFixture(self))
37
+        self.useFixture(nova_fixtures.PlacementFixture())
38
+        self.useFixture(policy_fixture.RealPolicyFixture())
39
+
40
+        # Start the compute services.
41
+        self.start_service('conductor')
42
+        self.start_service('scheduler')
43
+        self.compute = self.start_service('compute')
44
+        api_fixture = self.useFixture(nova_fixtures.OSAPIFixture(
45
+            api_version='v2.1'))
46
+        self.api = api_fixture.api
47
+        self.admin_api = api_fixture.admin_api
48
+
49
+    def test_server_fault_non_nova_exception(self):
50
+        """Creates a server using the non-admin user, then reboots it which
51
+        will generate a non-NovaException fault and put the instance into
52
+        ERROR status. Then checks that fault details are only visible to the
53
+        admin user.
54
+        """
55
+        # Create the server with the non-admin user.
56
+        server = self._build_minimal_create_server_request(
57
+            self.api, 'test_server_fault_non_nova_exception',
58
+            image_uuid=fake_image.get_valid_image_id(),
59
+            networks=[{'port': nova_fixtures.NeutronFixture.port_1['id']}])
60
+        server = self.api.post_server({'server': server})
61
+        server = self._wait_for_state_change(self.admin_api, server, 'ACTIVE')
62
+
63
+        # Stop the server before rebooting it so that after the driver.reboot
64
+        # method raises an exception, the fake driver does not report the
65
+        # instance power state as running - that will make the compute manager
66
+        # set the instance vm_state to error.
67
+        self.api.post_server_action(server['id'], {'os-stop': None})
68
+        server = self._wait_for_state_change(self.admin_api, server, 'SHUTOFF')
69
+
70
+        # Stub out the compute driver reboot method to raise a non-nova
71
+        # exception to simulate some error from the underlying hypervisor
72
+        # which in this case we are going to say has sensitive content.
73
+        error_msg = 'sensitive info'
74
+        with mock.patch.object(
75
+                self.compute.manager.driver, 'reboot',
76
+                side_effect=HypervisorError(error_msg)) as mock_reboot:
77
+            reboot_request = {'reboot': {'type': 'HARD'}}
78
+            self.api.post_server_action(server['id'], reboot_request)
79
+            # In this case we wait for the status to change to ERROR using
80
+            # the non-admin user so we can assert the fault details. We also
81
+            # wait for the task_state to be None since the wrap_instance_fault
82
+            # decorator runs before the reverts_task_state decorator so we will
83
+            # be sure the fault is set on the server.
84
+            server = self._wait_for_server_parameter(
85
+                self.api, server, {'status': 'ERROR',
86
+                                   'OS-EXT-STS:task_state': None})
87
+            mock_reboot.assert_called_once()
88
+        # The server fault from the non-admin user API response should not
89
+        # have details in it.
90
+        self.assertIn('fault', server)
91
+        fault = server['fault']
92
+        self.assertNotIn('details', fault)
93
+        # And the sensitive details from the non-nova exception should not be
94
+        # in the message.
95
+        self.assertIn('message', fault)
96
+        self.assertNotIn(error_msg, fault['message'])
97
+        # The exception type class name should be in the message.
98
+        self.assertIn('HypervisorError', fault['message'])
99
+
100
+        # Get the server fault details for the admin user.
101
+        server = self.admin_api.get_server(server['id'])
102
+        fault = server['fault']
103
+        # The admin can see the fault details which includes the traceback.
104
+        self.assertIn('details', fault)
105
+        # The details also contain the exception message (which is not in the
106
+        # fault message).
107
+        self.assertIn(error_msg, fault['details'])
108
+        # Make sure the traceback is there by looking for part of it.
109
+        self.assertIn('in reboot_instance', fault['details'])
110
+        # The exception type class name should be in the message for the admin
111
+        # user as well since the fault handling code cannot distinguish who
112
+        # is going to see the message so it only sets class name.
113
+        self.assertIn('HypervisorError', fault['message'])

+ 15
- 7
nova/tests/unit/compute/test_compute.py View File

@@ -4518,7 +4518,10 @@ class ComputeTestCase(BaseTestCase,
4518 4518
         self.assertEqual('ERROR', msg.priority)
4519 4519
         payload = msg.payload
4520 4520
         message = payload['message']
4521
-        self.assertNotEqual(-1, message.find("i'm dying"))
4521
+        # The fault message does not contain the exception value, only the
4522
+        # class name.
4523
+        self.assertEqual(-1, message.find("i'm dying"))
4524
+        self.assertIn('TestingException', message)
4522 4525
 
4523 4526
     def test_terminate_usage_notification(self):
4524 4527
         # Ensure terminate_instance generates correct usage notification.
@@ -6954,11 +6957,12 @@ class ComputeTestCase(BaseTestCase,
6954 6957
 
6955 6958
         def fake_db_fault_create(ctxt, values):
6956 6959
             self.assertIn('raise NotImplementedError', values['details'])
6960
+            self.assertIn('test', values['details'])
6957 6961
             del values['details']
6958 6962
 
6959 6963
             expected = {
6960 6964
                 'code': 500,
6961
-                'message': 'test',
6965
+                'message': 'NotImplementedError',
6962 6966
                 'instance_uuid': instance['uuid'],
6963 6967
                 'host': self.compute.host
6964 6968
             }
@@ -6989,12 +6993,14 @@ class ComputeTestCase(BaseTestCase,
6989 6993
             global raised_exc
6990 6994
 
6991 6995
             self.assertIn('raise messaging.RemoteError', values['details'])
6996
+            self.assertIn('Remote error: test My Test Message\nNone.',
6997
+                          values['details'])
6992 6998
             del values['details']
6993 6999
 
6994 7000
             expected = {
6995 7001
                 'code': 500,
6996 7002
                 'instance_uuid': instance['uuid'],
6997
-                'message': 'Remote error: test My Test Message\nNone.',
7003
+                'message': 'RemoteError',
6998 7004
                 'host': self.compute.host
6999 7005
             }
7000 7006
             self.assertEqual(expected, values)
@@ -7049,7 +7055,7 @@ class ComputeTestCase(BaseTestCase,
7049 7055
         def fake_db_fault_create(ctxt, values):
7050 7056
             expected = {
7051 7057
                 'code': 500,
7052
-                'message': 'test',
7058
+                'message': 'NotImplementedError',
7053 7059
                 'details': '',
7054 7060
                 'instance_uuid': instance['uuid'],
7055 7061
                 'host': self.compute.host
@@ -7085,9 +7091,11 @@ class ComputeTestCase(BaseTestCase,
7085 7091
                       fake_db_fault_create)
7086 7092
 
7087 7093
         ctxt = context.get_admin_context()
7088
-        compute_utils.add_instance_fault_from_exc(ctxt,
7089
-                                                  instance,
7090
-                                                  NotImplementedError(message))
7094
+        # Use a NovaException because non-nova exceptions will just have the
7095
+        # class name recorded in the fault message which will not exercise our
7096
+        # length trim code.
7097
+        exc = exception.NovaException(message=message)
7098
+        compute_utils.add_instance_fault_from_exc(ctxt, instance, exc)
7091 7099
 
7092 7100
     def test_add_instance_fault_with_message(self):
7093 7101
         instance = self._create_fake_instance_obj()

+ 23
- 0
releasenotes/notes/bug-1837877-cve-fault-message-exposure-5360d794f4976b7c.yaml View File

@@ -0,0 +1,23 @@
1
+---
2
+security:
3
+  - |
4
+    `OSSA-2019-003`_: Nova Server Resource Faults Leak External Exception
5
+    Details (CVE-2019-14433)
6
+
7
+    This release contains a security fix for `bug 1837877`_ where users
8
+    without the admin role can be exposed to sensitive error details in
9
+    the server resource fault ``message``.
10
+
11
+    There is a behavior change where non-nova exceptions will only record
12
+    the exception class name in the fault ``message`` field which is exposed
13
+    to all users, regardless of the admin role.
14
+
15
+    The fault ``details``, which are only exposed to users with the admin role,
16
+    will continue to include the traceback and also include the exception
17
+    value which for non-nova exceptions is what used to be exposed in the
18
+    fault ``message`` field. Meaning, the information that admins could see
19
+    for server faults is still available, but the exception value may be in
20
+    ``details`` rather than ``message`` now.
21
+
22
+    .. _OSSA-2019-003: https://security.openstack.org/ossa/OSSA-2019-003.html
23
+    .. _bug 1837877: https://bugs.launchpad.net/nova/+bug/1837877

Loading…
Cancel
Save