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

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(

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,

releasenotes/notes/bug-1901207-13762f85b8a04481.yaml

@@ -0,0 +1,7 @@
+---
+security:
+  - |
+    [`bug 1901207 <https://bugs.launchpad.net/keystone/+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.