[OSSA 2013-014] auth_token middleware neglects to check expiry of signed token

Bug #1179615 reported by Eoghan Glynn
276
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Invalid
Undecided
Unassigned
Folsom
Fix Released
Critical
Adam Young
OpenStack Security Advisory
Fix Released
High
Thierry Carrez
python-keystoneclient
Fix Released
Critical
Adam Young

Bug Description

Unless I'm mistaken the keystoneclient auth_token middleware seems to be neglecting to check the expiry of signed tokens.

Instead, it only checks if the proposed token has been explicitly revoked:

  https://github.com/openstack/python-keystoneclient/blob/master/keystoneclient/middleware/auth_token.py#L1047

Surely the expiration timestamp needs to be checked also and the token rejected if expired.

Eoghan Glynn (eglynn)
description: updated
Revision history for this message
Adam Young (ayoung) wrote :
Revision history for this message
Eoghan Glynn (eglynn) wrote :

Verified that ayoung's patch addresses the issue in my devstack env.

Revision history for this message
Adam Young (ayoung) wrote :

This version is a little clearer in what it is testing.

Revision history for this message
Eoghan Glynn (eglynn) wrote :

Confirmed second patch also, so +1 from me.

Revision history for this message
Adam Young (ayoung) wrote :

Backport for folsom. auth_token middleware in Folsom was in Keystone, not in the keystone client.

Changed in python-keystoneclient:
assignee: nobody → Adam Young (ayoung)
status: New → Confirmed
Changed in python-keystoneclient:
importance: Undecided → Critical
Revision history for this message
Thierry Carrez (ttx) wrote :

Adding more keystone core eyes to +1 the proposed patches (keystone/folsom and python-keystoneclient)

Changed in keystone:
status: New → Invalid
Revision history for this message
Thierry Carrez (ttx) wrote :

Making sure I understand this correctly...

Your patch fixes _cache_put() so that it doesn't store expired tokens... But _cache_get() seems to properly implement expiration check so it will not return expired tokens. So is this really exploitable ? Or exploitable only if the token is not in the cache already ?

Revision history for this message
Adam Young (ayoung) wrote :

It seems like that is the case on first glance, but further investigation of the code shows otherwise. The cache_get function does not return anything if the value is expired, but then the calling code treats that as a cache miss, and re-verifies the token. For a UUID token, this would mean going back to the server, and thus get a status of "Invalid." However, with PKI tokens, it just reruns the validation code in process which was not checking the expiration time. So the original cache validation is not effective in checking token expiration.

Revision history for this message
Thierry Carrez (ttx) wrote :

OK, so this only affects PKI tokens ?

Revision history for this message
Eoghan Glynn (eglynn) wrote :

@ttx: only affects signed tokens (there's a separate codepath in the auth_token middleware for verifying UUID tokens) .

Revision history for this message
Dolph Mathews (dolph) wrote :

The test included in the patch couldn't be any simpler to illustrate the issue; Big +1 for that! (and it fails without the corresponding fix)

I'm lost though on why the patch is so big? Why do ALL the examples/pki/ files have to be recreated, rather than just adding the two new files? (auth_token_scoped_expired.json and auth_token_scoped_expired.xml). +2 otherwise!

Revision history for this message
Adam Young (ayoung) wrote :

the pem files are the signed versions of those files. We don't regenerate them on the fly. So, even though the only change in the JSON file is the date, it completely changes the signature.

Revision history for this message
Thierry Carrez (ttx) wrote :

Proposed impact description:

Title: Expiration check failure in Keystone PKI tokens validation
Reporter: Eoghan Glynn (Red Hat)
Products/Affects: Keystone (Folsom only), python-keystoneclient (0.2.0+)

Description:
Eoghan Glynn from Red Hat reported a vulnerability in expiry checks for PKI tokens in the Keystone authentication middleware. Expired tokens for authenticated users could continue to be used as long as the user wasn't disabled, potentially resulting in the bypass of intended security policies. Only setups using PKI tokens are affected.

Note:
The affected code was added to Keystone in the Folsom release, but was moved to python-keystoneclient during the Grizzly development cycle.

Revision history for this message
Eoghan Glynn (eglynn) wrote :

Proposed tweak to wording of impact description:

s/Title: Expiration check failure/Title: Missing expiration check/

Otherwise looks good to me.

Revision history for this message
Dolph Mathews (dolph) wrote :

ayoung: Ah, I see the date changes now (s/2012/2112/). That makes sense. +1 for patch.

On description: s/as long as the user wasn't disabled/as long as the token wasn't otherwise revoked/

Revision history for this message
Eoghan Glynn (eglynn) wrote :

@dolph

> On description: s/as long as the user wasn't disabled/as long as the token wasn't otherwise revoked/

That brings up interesting point.

If I'm reading the keystone code correctly, only *non-expired* revoked tokens are returned from Driver.list_revoked_tokens(), at least in the SQL case:

  https://github.com/openstack/keystone/blob/master/keystone/token/backends/sql.py#L128

So a token that has been both revoked *and* expired would not be reported as revoked (presumably the assumption here is that the token would fall in any case on the expiry-check hurdle, so no need to pollute the reported revoked list with really old tokens).

IIUC this makes the vulnerability a tad worse, as the effect of revocation is reversed if the token is re-used post-expiry.

Revision history for this message
Eoghan Glynn (eglynn) wrote :

I've confirmed that the effect of signed token revocation is reversed when the token is expires (as envisaged in the previous comment).

IMO this should be reflected in the impact description, e.g. by adding the line:

"The effect of signed token revocation is also reversed when the token expires, in the sense that a revoked token is once again treated as being valid."

Revision history for this message
Thierry Carrez (ttx) wrote :

This one is getting better and better :) New version:

