Merge "Use app cred user ID in policy enforcement" into stable/train

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.