Password Change Doesn't Affirmatively Invalidate Sessions

[Dan Reif]

The password change dialog at /horizon/settings/password/ contains the following code:

<code>
        if user_is_editable:
            try:
                api.keystone.user_update_own_password(request,
                                                    data['current_password'],
                                                    data['new_password'])
                response = http.HttpResponseRedirect(settings.LOGOUT_URL)
                msg = _("Password changed. Please log in again to continue.")
                utils.add_logout_reason(request, response, msg)
                return response
            except Exception:
                exceptions.handle(request,
                                  _('Unable to change password.'))
                return False
        else:
            messages.error(request, _('Changing password is not supported.'))
            return False
</code>

There are at least two security concerns here:
1) Logout is done by means of an HTTP redirect. Let's say Eve, as MitM, gets ahold of Alice's token somehow. Alice is worried this may have happened, so she changes her password. If Eve suspects that the request is a password-change request (which is the most Eve can do, because we're running over HTTPS, right? Right!?), then it's a simple matter to block the redirect from ever reaching the client, or the redirect request from hitting the server. From Alice's PoV, something weird happened, but her new password works, so she's not bothered. Meanwhile, Alice's old login ticket continues to work.
2) Part of the purpose of changing a password is generally to block those who might already have the password from continuing to use it. A password change should trigger (insofar as is possible) a purging of all active logins/tokens for that user. That does not happen here.

Frankly, I'm not the least bit sure if I've thought of the worst-case scenario(s) for point #1. It just strikes me as very strange not to aggressively/proactively kill the ticket/token(s), instead relying on the client to do so. Feel free to apply minds smarter and more devious than my own!

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

[Paul McMillan (paul-mcmillan)]

While I agree this is a bug (the logout call should include an explicit session destruction), I'm not particularly overwhelmed by its severity.

Remember that the default session store for Horizon stores data in a client cookie. If an attacker gets access to that, they've got complete access to everything a Horizon user can do. Keystone should probably invalidate those tokens on a password change (I don't know if it actually does), but that's not Horizon's concern. If it does invalidate those tokens, the old session stored by horizon becomes useless and this bug is invalid.

It looks like that might happen? I'm not familiar enough with the keystone codebase to know by inspection if that really is what's happening here:
https://github.com/openstack/keystone/blob/3e80e0b005fb045acd83c8879169824f31a125b3/keystone/contrib/user_crud/core.py#L85

In the case of a non-cookie based session backend, you are correct that a blocked redirect to the logout url will prevent the user from being logged out and the previous session will remain valid. An attack of this form requires an active MITM and the technical ability to do traffic analysis to look into the opaque TLS session and interrupt it at just the correct moment. You _might_ be able to do it based on size of response or response timing. This is particularly hard if the connection is using keepalive, pipelining, or spdy. Horizon pages tend to be relatively uniform in size and resources, so you don't have a lot of latitude there.

In any case "just blocking this one response" is non-trivial to the point that I don't expect that you can demonstrate it reliably against a properly configured server. An attacker who has MITM can block _all_ traffic to prevent both a password reset and this session logout thing, which is functionally similar.

Your point 2 is entirely beyond the scope of Horizon. If what you say is true, you need to take it up with the keystone team.

While I agree that this is a bug related to security, I'm tempted to vote that it is not one we need to keep private.

[Lin Hua Cheng (lin-hua-cheng)]

Concern #1 - Paul have provided a good analysis of the issue. I don't think there is anything Horizon can do to prevent this, this is more of an opportunity for Keystone.

Concern #2 - Do you mean in Keystone?

Horizon does actually makes an effort to clean up the token, it makes a call to invalidate token in these two cases:
a. Whenever the user switches projects, the last project scoped token is invalidated.
https://github.com/openstack/django_openstack_auth/blob/1.1.7/openstack_auth/views.py#L196
b. Whenever the logout view is hit, the current token used is invalidated.
https://github.com/openstack/django_openstack_auth/blob/1.1.7/openstack_auth/views.py#L125

[Thierry Carrez (ttx)]

Adding Keystone devs for analysis

[Dan Reif (a-launchpad20130227)]

Excellent investigation by all involved, and I agree, especially as regards point #2. Given that I don't know Keystone's API, I suppose I was hoping there was a "destroy all tokens associated with this user" call, open to anyone authenticated as that user. Briefly searching the API, however, I don't see one.

Overall, it looks like an opportunity for a small improvement in Horizon (as Paul said, destroy the session), and a larger discussion with the Keystone folks. Is it most appropriate to shunt this over to them, to loop in some of them, or to open a new issue over there?

Thank you again for your very thoughtful and thorough analysis.

[Brant Knudson (blk-u)]

When Keystone changes the user's password all tokens for the user are revoked.

