Tenants can be created with invalid ids

[Bryan D. Payne]

When creating a new tenant, there is an optional argument 'id' that may be passed:

https://github.com/openstack/keystone/blob/9025b64a8f2bf5cf01a18453d6728e081bd2c3b9/keystone/assignment/controllers.py#L114

If not passed, this just creates a uuid and proceeds. If a value is passed, it will use that value. So a user with priv's to create a tenant can pass something like "../../../../../" as the id. If this is done, then the project can't be deleted without manually removing the value from the database. This can lead to a DoS that could fill the db and take down the cloud, in the worst of circumstances.

I believe the proper fix here would be to just remove this feature altogether. But this is because I'm not clear about why we would ever want to allow someone to set the id manually. If there's a valid use case here, then we should at least do some input validation.

[Jeremy Stanley (fungi)]

Looks like that line of code hasn't been touched since before Icehouse.

[Jeremy Stanley (fungi)]

Keystone devs: are you to the point with RBAC that it's expected admins might grant tenant creation rights to non-administrators? If so, this is likely something we need to embargo until fixed.

[Lance Bragstad (lbragstad)]

Hi Brian and Jeremy,

If we removed the functionality I'm not sure if we'd break existing deployments who use that feature but it's a possibility. Some sort of validation could be added and ported to previous releases. We do have input validation with jsonschema in v3, but I don't think we allow the user to specify the project['id'] in v3 (region['id']s can be specified however).

What about using something like shlex?

[Bryan D. Payne (bdpayne)]

shlex feels more expressive than we need here. Perhaps we should start by coming to common ground on this question: What are the valid inputs for this field? Is it just uuid style strings? Or would other things be valid too?

If we could limit it to uuid4 input, then the following could work:

try:
    valid_id = UUID(tenant_ref['id'], version=4)
except ValueError, KeyError:
    valid_id = uuid.uuid4().hex

[Dolph Mathews (dolph)]

While this isn't a documented feature in terms of examples, there is room in the v2 API spec to see this as a feature (the spec is not well defined). That said, the behavior has actually been around since Essex (so: forever):

  https://github.com/openstack/keystone/blob/308a766b5b8ddbfbaa549e3af7fa736c1e1d4218/keystone/identity/core.py#L120-L122

Given that it's been around for so long, someone is surely using it (and with input beyond just uuid-looking things, otherwise, why would you use it?), so I agree that dropping support is probably the wrong solution. Simplest solution would be to validate it against a configurable regex, similar to what we do for v3 in Juno (where we have a handful of intentionally user-definable IDs, such as region IDs), i.e. ([a-zA-z0-9-]){1,64}

[Morgan Fainberg (mdrnstm)]

I completely agree we cannot remove the feature. The solution proposed by Dolph is really quick to get rolled up into a patch set and makes sense to use as the fix as it is consistent with the other cases we allow user defined ids

[Lance Bragstad (lbragstad)]

Started working on a patch for this. Dolph, do you want the diff?

[Bryan D. Payne (bdpayne)]

A configurable regex sounds like a reasonable approach. I'd really like to be able to filter based on arbitrary python code (think flask converter), but if the framework isn't already in place for that, then it's certainly beyond scope for this fix.

[Dolph Mathews (dolph)]

Lance: let's catch up offline.

[Lance Bragstad (lbragstad)]

Bryan, we'll work with the regex path for now.

[Bryan D. Payne (bdpayne)]

Sounds good. Thanks for taking this on!

[Dolph Mathews (dolph)]

Sharing the solution discussed above based on regex, but we have LDAP tests that expect to pass unicode characters as the ID without encoding them. This patch is written to accommodate unicode (re.UNICODE is enabled), but I think those LDAP tests are wrong, so I'm now working on a second solution for comparison.

This patch applies to both master and proposed/juno.

[Bryan D. Payne (bdpayne)]

I agree that it would be best to exclude unicode. Otherwise, this looks ok to me from a security viewpoint. I'll leave it to keystone core to decide if the solution is generally correct for keystone.

[Lance Bragstad (lbragstad)]

Pulled this down and tested it manually and it works as expected. I also switched the regex to be more strict and that also works as expected. The help information for the configuration is nice and descriptive.

I'm good with this change.

+2

[Tristan Cacqueray (tristan-cacqueray)]

Any chance we can get this fix backported to stable/icehouse and the upcoming stable/juno ?
Quid of the stable backports that introduce a new configuration option with a sane default...

In the meantime, here is an impact description draft #1:

Title: Keystone allows creation of tenant with invalid ids
Reporter: Bryan D. Payne (Nebula)
Products: Keystone
Versions: up to 2014.2.1

Description:
Bryan D. Payne from Nebula reported a vulnerability in Keystone tenant creation. By creating malicious tenants with invalid id like "../../../../../" an authenticated user may corrupt keystone database resulting in new tenant being not removable, potentially leading to denial of service trough resources exhaustion. Note that by default only administrator have tenant creation permission, and the faulty tenants can still be deleted manually from the database. All Keystone setups are affected.

[Morgan Fainberg (mdrnstm)]

The code looks good to me in #12 (+2).