---------------------------------------------------------
Title: Missing expiration check in Keystone PKI tokens validation
Reporter: Eoghan Glynn (Red Hat)
Products/Affects: Keystone (Folsom only), python-keystoneclient (0.2.0+)

Description:
Eoghan Glynn from Red Hat reported a vulnerability in expiry checks for PKI tokens in the Keystone authentication middleware. Expired tokens for authenticated users could continue to be used, potentially resulting in the bypass of intended security policies. The effect of PKI token revocation is also reversed when the token expires, in the sense that a revoked token is once again treated as being valid. Only setups using PKI tokens are affected.

Note:
The affected code was added to Keystone in the Folsom release, but was moved to python-keystoneclient during the Grizzly development cycle.

Revision history for this message
Eoghan Glynn (eglynn) wrote :

+1 on the latest impact description.

Revision history for this message
Dolph Mathews (dolph) wrote :

Wow.

+1 for description.

Revision history for this message
Thierry Carrez (ttx) wrote :

I don't think I have the formal +1 from two keystone-core devs yet. Could you comment to that effect ?

Revision history for this message
Adam Young (ayoung) wrote :

You have 2. I am one and Dolph is the other.

Revision history for this message
Thierry Carrez (ttx) wrote :

One could argue that I need two other than the patch author, but you guys decide (and Eoghan +1ed too).
Waiting for a CVE assignment, will probably push downstream early next week.

Revision history for this message
Dolph Mathews (dolph) wrote :

I asked bknudson to review.

Revision history for this message
Brant Knudson (blk-u) wrote :

I reviewed the patch in comment #3 and it looks good to me. +1.

Revision history for this message
Brant Knudson (blk-u) wrote :

I reviewed the statement in comment #18 and it looks good to me. +1

Only suggestion is I think "PKI token validation" reads better than "PKI tokens validation".

Revision history for this message
Thierry Carrez (ttx) wrote :

This is CVE-2013-2104

Revision history for this message
Thierry Carrez (ttx) wrote :

Sent downstream. Proposed public disclosure date/time: Tuesday, May 28, 1500UTC.

Changed in python-keystoneclient:
status: Confirmed → Triaged
Revision history for this message
Alex Meade (alex-meade) wrote :

@Thierry Feel free to co-credit me on this since you asked :)

Glad I wasn't the first to find this. I like the keystone client fix in comment #3

Revision history for this message
Thierry Carrez (ttx) wrote :

@Alex: will add "Alex Meade (Rackspace)" as co-reporter on the final advisory.

Revision history for this message
Adam Young (ayoung) wrote :

I am going to need to update the patch once https://review.openstack.org/#/c/30147 is merged, as the certs in the unit tests will be different from the ones used to sign the tokens. The content of the patch will be same, with the exception of data in keystone/tests/signing

