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.

This commit also deviates slightly from backports to stable/ussuri,
stable/victoria, and master (wallaby). This is because newer branches
use `http.client` to assert status codes and in stable/train we are
still using `http_client`. This change is functionally the same.

Change-Id: I903d20fa41270499ca1c39d296120dd97cef5405
Closes-Bug: 1901207
(cherry picked from commit 2d7bf10a5a145ed3b6e34c3cb95e05fb7e33e0d1)
(cherry picked from commit 80832de6ced534bccbf7cfe24a6a01c8262c1779)
(cherry picked from commit 661bf6a1dca502f37b5da9995c45ba60581e5e97)

3 files changed, 118 insertions, 2 deletions

diff --git a/keystone/api/users.py b/keystone/api/users.py
index 525c5d1cd..a883752f8 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 e97954f43..bf06e3f3f 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 000000000..26e957a0d
--- /dev/null
+++ b/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.