For the description I would suggest that we change "By creating malicious tenants with invalid id like "../../../../../" an authenticated user may corrupt keystone database resulting in new tenant being not removable, potentially leading to denial of service trough resources exhaustion."

to

"An authenticated user is able to create tenants with invalid ids like '../../../../../' that cannot be removed via the REST API potentially leading to a denial of service through resource exhaustion."

This isn't really a corruption of the DB, no data is lost directly via this issue (indirectly, MySQL could have serious issues, but that could occur with other cases of too much data on disk as well).

[Tristan Cacqueray (tristan-cacqueray)]

Thanks Morgan!

removing the corruption bit, here is an updated impact description #2

Title: Keystone allows creation of tenant with invalid ids
Reporter: Bryan D. Payne (Nebula)
Products: Keystone
Versions: up to 2014.2.1

Description:
Bryan D. Payne from Nebula reported a vulnerability in Keystone tenant creation. By creating malicious tenants with invalid id like "../../../../../" an authenticated user may fill the keystone database with tenants that cannot be removed via the REST API potentially leading to a denial of service through resource exhaustion. Note that by default only administrator have tenant creation permission, and the faulty tenants can still be deleted manually from the database. All Keystone setups are affected.

[Dolph Mathews (dolph)]

+1 for impact description in #17

[Dolph Mathews (dolph)]

This is my preferred solution. Instead of configurable regex, it uses urllib to verify that the input tenant ID is url-safe. There is no configuration impact.

It also changes two LDAP tests which were creating tenants with Unicode IDs to use pure ASCII (otherwise those tests would fail against this new validation).

This patch applies to master and proposed/juno.

[Bryan D. Payne (bdpayne)]

+1 for impact description in #17

[Dolph Mathews (dolph)]

Small revision to the patch that I had just posted in #19. This just re-uses an i18n error message. The rest of comment #19 still applies here.

[Tristan Cacqueray (tristan-cacqueray)]

@Dolph that surely sounds more simple and thus easy to backport!

 I can't find the patch, did you forget to attach it ?

[Bryan D. Payne (bdpayne)]

Dolph, I'm not seeing a link to your urllib based patch.

[Dolph Mathews (dolph)]

Tristan: Bryan: My apologies, I was hoping to post a quick revision before anyone noticed :) See comment #21.

[Lance Bragstad (lbragstad)]

+1 for the description provided in comment #17.

[Tristan Cacqueray (tristan-cacqueray)]

Oups, a tight comment race happened :-)

So, the patch seems to apply on stable/icehouse, but tests might need some rework to apply cleanly.

[Dolph Mathews (dolph)]

stable/icehouse backport of the urllib solution is attached.

[Morgan Fainberg (mdrnstm)]

I have confirmed the patch in #21 works as expected. The code looks good.

The description in #17 looks good except a small typo: "Note that by default only administrator have tenant creation permission" should either be: "Note that by default only administrators have tenant creation permission" (administrator should be plural).

I am ok with it as is without the typo change.

[Morgan Fainberg (mdrnstm)]

I am still reviewing the code in #28. At a glance it looks good.

[Lance Bragstad (lbragstad)]

The patch set from #27 works for me on stable/icehouse.

+2

[Morgan Fainberg (mdrnstm)]

The patch from #27 looks good to me.

[Grant Murphy (gmurphy)]

Before we move forward with getting a CVE assigned for this issue I'd like to revisit whether it really needs to be handled as a security flaw. I'm not sure where the trust boundary is being crossed here. The user must already be given access rights to create a tenant so aside from not being easily deleted how is this abuse case different from creating a bunch tenants with valid ID's?

I think Jeremy originally touched on this in comment #2 and I'm not sure that question was ever answered?

[Bryan D. Payne (bdpayne)]

This is a DoS and a consistency issue. The scope and impact are going to depend on how a given cloud is configured. But, from an operational context, it is typically harder to find someone that can manually manipulate the backend db than it is to find someone that can drive the OpenStack CLI tools. And I think that gives this particular issue a higher impact.

[Thierry Carrez (ttx)]

From an attacker perspective, triggering a DoS by creating millions of tenants is the same attack, whether this bug is fixed or not. That's what I cause "normal usage" DoS -- there is no amplification here that would suddenly make it more viable than normal usage, and the "DoS" is undistinguishable from a peak of activity. The fact that you can't delete the newly-created tenants using admin CLI is a bug, it doesn't make "DoS by creating a bunch of tenants" more (or less) efficient.

So I don't think this qualifies as a vulnerability.

[Dolph Mathews (dolph)]

From a security standpoint, I don't see this as being a significant risk.

[Morgan Fainberg (mdrnstm)]

I agree with Dolph. This is low risk. I do think it'll provide significant annoyance, but likely not much else, to a deployer to cleanup after in the worst case (as outlined by Thierry).

[Bryan D. Payne (bdpayne)]

I'm ok making this public. My primary concern at this point is seeing this bug fixed. I agree that this is borderline based on the definition of vulnerability typically used by the VMT.

[Morgan Fainberg (mdrnstm)]

With V2 slowly disappearing, i am marking this as Wont Fix.