From 686c4453a184bdcf609f64dbd22897ce6a4d321e Mon Sep 17 00:00:00 2001 From: Colleen Murphy Date: Thu, 16 Apr 2020 12:37:37 -0700 Subject: [PATCH 2/2] Prohibit users from PATCHing metadata in EC2 cred When a user uses an application credential or a trust to create an EC2 credential, keystone automatically adds the trust ID or application credential ID as metadata in the EC2 access blob so that it knows how the token can be scoped when it is used. Without this patch, a user who has created a credential in this way can update the access blob to remove or alter this metadata and escalate their privileges to be fully authorized for the trustor's or application credential creator's privileges on the project. This patch fixes the issue by simply disallowing updates to keystone-controlled metadata in the credential. Change-Id: I1d657958c44ef5de8dfc3332e23704a4d4bb4d02 Closes-bug: #1872755 --- keystone/api/credentials.py | 45 ++++++++++----- keystone/tests/unit/test_v3_credential.py | 56 +++++++++++++++++++ .../notes/bug-1872755-2c81d3267b89f124.yaml | 19 +++++++ 3 files changed, 107 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/bug-1872755-2c81d3267b89f124.yaml diff --git a/keystone/api/credentials.py b/keystone/api/credentials.py index 201f7a20c..d477ed864 100644 --- a/keystone/api/credentials.py +++ b/keystone/api/credentials.py @@ -60,21 +60,24 @@ class CredentialResource(ks_flask.ResourceBase): ref['blob'] = jsonutils.dumps(blob) return ref + def _validate_blob_json(self, ref): + try: + blob = jsonutils.loads(ref.get('blob')) + except (ValueError, TabError): + raise exception.ValidationError( + message=_('Invalid blob in credential')) + if not blob or not isinstance(blob, dict): + raise exception.ValidationError(attribute='blob', + target='credential') + if blob.get('access') is None: + raise exception.ValidationError(attribute='access', + target='credential') + return blob + def _assign_unique_id(self, ref, trust_id=None, app_cred_id=None): # Generates an assigns a unique identifier to a credential reference. if ref.get('type', '').lower() == 'ec2': - try: - blob = jsonutils.loads(ref.get('blob')) - except (ValueError, TabError): - raise exception.ValidationError( - message=_('Invalid blob in credential')) - if not blob or not isinstance(blob, dict): - raise exception.ValidationError(attribute='blob', - target='credential') - if blob.get('access') is None: - raise exception.ValidationError(attribute='access', - target='credential') - + blob = self._validate_blob_json(ref) ref = ref.copy() ref['id'] = hashlib.sha256( blob['access'].encode('utf8')).hexdigest() @@ -159,16 +162,32 @@ class CredentialResource(ks_flask.ResourceBase): ref['id'], ref, initiator=self.audit_initiator) return self.wrap_member(ref), http.client.CREATED + def _validate_blob_update_keys(self, credential, ref): + if credential.get('type', '').lower() == 'ec2': + new_blob = self._validate_blob_json(ref) + old_blob = credential.get('blob') + if isinstance(old_blob, str): + old_blob = jsonutils.loads(old_blob) + # if there was a trust set, prevent changing it or unsetting it + if old_blob.get('trust_id') != new_blob.get('trust_id'): + raise exception.ValidationError( + message=_('trust_id can not be updated for credential')) + # if there was an app cred set, prevent changing it or unsetting it + if old_blob.get('app_cred_id') != new_blob.get('app_cred_id'): + raise exception.ValidationError( + message=_('app_cred_id can not be updated for credential')) + def patch(self, credential_id): # Update Credential ENFORCER.enforce_call( action='identity:update_credential', build_target=_build_target_enforcement ) - PROVIDERS.credential_api.get_credential(credential_id) + current = PROVIDERS.credential_api.get_credential(credential_id) credential = self.request_body_json.get('credential', {}) validation.lazy_validate(schema.credential_update, credential) + self._validate_blob_update_keys(current.copy(), credential.copy()) self._require_matching_id(credential) ref = PROVIDERS.credential_api.update_credential( credential_id, credential) diff --git a/keystone/tests/unit/test_v3_credential.py b/keystone/tests/unit/test_v3_credential.py index 2e38c87e2..fd08362b7 100644 --- a/keystone/tests/unit/test_v3_credential.py +++ b/keystone/tests/unit/test_v3_credential.py @@ -285,6 +285,62 @@ class CredentialTestCase(CredentialBaseTestCase): 'credential_id': credential_id}, body={'credential': update_ref}) + def test_update_ec2_credential_change_trust_id(self): + """Call ``PATCH /credentials/{credential_id}``.""" + blob, ref = unit.new_ec2_credential(user_id=self.user['id'], + project_id=self.project_id) + blob['trust_id'] = uuid.uuid4().hex + ref['blob'] = json.dumps(blob) + r = self.post( + '/credentials', + body={'credential': ref}) + self.assertValidCredentialResponse(r, ref) + credential_id = r.result.get('credential')['id'] + # Try changing to a different trust + blob['trust_id'] = uuid.uuid4().hex + update_ref = {'blob': json.dumps(blob)} + self.patch( + '/credentials/%(credential_id)s' % { + 'credential_id': credential_id}, + body={'credential': update_ref}, + expected_status=http.client.BAD_REQUEST) + # Try removing the trust + del blob['trust_id'] + update_ref = {'blob': json.dumps(blob)} + self.patch( + '/credentials/%(credential_id)s' % { + 'credential_id': credential_id}, + body={'credential': update_ref}, + expected_status=http.client.BAD_REQUEST) + + def test_update_ec2_credential_change_app_cred_id(self): + """Call ``PATCH /credentials/{credential_id}``.""" + blob, ref = unit.new_ec2_credential(user_id=self.user['id'], + project_id=self.project_id) + blob['app_cred_id'] = uuid.uuid4().hex + ref['blob'] = json.dumps(blob) + r = self.post( + '/credentials', + body={'credential': ref}) + self.assertValidCredentialResponse(r, ref) + credential_id = r.result.get('credential')['id'] + # Try changing to a different app cred + blob['app_cred_id'] = uuid.uuid4().hex + update_ref = {'blob': json.dumps(blob)} + self.patch( + '/credentials/%(credential_id)s' % { + 'credential_id': credential_id}, + body={'credential': update_ref}, + expected_status=http.client.BAD_REQUEST) + # Try removing the app cred + del blob['app_cred_id'] + update_ref = {'blob': json.dumps(blob)} + self.patch( + '/credentials/%(credential_id)s' % { + 'credential_id': credential_id}, + body={'credential': update_ref}, + expected_status=http.client.BAD_REQUEST) + def test_delete_credential(self): """Call ``DELETE /credentials/{credential_id}``.""" self.delete( diff --git a/releasenotes/notes/bug-1872755-2c81d3267b89f124.yaml b/releasenotes/notes/bug-1872755-2c81d3267b89f124.yaml new file mode 100644 index 000000000..a4293d5de --- /dev/null +++ b/releasenotes/notes/bug-1872755-2c81d3267b89f124.yaml @@ -0,0 +1,19 @@ +--- +security: + - | + [`bug 1872755 `_] + Added validation to the EC2 credentials update API to ensure the metadata + labels 'trust_id' and 'app_cred_id' are not altered by the user. These + labels are used by keystone to determine the scoped allowed by the + credential, and altering these automatic labels could enable an EC2 + credential holder to elevate their access beyond what is permitted by the + application credential or trust that was used to create the EC2 credential. +fixes: + - | + [`bug 1872755 `_] + Added validation to the EC2 credentials update API to ensure the metadata + labels 'trust_id' and 'app_cred_id' are not altered by the user. These + labels are used by keystone to determine the scoped allowed by the + credential, and altering these automatic labels could enable an EC2 + credential holder to elevate their access beyond what is permitted by the + application credential or trust that was used to create the EC2 credential. -- 2.26.0