[OSSA 2013-005] EC2 authentication does not ensure user or tenant is enabled

Awesome !
Adding Keystone core for patch review.

Keystone core, please confirm issue and review proposed patches. Please do not communicate about this issue or patches outside of this bug until the end of the security embargo.

Nit on the grizzly patch: change the reference to "token.core.validate_auth_info" to just "token.validate_auth_info". The "core" module is only referenced directly from within the token package itself.

Although the grizzly patch is primarily a refactor, I'd be happy to argue that it's totally appropriate here as a security fix: the security vulnerability is ultimately that two code paths are not sharing code, when they should be.

On the folsom and essex patches, remove the msg from being passed to Unauthorized (only use it for logging), as you're actually creating a new security vulnerability there which we are protected from in Grizzly (unnecessarily exposing the reason for auth failure to the API). In Grizzly, the msg passed to exceptions.Unauthorized() is exposed to the API in debug mode, but suppressed otherwise. We don't have that feature in essex/folsom, so we can't allow the exception's message to be overridden.

I can update the patches and post them to this bug later this evening. For stable/essex and stable/folsom, it looks like I should also fix the message being passed to exception.Unauthorized in the role check:

        if not roles:
            raise exception.Unauthorized(message='User not valid for tenant.')

Hrm, I'd support that, but it's up to ttx. In this specific example, you could make the same call without a tenant to verify that authentication is succeeding, but then the only possibility for raising a 401 is that you're not authorized on the specified tenant... so I don't see any unnecessary exposure. However, I'd +1 for correcting it as part of this patch (I'd rather be on the safe side); it's not an issue worth distinctly tracking, IMO. Here's the line referenced above:

  https://github.com/openstack/keystone/blob/stable/essex/keystone/contrib/ec2/core.py#L173

Looking through the same code, there's two other 401's that should really be 400's. There's also this:

  https://github.com/openstack/keystone/blob/stable/essex/keystone/contrib/ec2/core.py#L115

... which I'm not sure represents an exposure or not. I'm guessing not, because either the auth will succeed or fail there without any granularity.

I attached three new patch files for grizzly, stable/folsom, and stable/essex. I updated two 401s (Unauthorized) to 400s (ValidationError). I also removed the messages being passed to the exceptions in folsom and essex based on your feedback.

+1 all around, except I'd rather not change the status codes from 401 -> 400 in essex & folsom, even though 400 is obviously more appropriate. I'd prefer to change it in grizzly, but perhaps that change would be more appropriate as a public followup to this security fix. I'm happy to see the message argument removed from those exceptions, however.

I imagine the Grizzly patch will also conflict with https://review.openstack.org/#/c/21487/

@gyee, can you ensure that your approach is compatible with the refactoring here / reusable by ec2, without including the security fix in your patch?

Uploaded new patches. Removed 401 -> 400 changes in grizzly patch, will submit in future public fix. Removed 401 -> 400 changes from stable patches. Removed message text from being passed to the Unauthorized exceptions in stable patches.

@Dolph, is there anything you want me to do regarding the code in review that may conflict? Or wait for this security fix to land and then have @gyee rebase his code?

+1 for the patches as-is.

I just want to ensure that you and gyee are on the same page for the grizzly patch, so that both patches can be merged in either order without issue.

I'll need another +1 on that. FWIW the security patch will probably get merged after gyee's patch, due to the disclosure delay... so assuming that one will and first is probably good.

Working on impact description now.

Proposed description:

----------------
Title: Keystone EC2-style authentication accepts disabled user/tenants
Reporter: $CREDIT
Products: Keystone
Affects: All versions

Description:
$CREDIT reported a vulnerability in EC2-style authentication in Keystone. Keystone fails to check whether a user, tenant, or domain is enabled before authenticating a user using the EC2 api. Authenticated, but disabled users (or authenticated users in disabled tenants or domains) could therefore retain access rights that were thought removed. Only setups enabling EC2-style authentication are affected.
---------------

@mathrock: what do you want to appear in $CREDIT ?
@Keystone-core: Is it even possible to disable EC2-style auth to workaround this issue? If yes, how?

<dolphm> to disable, remove ec2 extension from your pipeline: keystone.contrib.ec2:Ec2Extension.factory

CREDIT='Nathanael Burton (National Security Agency)'

+1 from me on the patches.

Henry
On 13 Feb 2013, at 00:09, mathrock wrote:

> CREDIT='Nathanael Burton (National Security Agency)'
>
> --
> You received this bug notification because you are a member of Keystone
> Core Developers, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1121494
>
> Title:
> EC2 authentication does not ensure user or tenant is enabled
>
> Status in OpenStack Identity (Keystone):
> Confirmed
>
> Bug description:
> Keystone does not check whether a user, tenant, or domain is enabled
> before authenticating a user using the EC2 api. I've attached three
> patches based on Grizzly (master), stable/folsom, and stable/essex.
> For the Grizzly patch, I've refactored the code to ensure the same
> checks used in token-based auth are checked when using EC2 signature-
> based auth.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/keystone/+bug/1121494/+subscriptions
>

+1 for impact description

Updated description:

--------------------------------------

Title: Keystone EC2-style authentication accepts disabled user/tenants
Reporter: Nathanael Burton (National Security Agency)
Products: Keystone
Affects: All versions

Description:
Nathanael Burton reported a vulnerability in EC2-style authentication in Keystone. Keystone fails to check whether a user, tenant, or domain is enabled before authenticating a user using the EC2 api. Authenticated, but disabled users (or authenticated users in disabled tenants or domains) could therefore retain access rights that were thought removed. Only setups enabling EC2-style authentication are affected. To disable EC2-style authentication to work around the issue, remove the ec2_extension from the keystone API pipeline in keystone.conf.

-----------------------------------------

Proposed release date: Tuesday, February 19 at 1500 UTC

On the patches, what name/email should be used as the author of the code when the time comes to merge them? If you want, you have this pre-set by exporting them from your local git tree using "git format-patch". This format will preserve the commit message, as well. Otherwise we can fill it in.

Updated description, changing the way we refer to the EC2 extension:

--------------------------------------

Title: Keystone EC2-style authentication accepts disabled user/tenants
Reporter: Nathanael Burton (National Security Agency)
Products: Keystone
Affects: All versions

Description:
Nathanael Burton reported a vulnerability in EC2-style authentication in Keystone. Keystone fails to check whether a user, tenant, or domain is enabled before authenticating a user using the EC2 api. Authenticated, but disabled users (or authenticated users in disabled tenants or domains) could therefore retain access rights that were thought removed. Only setups enabling EC2-style authentication are affected. To disable EC2-style authentication to work around the issue, remove the EC2 extension (keystone.contrib.ec2:Ec2Extension.factory) from the keystone API pipeline in keystone.conf.

-----------------------------------------

Proposed release date: Tuesday, February 19 at 1500 UTC

+1 for latest impact description

+1 for the description

The early notification has been sent out to downstream stakeholders.

@Russell, Nathanael Burton (<email address hidden>) works for me. I will make sure to use "git format-patch" in the future.

@Russell, actually my name/email known by gerrit is "mathrock <email address hidden>", but whatever works is fine by me.

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

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

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

Reviewed: https://review.openstack.org/22319
Committed: http://github.com/openstack/keystone/commit/9572bfc393f66f5ce3b44c0a77a9e29cc0374c6f
Submitter: Jenkins
Branch: master

commit 9572bfc393f66f5ce3b44c0a77a9e29cc0374c6f
Author: Nathanael Burton <email address hidden>
Date: Tue Feb 19 10:15:24 2013 -0600

    Ensure user and tenant enabled in EC2

    Fixes bug 1121494.

    Change-Id: If7277f0b4a55aa5be81b354cd4c7ed338a600a62

Reviewed: https://review.openstack.org/22320
Committed: http://github.com/openstack/keystone/commit/f0b4d300db5cc61d4f079f8bce9da8e8bea1081a
Submitter: Jenkins
Branch: stable/folsom

commit f0b4d300db5cc61d4f079f8bce9da8e8bea1081a
Author: Nathanael Burton <email address hidden>
Date: Tue Feb 19 09:27:04 2013 -0600

    Ensure user and tenant enabled in EC2

    Fixes bug 1121494.

    Change-Id: Icc90d581691b5aa63754e076ce983dfa2885a1dc

Reviewed: https://review.openstack.org/22321
Committed: http://github.com/openstack/keystone/commit/7402f5ef994599653bdbb3ed5ff1a2b8c3e72b9f
Submitter: Jenkins
Branch: stable/essex

commit 7402f5ef994599653bdbb3ed5ff1a2b8c3e72b9f
Author: Nathanael Burton <email address hidden>
Date: Tue Feb 19 09:27:54 2013 -0600

    Ensure user and tenant enabled in EC2

    Fixes bug 1121494.

    Change-Id: Ia9a149a93dec87f46b667100e0ee2eda56d9489d

Looks like status was missed. Merged properly etc, old bug cleanup