Revision history for this message
Thierry Carrez (ttx) wrote :

OK, please update the bug as soon as possible once that merges, will communicate updated patch to stakeholders.

Thierry Carrez (ttx)
Changed in ossa:
status: New → In Progress
importance: Undecided → High
assignee: nobody → Thierry Carrez (ttx)
status: In Progress → Fix Committed
Revision history for this message
Adam Young (ayoung) wrote :
Revision history for this message
Thierry Carrez (ttx) wrote :

@ayoung: Thanks for the rebase. It looks like the python-keystoneclient patch also needs a rebase on top of 11263ac3 (to account for changes in tests/test_auth_token_middleware.py). Any chance you could have a look ?

Revision history for this message
Adam Young (ayoung) wrote :
Thierry Carrez (ttx)
information type: Private Security → Public Security
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to python-keystoneclient (master)

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

Changed in python-keystoneclient:
assignee: Adam Young (ayoung) → Thierry Carrez (ttx)
status: Triaged → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/folsom)

Fix proposed to branch: stable/folsom
Review: https://review.openstack.org/30743

Thierry Carrez (ttx)
summary: - auth_token middleware neglects to check expiry of signed token
+ [OSSA 2013-014] auth_token middleware neglects to check expiry of signed
+ token
Changed in python-keystoneclient:
assignee: Thierry Carrez (ttx) → Adam Young (ayoung)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to python-keystoneclient (master)

Reviewed: https://review.openstack.org/30742
Committed: http://github.com/openstack/python-keystoneclient/commit/8fe7a822d3fd9f435bb4a73a838b380659196cf7
Submitter: Jenkins
Branch: master

commit 8fe7a822d3fd9f435bb4a73a838b380659196cf7
Author: Adam Young <email address hidden>
Date: Tue May 28 09:50:51 2013 -0400

    Check Expiry

    Explicitly checks the expiry on the tokens, and rejects tokens that
    have expired

    had to regenerate the sample data for the tokens as they all had been
    generated with values that are now expired.

    bug 1179615

    Change-Id: Ie06500d446f55fd0ad67ea540c92d8cfc57483f4

Changed in python-keystoneclient:
status: In Progress → Fix Committed
Revision history for this message
Matthew Thode (prometheanfire) wrote :

Would it be possible to get a patch rebased against python-keystoneclient-0.2.3 (so I can get a fix out for it)?
also, keystone-folsom is fixed in gentoo

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Ubuntu is also interested in a patch against 0.2.3. Thanks!

Revision history for this message
Thierry Carrez (ttx) wrote :

FYI we are still investigating why the proposed stable/folsom patch fails integration tests, please stay put.

Revision history for this message
Thierry Carrez (ttx) wrote :

Failure reason found, slightly modified patch coming up

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (stable/folsom)

Reviewed: https://review.openstack.org/30743
Committed: http://github.com/openstack/keystone/commit/8d23da1302dde9d38bbc227d9aba30da919b60c8
Submitter: Jenkins
Branch: stable/folsom

commit 8d23da1302dde9d38bbc227d9aba30da919b60c8
Author: Adam Young <email address hidden>
Date: Mon May 13 16:07:51 2013 -0400

    Check token Expiration

    Backport for Folsom.

    Bug 1179615

    Change-Id: I8516d87ffc72cf35d3bff6fc21cb5324da4ad2bb

Revision history for this message
Thierry Carrez (ttx) wrote :

OSSA 2013-014

Changed in ossa:
status: Fix Committed → Fix Released
Revision history for this message
Thierry Carrez (ttx) wrote :

My try at a minimal backport over python-keystoneclient 0.2.3...

Revision history for this message
Thierry Carrez (ttx) wrote :

python-keystoneclient 0.2.4 (including this fix) is available on Pypi

Revision history for this message
Matthew Thode (prometheanfire) wrote :

Thanks for the release, makes it easier. I made a more full backport patch for python-keystoneclient-0.2.3 just now as well (not really tested, but here you go).

bug fully fixed in gentoo as of sys-auth/keystone-2012.2.4-r4 and dev-python/python-keystoneclient-0.2.4

Dolph Mathews (dolph)
Changed in python-keystoneclient:
milestone: none → 0.2.4
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers