From 6b739ffc337ad5491b027a4c2312bf3f461079c8 Mon Sep 17 00:00:00 2001 From: Lance Bragstad Date: Mon, 2 Nov 2020 17:22:49 +0000 Subject: [PATCH] Use app cred user ID in policy enforcement The application credential policies use the `rule:owner` policy to allow users to manage their own credentials. The policy engine pulled the user_id attribute from the request path instead of the actual application credential. This allowed for users to exploit the enforcement and view or delete application credentials they don't own. This commit attempts to resolve the issue by updating the flask parameters before they're translated to policy arguments and target data, prior to policy enforcement. Change-Id: I903d20fa41270499ca1c39d296120dd97cef5405 Closes-Bug: 1901207 (cherry picked from commit 2d7bf10a5a145ed3b6e34c3cb95e05fb7e33e0d1) (cherry picked from commit 80832de6ced534bccbf7cfe24a6a01c8262c1779) --- keystone/api/users.py | 47 ++++++++++++- .../v3/test_application_credential.py | 66 +++++++++++++++++++ .../notes/bug-1901207-13762f85b8a04481.yaml | 7 ++ 3 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/bug-1901207-13762f85b8a04481.yaml diff --git a/keystone/api/users.py b/keystone/api/users.py index f7f1ec0cfa..10f26bd428 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -121,6 +121,41 @@ def _build_enforcer_target_data_owner_and_user_id_match(): return ref +def _update_request_user_id_attribute(): + # This method handles a special case in policy enforcement. The application + # credential API is underneath the user path (e.g., + # /v3/users/{user_id}/application_credentials/{application_credential_id}). + # The RBAC enforcer thinks the user to evaluate for application credential + # ownership comes from the path, but it should come from the actual + # application credential reference. By ensuring we pull the user ID from + # the application credential, we close a loop hole where users could + # effectively bypass authorization to view or delete any application + # credential in the system, assuming the attacker knows the application + # credential ID of another user. So long as the attacker matches the user + # ID in the request path to the user in the token of the request, they can + # pass the `rule:owner` policy check. This method protects against that by + # ensuring we use the application credential user ID and not something + # determined from the client. + try: + app_cred = ( + PROVIDERS.application_credential_api.get_application_credential( + flask.request.view_args.get('application_credential_id') + ) + ) + flask.request.view_args['user_id'] = app_cred['user_id'] + + # This target isn't really used in the default policy for application + # credentials, but we return it since we're using this method as a hook + # to update the flask request variables, which are used later in the + # keystone RBAC enforcer to populate the policy_dict, which ultimately + # turns into target attributes. + return {'user_id': app_cred['user_id']} + except ks_exception.NotFound: # nosec + # Defer existance in the event the application credential doesn't + # exist, we'll check this later anyway. + pass + + def _format_role_entity(role_id): role = PROVIDERS.role_api.get_role(role_id) formatted_entity = role.copy() @@ -652,7 +687,11 @@ class UserAppCredGetDeleteResource(ks_flask.ResourceBase): GET/HEAD /v3/users/{user_id}/application_credentials/ {application_credential_id} """ - ENFORCER.enforce_call(action='identity:get_application_credential') + target = _update_request_user_id_attribute() + ENFORCER.enforce_call( + action='identity:get_application_credential', + target_attr=target, + ) ref = PROVIDERS.application_credential_api.get_application_credential( application_credential_id) return self.wrap_member(ref) @@ -663,7 +702,11 @@ class UserAppCredGetDeleteResource(ks_flask.ResourceBase): DELETE /v3/users/{user_id}/application_credentials/ {application_credential_id} """ - ENFORCER.enforce_call(action='identity:delete_application_credential') + target = _update_request_user_id_attribute() + ENFORCER.enforce_call( + action='identity:delete_application_credential', + target_attr=target + ) token = self.auth_context['token'] _check_unrestricted_application_credential(token) PROVIDERS.application_credential_api.delete_application_credential( diff --git a/keystone/tests/protection/v3/test_application_credential.py b/keystone/tests/protection/v3/test_application_credential.py index 5807d7f90d..5f7c2a202b 100644 --- a/keystone/tests/protection/v3/test_application_credential.py +++ b/keystone/tests/protection/v3/test_application_credential.py @@ -418,6 +418,72 @@ class OwnerTests(_TestAppCredBase, def test_owner_can_delete_application_credential(self): self._test_delete_application_credential() + def test_user_cannot_lookup_application_credential_for_another_user(self): + # create another user + another_user = unit.new_user_ref( + domain_id=CONF.identity.default_domain_id + ) + another_user_id = PROVIDERS.identity_api.create_user( + another_user + )['id'] + + auth = self.build_authentication_request( + user_id=another_user_id, + password=another_user['password'] + ) + + # authenticate for a token as a completely different user with + # completely different authorization + with self.test_client() as c: + r = c.post('/v3/auth/tokens', json=auth) + another_user_token = r.headers['X-Subject-Token'] + + # create an application credential as the self.user_id user on a + # project that the user above doesn't have any authorization on + app_cred = self._create_application_credential() + + # attempt to lookup the application credential as another user + with self.test_client() as c: + c.get( + '/v3/users/%s/application_credentials/%s' % ( + another_user_id, + app_cred['id']), + expected_status_code=http.client.FORBIDDEN, + headers={'X-Auth-Token': another_user_token}) + + def test_user_cannot_delete_application_credential_for_another_user(self): + # create another user + another_user = unit.new_user_ref( + domain_id=CONF.identity.default_domain_id + ) + another_user_id = PROVIDERS.identity_api.create_user( + another_user + )['id'] + + auth = self.build_authentication_request( + user_id=another_user_id, + password=another_user['password'] + ) + + # authenticate for a token as a completely different user with + # completely different authorization + with self.test_client() as c: + r = c.post('/v3/auth/tokens', json=auth) + another_user_token = r.headers['X-Subject-Token'] + + # create an application credential as the self.user_id user on a + # project that the user above doesn't have any authorization on + app_cred = self._create_application_credential() + + # attempt to delete the application credential as another user + with self.test_client() as c: + c.delete( + '/v3/users/%s/application_credentials/%s' % ( + another_user_id, + app_cred['id']), + expected_status_code=http.client.FORBIDDEN, + headers={'X-Auth-Token': another_user_token}) + class DomainAdminTests(_TestAppCredBase, common_auth.AuthTestMixin, diff --git a/releasenotes/notes/bug-1901207-13762f85b8a04481.yaml b/releasenotes/notes/bug-1901207-13762f85b8a04481.yaml new file mode 100644 index 0000000000..26e957a0d2 --- /dev/null +++ b/releasenotes/notes/bug-1901207-13762f85b8a04481.yaml @@ -0,0 +1,7 @@ +--- +security: + - | + [`bug 1901207 `_] + Policy enforcement for application credentials has been updated to protect + against invalid ownership checks resulting in unauthorized users being able + to get and delete application credentials for other users.