[OSSA 2012-014] Token validation includes revoked roles (CVE-2012-4413)

Note: I'm suddenly having trouble getting the stable/essex S3 & swift middleware tests to run on my machine (both with and without this patch) -- but I don't think they should be impacted, regardless.

We need a couple of keystone-core reviews on these patches.

Here is a draft description. Right now it's mostly one long awkward sentence. There's probably a more elegant way of describing it ...

Title: Revoking a role does not affect existing tokens
Impact: High
Reporter: Dolph Mathews (Rackspace)
Products: Keystone
Affects: Essex, Folsom

Description:
Dolph Mathews reported a vulnerability in Keystone. If you revoke a role from a user from the admin API and then validate a token that existed before revoking the role, the token validation response will still include that role.

I just looked through reviewboard and didn't see any patches pending review.

-joe

On Aug 27, 2012, at 2:16 PM, Russell Bryant <email address hidden> wrote:
> We need a couple of keystone-core reviews on these patches.
>
> Here is a draft description. Right now it's mostly one long awkward
> sentence. There's probably a more elegant way of describing it ...
>
>
> Title: Revoking a role does not affect existing tokens
> Impact: High
> Reporter: Dolph Mathews (Rackspace)
> Products: Keystone
> Affects: Essex, Folsom
>
> Description:
> Dolph Mathews reported a vulnerability in Keystone. If you revoke a role from a user from the admin API and then validate a token that existed before revoking the role, the token validation response will still include that role.
>
> --
> You received this bug notification because you are a member of Keystone
> Bugs, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1041396
>
> Title:
> Token validation includes revoked roles
>
> Status in OpenStack Identity (Keystone):
> In Progress
>
> Bug description:
> To reproduce:
> 1) Initial setup: http://paste.openstack.org/raw/20477/
> 2) Authenticate as a normal user, generating a token
> 3) On the admin API, revoke a role from that user
> 4) On the admin API, validate the user's generated token
>
> The revoked role is included in the validation response. Ideally, the
> token should be entirely invalidated and return 404, although at the
> very least, the revoked role should *NOT* be included in the
> validation response.
>
> Full example:
>
> Authenticate as a user who has been granted the 'manager' role:
>
> POST http://127.0.0.1:5000/v2.0/tokens
> ======================================
>
> Content-Type: application/json
>
> {
> "auth": {
> "tenantName": "project-x",
> "passwordCredentials": {
> "username": "joe",
> "password": "secret"
> }
> }
> }
>
> 200 OK
> ======
>
> Date: Fri, 24 Aug 2012 22:43:24 GMT
> Vary: X-Auth-Token
> Content-Length: 448
> Status: 200
> Content-Type: application/json
>
> {
> "access": {
> "token": {
> "expires": "2012-08-25T22:43:24Z",
> "id": "c0db082bdb7f47e4845d7be551558620",
> "tenant": {
> "id": "10e2a090121748388cf39e915d376f29",
> "enabled": true,
> "description": null,
> "name": "project-x"
> }
> },
> "serviceCatalog": {},
> "user": {
> "username": "joe",
> "roles_links": [],
> "id": "b2a6f8d5dbb249f3b9ac8a46e8cb77e6",
> "roles": [
> {
> "name": "manager"
> }
> ],
> "name": "joe"
> },
> "metadata": {
> "is_admin": 0,
> "roles": [
> "facd80ce22d44eae87375f11295f8e51"
> ]
> }
> }
> }
>
> Validate the user's token (note the presence of the 'manager' role):
>
> GET http://127.0.0.1:35357/v2.0/tokens/c0db082bdb7f47e4...

The patches are attached to this bug since it's a security vulnerability.

"Granting and revoking roles from a user is not reflected upon token validation for pre-existing tokens. Pre-existing tokens continue to be valid for the original set of roles for the remainder of the token's lifespan, or until explicitly invalidated."

The proposed patch invalidates all tokens held by a user upon role grant/revoke to circumvent the issue.

+2 from me on the patches, although we need to be aware that the solution to this vulnerability is *dependent* on the token backend being able to list tokens through the internal token API.

The patches current invoke a WARN log message in that case (i.e. listing tokens returning a NotImplemented exception) - should this be an "ERROR" and is it actionable?

I think WARN is appropriate here, but I thought it worth asking the question.

So it looks like memcache is the only token backend that would still have this problem? We should note that in the description. If someone is using the memcache backend, presumably they are accepting this shortcoming. Are they going to get annoyed with getting this warning in their log over and over? Perhaps it should only be logged once?

Updated description:

Title: Revoking a role does not affect existing tokens
Impact: High
Reporter: Dolph Mathews (Rackspace)
Products: Keystone
Affects: Essex, Folsom

Description:
Dolph Mathews reported a vulnerability in Keystone. Granting and revoking roles from a user is not reflected upon token validation for pre-existing tokens. Pre-existing tokens continue to be valid for the original set of roles for the remainder of the token's lifespan, or until explicitly invalidated.

The proposed patch invalidates all tokens held by a user upon role grant/revoke to circumvent the issue. Note that due to how the memcache token backend works, it will still be affected by this issue.

-2

Please add in code to disable the memcached backend for tokens. Token revocation cannot be done on Memcached, which means that even past changes to deal with revocation do not cover it.

Adam: Not sure exactly what you're asking for here? "code to disable the memcached backend for tokens."

I interpreted it as "we should get rid of memcache token backend support completely" ...

@Dolph, Adam: any chance you could work out a patch that would satisfy you both ? I'd like to close this one asap...

Proposal for Memcached: Each token created gets written into the users token list which is persisted to memcached as well. Upon user disable and revocation events, remove all tokens in the users list.

Applying Adam's patch for bug 1046905 (currently in review: https://review.openstack.org/#/c/12506/ ) *along with* the above patch should solve this issue for the memcache driver.

Keystone-core: please confirm that patches above are OK

Dolph: do they need any rebasing now that Adam's patch landed ?

All: How does the complete fix affect the proposed description in comment 8 ?

Dolph's patch should not require rebasing. It changes:

keystone/identity/core.py

The other patch only changed
 keystone/token/backends/memcache.py
 tests/test_backend.py
 tests/test_backend_memcache.py

Dolph's patch was no explicitly skipping the memcached backend, the problem was entirely in memcached.

This completely addresses the issue in comment 8. The whole thing now needs testing together.

Patches could use a bit of refactoring, to avoid the copy-paste of code.

Adding Mark to make sure that behavior change (invalidate all the user tokens whenever a role is granted or revoked) would be acceptable for stable/essex.

Fixed proposed description:
"""
Title: Revoking a role does not affect existing tokens
Impact: High
Reporter: Dolph Mathews (Rackspace)
Products: Keystone
Affects: Essex, Folsom

Description:
Dolph Mathews reported a vulnerability in Keystone. Granting and revoking roles from a user is not reflected upon token validation for pre-existing tokens. Pre-existing tokens continue to be valid for the original set of roles for the remainder of the token's lifespan, or until explicitly invalidated. This fix invalidates all tokens held by a user upon role grant/revoke to circumvent the issue.
"""

I don't think leaving the current behaviour is acceptable for stable/essex

Ideally, the behaviour change to fix this wouldn't be so violent, but I think we're unlikely to break any clients here ... clients can't really assume their token won't be invalidated at any point and removing roles etc. doesn't happen often

So, yeah - fine by me for stable/essex

Refactored implementation into token API.

Keystone core, please ack latest patch

We should drop the catch block in revoke_tokens long term, as not supporting list is not acceptable any more. Failure to support list should make things blow up.

agreed with Adam - not support token_list() is opening a security hole

patches are good with https://review.openstack.org/#/c/12506/ merged in place (which it is in master)

we'll need a https://review.openstack.org/#/c/12506/ equiv against stable-essex as well.

So would you guys like to update the patch so it makes not having token_list an error?

Also, Adam, can you prepare an Essex version of the memcache changes? We will need that before we can send out the advance notification of the advisory. If we can get it out today, we'll be able to release all of this next week and have it in folsom-rc1.

stable-essex version.

https://review.openstack.org/#/c/12590/1

I get a "Patch format detection failed" with the patches above. They should be generated using git format-patch.

Agree; dropped the 'try.' An implementation of list_tokens is now *required* in the token driver in order to grant and revoke roles.

Alternative version of above patches produced with format-patch

removed the try block.

I missed a LOG reference that was no longer used in patch v3 -- updated here.

I can approve both patches.

Note that the essex patch requires https://review.openstack.org/#/c/12590/

Cool, thanks guys! Based on heckj's ack earlier and agreement that token_list should be mandatory, I'm going to go ahead and push out the advance notification. If any further changes are needed, I can push out updates.

Advance notification has been sent. Thanks for working on this today, guys!

Proposed public disclosure date/time:
Wednesday, September 12th, 1500 UTC

Fix proposed to branch: master
Review: https://review.openstack.org/12868

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/12870

Pushed both patches as draft in preparation for release today at 1500 UTC. Feel free to test.

Patches public now.

Reviewed: https://review.openstack.org/12868
Committed: http://github.com/openstack/keystone/commit/efb6b3fca0ba0ad768b3e803a324043095d326e2
Submitter: Jenkins
Branch: master

commit efb6b3fca0ba0ad768b3e803a324043095d326e2
Author: Dolph Mathews <email address hidden>
Date: Fri Sep 7 14:35:21 2012 -0500

    Delete user tokens after role grant/revoke

    Delete user tokens when a new role is granted or revoked, in order to
    prevent old tokens to continue to be valid for the original set of
    roles for the remainder of the token's lifespan.

    Addresses CVE-2012-4413.
    Fixes bug 1041396.

    Change-Id: Iecf891f274b67408f568b949a7028362c4c30312

Reviewed: https://review.openstack.org/12870
Committed: http://github.com/openstack/keystone/commit/58ac6691a21675be9e2ffb0f84a05fc3cd4d2e2e
Submitter: Jenkins
Branch: stable/essex

commit 58ac6691a21675be9e2ffb0f84a05fc3cd4d2e2e
Author: Dolph Mathews <email address hidden>
Date: Fri Sep 7 14:55:31 2012 -0500

    Delete user tokens after role grant/revoke

    Delete user tokens when a new role is granted or revoked, in order to
    prevent old tokens to continue to be valid for the original set of
    roles for the remainder of the token's lifespan.

    Addresses CVE-2012-4413.
    Fixes bug 1041396.

    Change-Id: Ib11b5b3a933c6000afe0c875c3f71f1f101bb202

OSSA 2012-014 published

Hello Dolph, or anyone else affected,

Accepted keystone into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/keystone/2012.1.3+stable-20130423-f48dd0fc-0ubuntu1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Please find the attached test log from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Keystone has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

For records of upstream test coverage of this update, please see the Jenkins links in the comments of the relevant upstream code-review(s):

Trunk review: https://review.openstack.org/12868
Stable review: https://review.openstack.org/12870

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

Test coverage log.

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.