Token in URL is a security risk

Bug #861854 reported by Ziad Sawalha
24
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
High
Dolph Mathews

Bug Description

Raised by anotherjesse. The GET /tokens/id use case requires the token in the URL. The token by itself provides access to resources, so having that go to an HTTP log is a security risk, especially for tokens with a long life.

Revision history for this message
Joe Savak (jsavak) wrote :

How should we fix this? Should we hash the token so it goes encrypted over the wire? Should we re-design the validate-token? Would having the token as an http header in the call help?

Revision history for this message
Robin Norwood (robin-norwood-8) wrote :

The calls in question are described in the dev guide here:

http://docs.openstack.org/incubation/identity-dev-guide/content/Validate_Token-d1e1914.html

The use case here is to validate the token, so my suggestion is to replace the operation to validate a token from GET /tokens/<id> to POST /tokens/validate with a body that includes the token ID and tenant:

POST /tokens/validate
{
    "token": {
      "id": "asdasdasd-adsasdads-asdasdasd-adsadsasd",
     "tenantId": "1234"
}

That way the id will be encrypted for clients using SSL and not show up in the log.

Joe Savak (jsavak)
Changed in keystone:
importance: Undecided → Critical
Revision history for this message
Joe Savak (jsavak) wrote :

I like this solution but would this be violating ReST principles in any way? POST should be used for creates, and validating a token doesn't really create anything.

Revision history for this message
Robin Norwood (robin-norwood-8) wrote :

Yes, my understanding is that it isn't as ReSTfully pure as the original solution...but security should trump restful purity, and my suggestion is the most straightforward way I could think of...not to say that there aren't other (better) solutions possible.

Revision history for this message
Ziad Sawalha (ziad-sawalha) wrote :

I agree with Robin's solution and logic:

The use case here is to validate the token, so my suggestion is to replace the operation to validate a token from GET /tokens/<id> to POST /tokens/validate with a body that includes the token ID and tenant:

POST /tokens/validate
{
    "token": {
      "id": "asdasdasd-adsasdads-asdasdasd-adsadsasd",
     "tenantId": "1234"
}

That way the id will be encrypted for clients using SSL and not show up in the log.

We should add this as an additional call and not break the old one until we have verified clients are not using it. It should also be documented as the recommended call and the old call should be clearly marked as an insecure call.

Changed in keystone:
milestone: none → essex-2
Revision history for this message
Yogeshwar (yogesh-srikrishnan) wrote :

Elegant solution.Minor concern.Should validate be a query param instead of path param.
Should all resources not be a nown?

Revision history for this message
Yogeshwar (yogesh-srikrishnan) wrote :

Also we have a shorthand HEAD call /tokens/{tokenId}. I dont thin HEAD call would take body.
How do we plan to support it?

Revision history for this message
Jorge L. Williams (jorgew) wrote : Re: [Bug 861854] Re: Token in URL is a security risk

The approach is a misuse of HTTPs uniform interface. If it's necessary for security reasons then so be it.

That said, I'm wondering if a better approach would be to simply encrypt the token in the URI for the purpose of logging. Or better yet (and I realize I'm setting the bar high here) encrypt and base64 the token before placing it in the URI. The caller's own token can be used as the shared secret for the encryption, since it will be encrypted anyway by the SSL.

We might use something like this: http://www.codekoala.com/blog/2009/aes-encryption-python-using-pycrypto/

Revision history for this message
Ziad Sawalha (ziad-sawalha) wrote :

That could be a good solution. It won't need an API change either. Thanks!

On 11/8/11 11:28 PM, "Jorge Williams" <email address hidden> wrote:

>
>The approach is a misuse of HTTPs uniform interface. If it's necessary
>for security reasons then so be it.
>
>That said, I'm wondering if a better approach would be to simply encrypt
>the token in the URI for the purpose of logging. Or better yet (and I
>realize I'm setting the bar high here) encrypt and base64 the token
>before placing it in the URI. The caller's own token can be used as the
>shared secret for the encryption, since it will be encrypted anyway by
>the SSL.
>
>We might use something like this:
>http://www.codekoala.com/blog/2009/aes-encryption-python-using-pycrypto/
>

Revision history for this message
Yogeshwar (yogesh-srikrishnan) wrote :

Could we not have token as a header
/tokens GET and HEAD
X-VALIDATE-TOKEN - Token to be validated.
We then dont have to expect clients to do encryption and encoding.

Revision history for this message
Yogeshwar (yogesh-srikrishnan) wrote :

I do like Jorge's approach for its consistency.However it adds more work on the clients part.

Revision history for this message
Jay Pipes (jaypipes) wrote :

FWIW, we just added similar functionality in Glance because of security credentials in the Location field:

https://github.com/openstack/glance/commit/5e6fb33b22c868e8ce2638198c27f3e3a099318a

Might be something to look at :)

-jay

Revision history for this message
Robin Norwood (robin-norwood-8) wrote :

In this case I'd argue for simplicity and ease of use over HTTP/REST purity. We could probably "noun-ify" the validate call into something like POST /validation-request, but that doesn't seem to gain anything to me. Otherwise, if we use the scheme Jay points to, at least there's consistency between openstack projects.

Revision history for this message
Yogeshwar (yogesh-srikrishnan) wrote :

Regardless of the option we chose I think we should stick to GET/HEAD to better support caching.

Changed in keystone:
status: New → Confirmed
Joe Savak (jsavak)
Changed in keystone:
milestone: essex-2 → essex-3
Joe Savak (jsavak)
Changed in keystone:
assignee: nobody → Rackspace Integration (rackspace-integration)
Revision history for this message
Ziad Sawalha (ziad-sawalha) wrote :

I'm going to build an extensions for this.
Middleware can query if the extension is there (OS-KSVALIDATE). If so, it can send an encrypted token as a header to tokens/validate.
I'll support everything under token/:id, including token/:id/endpoints (so that would be token/validate/endpoints)
I'll propose it and see where it goes from there.

Changed in keystone:
assignee: Rackspace Integration (rackspace-integration) → Ziad Sawalha (ziad-sawalha)
Changed in keystone:
status: Confirmed → In Progress
Revision history for this message
Joe Savak (jsavak) wrote :

Ziad was able to find a solution that preserves backwards compatibility.

Solution adds the following call in an extension:

GET v2.0/OS-KSVALIDATE/token/validate
     Passing in a header with X-Subject-Token (ex: T1000)
     Also passing in the X-Auth-Token (service token)

In the background this maps to v2.0/tokens/T1000

This has the benefit of not changing or breaking the core API. It's backwards compatible. In the middleware now, you can query the server to see if it supports the OS-KSVALIDATE extension and use this more secure call instead of the core call.

You can make this call with any of the token core APIs

Ex:
GET v2.0/OS-KSVALIDATE/token/endpoints maps to GET v2.0/tokens/{tokendID}/endpoints

Notes on the solution:

Maybe need option to disable core v2.0/tokens (503?)

Logging also needed with message to indicate to devops to use OS-KSVALIDATE extension call.

Documentation changes may also help make devops aware.

May also need VARY header (X-SUBJECT-TOKEN, X-AUTH-TOKEN)

Revision history for this message
Joe Savak (jsavak) wrote :

For the check token call:
HEAD v2.0/tokens/(tokenid)?belongsTo=string (where string is the tenant in scope).

This is mapped to:
HEAD v2.0/OS-KSVALIDATE/token/validate

But if v2.0/tokens is disabled, it will return a 503 with no message-body in the response. The user will need to try a GET if they want more information or don't understand the 503 by itself.

Changed in keystone:
status: In Progress → Fix Committed
Revision history for this message
Dolph Mathews (dolph) wrote :

Resetting the status of this bug because the above fix is not present in the new codebase (AFAIK).

Changed in keystone:
milestone: essex-3 → none
status: Fix Committed → Confirmed
Revision history for this message
Joseph Heck (heckj) wrote :

thanks Dolph!

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

Cool, Joe just added the VMT. For some reason we couldn't access this bug (probably created before we set up the team)

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

@heckj: do you agree this is a known weakness of the current API that needs to be strengthened, rather than a directly-exploitable security vulnerability ? In which case we will open this bug publicly rather than continue to consider it under embargo.

Revision history for this message
Joseph Heck (heckj) wrote :

@ttx - yes, weakness in the API setup. The V3 proposed API will resolve (I hope) this issue

Revision history for this message
Joseph Heck (heckj) wrote :

(open the bug)

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

Opened.

visibility: private → public
Thierry Carrez (ttx)
Changed in keystone:
assignee: Ziad Sawalha (ziad-sawalha) → nobody
importance: Critical → High
Thierry Carrez (ttx)
tags: added: security
security vulnerability: yes → no
Revision history for this message
Matt Joyce (matt-nycresistor) wrote :

So with v3 not going into folsom should someone attempt to triage this now while folsom is still in RC?

Revision history for this message
Joseph Heck (heckj) wrote :

Matt,

This has been triaged - and the solution is a new/updated API sequence, so it's resolution is entirely dependent on the V3 API

Revision history for this message
Matt Joyce (matt-nycresistor) wrote :

Yeah that's my point. v3 isn't landing for folsom ( as I understand it ). So do we want to put in an alternative fix until it does land?

Revision history for this message
Joseph Heck (heckj) wrote :

Matt -

what fix would you suggest that isn't changing the API?

Revision history for this message
Matt Joyce (matt-nycresistor) wrote :

Not sure atm. Trying to take a look now. Maybe we could do something to supress the value in logs / stacktraces. Sanitize at the very least.

Revision history for this message
Joseph Heck (heckj) wrote :

More than happy to hear your suggestions, but the fundamental issue here is that the token is the URL, and its that passing through proxies, etc. that represents the disclosure of a potential security issue.

Revision history for this message
Matt Joyce (matt-nycresistor) wrote :

Obviously there's not much we can do about that without modifying the API.

Dolph Mathews (dolph)
tags: added: v3api
Changed in keystone:
assignee: nobody → Dolph Mathews (dolph)
Revision history for this message
Dolph Mathews (dolph) wrote :

This was addressed by revving the Identity API to v3 and implemented by gyee as part of bp stop-ids-in-uris

v3 spec: https://github.com/openstack/identity-api/blob/master/openstack-identity-api/src/markdown/identity-api-v3.md

Changed in keystone:
status: Confirmed → Fix Committed
Thierry Carrez (ttx)
Changed in keystone:
milestone: none → grizzly-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in keystone:
milestone: grizzly-3 → 2013.1
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Related blueprints