[Adam Young (ayoung)]

Brant is correct.

What does not happen upon change is that Horizon does not re-authenticate tokens at any point. So if a user changes their password, while their token is revoked, Horizon does not know this.

However, any operation on a remote system will require a token validation, and will fail. So the only surface for attack is the data in the Horizon session itself.

If Horizon gets back any error that indicates a token is invalid, it should probably validate the token itself.

[Dolph Mathews (dolph)]

If there is any work for Horizon to do here, it's not private security related, but rather UX related around the process of re-authenticating the user after they've taken an action that causes their existing authentication to be invalidated. That said, I don't see anything particularly wrong with the approach Horizon is taking in the code provided in the description.

I'm currently trying to validate Brant's statement in comment #6, as I'm not confident we have sufficient scenario tests for this. Please maintain this as Private Security for the moment.

[Morgan Fainberg (mdrnstm)]

The intention here is as Brant describes, a password change *should* invalidate all active tokens.

https://github.com/openstack/keystone/blob/52738c6f5ab4383b3c9fbb170a3aeded7a8b2786/keystone/identity/core.py#L850-L857

calls update user

update user has a check for password invalidation:

https://github.com/openstack/keystone/blob/52738c6f5ab4383b3c9fbb170a3aeded7a8b2786/keystone/identity/core.py#L649-L650

Horizon doesn't do the re-auth with the new password, hence the redirect to relogin. The passwords should be invalidated on password changes (administrative OR by the user).

We should have at least a couple scenario tests for this, however, we may want to increase the coverage.

[Morgan Fainberg (mdrnstm)]

Whoopse, typo in my last comment:

Tokens should be invalidated on password changes.

[Brant Knudson (blk-u)]

The way I validated it was: Got 2 tokens, did keystone requests using both tokens, changed the user password, then tried doing the requests again. Those requests are expected to now fail.

[Dolph Mathews (dolph)]

Keystone has four API calls that can result in a user's password being changed, and as far as I can see, we only have a security-focused test for this scenario that covers API call (1):

1) Administrative password reset on v2: POST /v2.0/users/{user_id}/OS-KSADM/password
2) Self-service password change on v2: PATCH /v2.0/OS-KSCRUD/users/{user_id}
3) Administrative password reset on v3: POST /v3/users/{user_id}
4) Self-service password change on v3: POST /v3/users/{user_id}/password

I wrote additional tests for stable/juno which ensure the correct token invalidation behaviors consistently across all these APIs (and they pass). The patch does not cleanly apply to stable/icehouse or master, so I haven't tested those branches yet (but I'm sure master will pass, and I think stable/icehouse is missing at least one of these APIs anyway).

[Thierry Carrez (ttx)]

If I understand correctly, there is no vulnerability here, just room for tighter behavior in Horizon and additional tests to make sure we don't regress in the future in Keystone. If that's the case, I propose we open this bug up and work on improvements in the open ?

[Dolph Mathews (dolph)]

Thierry, I agree and I'd like to fix this in the open, but not before ensuring that the above tests pass on master and stable/icehouse (porting those tests is on my plate for today).

[Dolph Mathews (dolph)]

Backport of tests to stable/icehouse is attached.

[Dolph Mathews (dolph)]

Tests for keystone master are attached.

[Dolph Mathews (dolph)]

If keystone reviewers agree that the above tests illustrate that there is no vulnerability in Keystone, I'm in favor of opening this bug publicly and submitting the above patches via gerrit.

[Brant Knudson (blk-u)]

I took a quick look at the master tests and they look like they cover it. So there's no new vulnerability in Keystone here. I think the reporter assumed that tokens weren't revoked on a password change operation (which is the safe thing to assume, and I'm not sure if it's documented anywhere that tokens are revoked).

[Lin Hua Cheng (lin-hua-cheng)]

@Adam: We can enhance Horizon to redirect to login page when the token is expired. However, figuring out when the token is expired is tricky.

When the token is expired, Horizon is not aware until it uses the token to make an API call to OpenStack and eventually gets an UnAuthorized exception from the python-client. It seems like the python-client returns the same response/exception for expired token and unauthorized access, does Keystone returns different response for those exceptions?

[Adam Young (ayoung)]

It really doesn't matter what Keystone does, as calls to Keystone are not part of the standard operations for most openstack users. Once a Token is in Horizon, Horizon doesn't call back the Keystone except for:

1. Keystone specific operations
2. Switching projects

A failure from Keystone returns a 400 series error code to the client. Processing varies depending on which call and the error; some APIs return 404s to avoid giving away information about entities, others 403 or 401 as appropriate.

[Lin Hua Cheng (lin-hua-cheng)]

I understand how Horizon uses the Keystone token. I am trying to understand why the python-client returns the same UnAuthorized exception for either expired token and unauthorized access. I guess giving a generic error is by design (to not give away information).

If that is the case, there is no way for Horizon to distinguish those two from the UnAuthorized exception it gets from python-client. Horizon have to make a validate token call separately just to check if the token is actually expired, not sure if its worth the extra call just to be able to redirect the user directly to the login page.

[Brant Knudson (blk-u)]

When horizon gets a token, the response includes the expiration time for the token. So Horizon could tell if the token was expired or not and also it could get a token before the old one expired. Whether the token is expired or has been revoked there's really only one thing for Horizon to do -- get a new token. Tokens can be revoked for reasons other than a password change.

[Lin Hua Cheng (lin-hua-cheng)]

If the token is revoked outside of Horizon, Horizon has no way to get a new token since Horizon does not store user credentials. We just have to redirect the user to the login screen.

Currently, the python-client returns the same UnAuthorized exception for both:
* revoked/expired token
* unauthorized access

Would be helpful if Horizon can figure out when the exception is really caused by the revoked/expired token, that way we can redirect the user to the login page.

[Morgan Fainberg (mdrnstm)]

This might be a result of mixing 401 and 403 responses.

401 - unauthorized, this should mean that the user is *not* authenticated and a re-authentication should be sufficient to perform an action (revoked, expired, etc token).
403 - Forbidden, this should mean the current authorization doesn't allow the action to be performed.

In the 401 case redirecting to login should be sane (this may not actually be the case).

[Morgan Fainberg (mdrnstm)]

401 - s/sufficent/required [does not imply the user will be authorized once re-authentication occurs]

[Lin Hua Cheng (lin-hua-cheng)]

@Morgan: Interesting point, I'll do some more testing on some of the python-*client to see where the mix-up is happening.

Anyway, there is no security issue here for Horizon. Just opportunity for UX improvement.

[Brant Knudson (blk-u)]

If Horizon wants to know if a request was rejected due to unauthorized access (I assume this means that it was rejected by policy) vs an invalid token (either by expiration or revocation), it can call the validate token API with the token (GET or HEAD /v3/auth/tokens). If the identity server says the token isn't valid then you need a new token.

[Lin Hua Cheng (lin-hua-cheng)]

Morgan: You are correct about the response! Just tested this on devstack.

401 Unauthorized - is returned if I passed an invalid token
403 Forbidden - is returned if the policy rejects it.

From Horizon, we can redirect the user to the login page whenever we get an UnAuthorized exception. That should improve the UX for the case mentioned in this bug.

[Morgan Fainberg (mdrnstm)]

I think it is safe to make his public, add the tests to Keystone and let horizon work on the UX. All work at this point could be done in the open.

[Jeremy Stanley (fungi)]

Given the overwhelming consensus that this isn't exploitable, I've switched the bug to public and marked the security advisory task "won't fix" so this can just be worked as a normal bug/hardening opportunity.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/146589
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=a22aa08bf2176ac2da75258223f58d70303259e4
Submitter: Jenkins
Branch: master

commit a22aa08bf2176ac2da75258223f58d70303259e4
Author: Dolph Mathews <email address hidden>
Date: Mon Jan 12 17:55:16 2015 +0000

    Additional test coverage for password changes

    Keystone has four API calls which may result in a user's password
    changing.

    1. Administrative password reset on v2:

        POST /v2.0/users/{user_id}/OS-KSADM/password

    2. Self-service password change on v2:

        PATCH /v2.0/OS-KSCRUD/users/{user_id}

    3. Administrative password reset on v3:

        POST /v3/users/{user_id}

    4. Self-service password change on v3:

        POST /v3/users/{user_id}/password

    This patch adds additional test coverage to *consistently* ensure that:

    - Old passwords no longer work
    - Old tokens no longer work
    - The new password works

    Change-Id: I2a296b7ed407c75018fff3b60bd13aaa4fa9a849
    Closes-Bug: 1407105

[Vlad Okhrimenko (vokhrimenko)]

Thanks, Lin! :)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/146902

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/146908

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/146902
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=4cdd217e8d25d03a9ec85df2ff5e1e59341af42d
Submitter: Jenkins
Branch: stable/juno

commit 4cdd217e8d25d03a9ec85df2ff5e1e59341af42d
Author: Dolph Mathews <email address hidden>
Date: Mon Jan 5 22:42:30 2015 +0000

    Additional test coverage for password changes

    Keystone has four API calls which may result in a user's password
    changing.

    1. Administrative password reset on v2:

        POST /v2.0/users/{user_id}/OS-KSADM/password

    2. Self-service password change on v2:

        PATCH /v2.0/OS-KSCRUD/users/{user_id}

    3. Administrative password reset on v3:

        POST /v3/users/{user_id}

    4. Self-service password change on v3:

        POST /v3/users/{user_id}/password

    This patch adds additional test coverage to *consistently* ensure that:

    - Old passwords no longer work
    - Old tokens no longer work
    - The new password works

    Change-Id: I2a296b7ed407c75018fff3b60bd13aaa4fa9a849
    Closes-Bug: 1407105
    (cherry picked from commit a22aa08bf2176ac2da75258223f58d70303259e4)

[Timur Sufiev (tsufiev-x)]

Reassigning this to Paul Karikh since he is closer to our Keystone team and will be able to debug this with them.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/146908
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=096705800736bb6d2be39f1cb22ad69e18d4252a
Submitter: Jenkins
Branch: stable/icehouse

commit 096705800736bb6d2be39f1cb22ad69e18d4252a
Author: Dolph Mathews <email address hidden>
Date: Mon Jan 5 22:42:30 2015 +0000

    Additional test coverage for password changes

    Keystone has four API calls which may result in a user's password
    changing.

    1. Administrative password reset on v2:

        POST /v2.0/users/{user_id}/OS-KSADM/password

    2. Self-service password change on v2:

        PATCH /v2.0/OS-KSCRUD/users/{user_id}

    3. Administrative password reset on v3:

        POST /v3/users/{user_id}

    4. Self-service password change on v3:

        POST /v3/users/{user_id}/password

    This patch adds additional test coverage to *consistently* ensure that:

    - Old passwords no longer work
    - Old tokens no longer work
    - The new password works

    Change-Id: I2a296b7ed407c75018fff3b60bd13aaa4fa9a849
    Closes-Bug: 1407105
    (cherry picked from commit a22aa08bf2176ac2da75258223f58d70303259e4)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/142481
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=878c703fd006569219d3fc5be459f6ab76a48a15
Submitter: Jenkins
Branch: master

commit 878c703fd006569219d3fc5be459f6ab76a48a15
Author: Vlad Okhrimenko <email address hidden>
Date: Wed Dec 17 16:47:16 2014 +0200

    Logout user if he has no valid tokens

    Before this patch, if user's rights were changed
    or revoked - there would be "Unauthorized" errors
    on every page since user had no rights to view them
    because he had no valid tokens in that case.

    Now user will be logged out if he has no valid tokens.
    Set `escalate` to True (for unauthorized-error)
    to always log user out.

    Also, now horizon.exceptions.NotAuthorized is a part of
    UNAUTHORIZED tuple in the exceptions.py, because this type
    of exception is re-raised after handling services unauthorized errors.
    Looks like it was missing. Now the horizon.exceptions.NotAuthorized
    is handled like all NotAuthorized exceptions.

    And horizon_middleware.py in process_exception now generates
    logout_reason for cases if user is not authorized.

    Closes-Bug: #1252341
    Closes-Bug: #1407105
    Co-Authored-By: Paul Karikh <email address hidden>
    Change-Id: I417cad936ea80c0569c2f442fc87cbd58745757e

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/304504

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/304504
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=38dfe3d907c35dad82514005f9075c0fb1f57a2e
Submitter: Jenkins
Branch: stable/kilo

commit 38dfe3d907c35dad82514005f9075c0fb1f57a2e
Author: Vlad Okhrimenko <email address hidden>
Date: Wed Dec 17 16:47:16 2014 +0200

    Logout user if he has no valid tokens

    Before this patch, if user's rights were changed
    or revoked - there would be "Unauthorized" errors
    on every page since user had no rights to view them
    because he had no valid tokens in that case.

    Now user will be logged out if he has no valid tokens.
    Set `escalate` to True (for unauthorized-error)
    to always log user out.

    Also, now horizon.exceptions.NotAuthorized is a part of
    UNAUTHORIZED tuple in the exceptions.py, because this type
    of exception is re-raised after handling services unauthorized errors.
    Looks like it was missing. Now the horizon.exceptions.NotAuthorized
    is handled like all NotAuthorized exceptions.

    And horizon_middleware.py in process_exception now generates
    logout_reason for cases if user is not authorized.

    Conflicts:
     openstack_dashboard/dashboards/project/overview/tests.py

    Closes-Bug: #1528967
    Closes-Bug: #1252341
    Closes-Bug: #1407105
    Co-Authored-By: Paul Karikh <email address hidden>
    Change-Id: I417cad936ea80c0569c2f442fc87cbd58745757e
    (cherry picked from commit 878c703fd006569219d3fc5be459f6ab76a48a15)

[Doug Hellmann (doug-hellmann)]

This issue was fixed in the openstack/horizon 2015.1.4 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/horizon 2015.1.4 release.