[OSSA 2013-003] unauthenticated POST to /tokens can fill up disk/logs

Keystone-core, please review proposed patches (only comment on the private bug please).
@Dan: would that also affect Essex ?

@ttx: yes. I think it would. I can also look at backporting it there...

----

Also, in addition to these patches I would like to go ahead and put in a 'size_limit' middleware (same thing we did in Nova) to guard against really large requests in Keystone in general. Any issues with me pushing that patch separately upstream (outside of the scope of thise security issue)?

Essex patch would be cool. You can go ahead with the middleware approach, just don't mention this particular bug.

@Keystone core, please review all proposed patches

Proposed impact description:

Title: Keystone denial of service through invalid token requests
Reporter: Dan Prince (Red Hat)
Products: Keystone
Affects: All versions

Description:
Dan Prince of Red Hat reported a vulnerability in token creation error handling in Keystone. By requesting lots of invalid tokens, an unauthenticated user may fill up logs on Keystone API servers disks, potentially resulting in a denial of service attack against Keystone.

Patch content looks good, but grizzly appears to have a merge conflict on master.

    error: patch failed: keystone/exception.py:73
    error: keystone/exception.py: patch does not apply

Refreshed grizzly patch

Thanks for refreshing the patch ttx.

If other things slip in I'll rebase and make sure these patch cleanly before I push them to Gerrit.

Remaining tasks before disclosure:
Keystone core: please review proposed patches and approve them on the bug
Anyone: plese review proposed description from comment 8

My immediate concern on reviewing the patches is that they are neither necessary nor sufficient to address the issue: Instead of looking at the token size, we should look at the overall request size, as you can stuff any cookie to blow out the memory. The Token should not be getting logged anywhere, so it should not be an issue in filling up logs.

ayoung:

Regarding your concern about request sizes my aim is to address that here: https://review.openstack.org/#/c/19567/

---

To your other point the fact that keystone isn't checking these token sizes up front is arguably sloppy and is in fact directly related to this specific issue which is caused by letting allowing really large tokens to go all the way to the database layer. I guess my take is that validating the size of an input parameter (a token) is something we should be doing regardless of whether or not we have a generic request size limiter in place.

Right. If https://review.openstack.org/#/c/19567/ is not sufficeint, then juchecking Token size won't be,either, as any of the IDs probably can be exploded this way. If I understand the underlying problem, it is that we are logging the whole URL, and the Token ID is in the URL body.

Thus, the fix we need for this is limiting the size of the URL. not justthe size of the token.

I would actually be OK with limiting token size even further. We currently allow a validation of an entire token, with an optimization that we will validate a Hash of the token as the ID. This is actually the PKEY of the token in the database. We can shorten token length for validation to the length of the Hash.

ayoung:

what specifically don't you like about these patches? All I'm doing is:

    Add size validations to token controller.

    Updates token controller so that it explicitly checks the max
    size of userId, username, tenantId, tenantname, token, and password
    before continuing with a request.

I don't see what is so bad about this? I've tested it manually and it certainly fixes the issue I describe up top.

---

Also, the request to /tokens we are talking about in this ticket is a POST where the body is really large. (not the URL).

I suspect Adam means that the fix is correct, but does not fully address the class of issues you uncovered, in particular we are still logging the full URL which may be stuffed outside of the userId/username/tenantId/tenantname/token/password parameters.

Since the new size-limiting middleware would only go to Grizzly, if we are indeed logging the full URL somewhere it would probably be a good move to check the size of it in the security fix too (or log only the first n characters).

ayoung/ttx:

Where are we logging request URL's (with verbose/debug logging disabled)?

Also, AFAIK eventlet defaults are going to cover us in terms of limiting both the request URL size and the max size of HTTP headers.

Looking at the source for Eventlets wsgi.py shows:

 url_length_limit=MAX_REQUEST_LINE (which is set to 8192 by default...)

 MAX_HEADER_LINE = 8192

 MAX_TOTAL_HEADER_SIZE = 65536

----

Perhaps I've misunderstood something but if this is in fact what you are concerned about it would appear that eventlet's WSGI defaults cover us here.

----

The issue I'm talking about in this ticket is only caused by a really large request body (100 MB+) where the large data inputs go all the way to the keystone backends and only then get logged to disks.

And by doing that I am able to increase the log file size by 100MB per request at a production logging level. At that rate it isn't going to take that many requests for me to fill up a disk.

@dprince: Reading the bug again, I caught that you were reproducing this with default log levels (verbose & debug = False) -- can you post your keystone.conf and logging.conf?

@dolph

See the attached keystone.conf and logging.conf above.

Also, If you are interested here is the first snippet of the 96M log message (a single line) after performing the request above:

2013-01-22 11:53:32 ERROR [root] (OperationalError) (2006, 'MySQL server has gone away') 'SELECT user.id AS user_id, user.name AS user_name, user.password AS user_password, user.enabled AS user_enabled, user.extra AS user_extra \nFROM user \nWHERE user.name = %s \n LIMIT %s' ('000000000000000000000000000000<snip>

The gist of this patch is that we check value lengths up front in the API layer before passing into the various backends (SQL, LDAP, etc.)

Thanks; the ERROR log explains why I wasn't able to reproduce with default settings.

The attack listed above has a couple assumptions:

1. The attack is against an URL that will process unauthenticated requests
2. Limiting the size of values that are sent to the database will prevent them from being logged upon failure.

This patch was just approved for master

https://review.openstack.org/#/c/19567/

It does the overall size limiting of the request header. It defends against the attack above and a wider array of attacks: any place where overflowing a field can cause an error.

That patch should be backported to Folsom and Essex as it defends against a wider range of attacks.

The above patch has a few issues with it.

 The max values are coded in python. They should be config file values, so they can be tuned in a deployment.

It limits the size of the tokens, which is probably OK, but could be an issue for very large tokens in a limited usage scenario. I would suggest instead that we limit the size of the value that is sent to the backend for looking up a token instead. This number can be much smaller: the length of a UUID/SHA1 hash, and can likely be MAX_PARAM_SIZE, not 8K.

My recommendation is that we focus on getting review 19567 backported, and then apply the above patches as good programming practices, but not as a response to a CVE.

The only problem with backporting https://review.openstack.org/#/c/19567/ is that it requires people to revise their pipelines in order to deploy the new middleware. The patch here definitely fixes a relatively narrow vulnerability specifically exposed on :5000.

+1 for moving MAX_PARAM_SIZE, etc, into keystone.conf; the appropriate values will vary among deployments and need to be controlled by the end user.

Dolphm: I think you are spot on. Obviously both of these patches are important... but I do feel like the immediate exploit might be better handled by this patch which takes a more fined grained approach.

Both this patch and what I did in https://review.openstack.org/#/c/19567/ prevent the issue however. I did 19567 as a catch all to prevent other things we might not know about... not necessarily to supplant the checks here.

-----

I'll look into making MAX_PARAM_SIZE a config variable. I suppose I based this roughly on the max size of the SQL backend (which to me seemed like a reasonable default).

ayoung: I do think the MAX_TOKEN_SIZE is going to need to be larged than MAX_PARAM_SIZE so it warrants either an extra config or global.

The new middleware approach is not acceptable for stable backport, it's basically a new feature. We need to address specific known cases for the security backports, so we need separate patch(es).

Okay. See the latest patches which make a config variable for max_param_size and max_token_size.

Note that in Essex I made the max_token_size match the max_param_size since there was no PKI support at that release.

It's basically inconsequential, but in the essex backport for TokenController, there's a redundant three lines validating:

    len(tenant_id) > CONF.max_param_size

Also, there's some trivial refactoring (pulling CONF into the local namespace) in the grizzly patch that you backported to folsom. +1 for the refactoring, but I'm not sure it should be included in the backport?

Grizzly patch looks good. I'll defer to Dolph on the others regarding the refactoring, but they look good to me.

+1 on the Grizzly patch.

Henry
On 23 Jan 2013, at 17:52, Adam Young wrote:

> Grizzly patch looks good. I'll defer to Dolph on the others regarding
> the refactoring, but they look good to me.
>
> --
> 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/1098307
>
> Title:
> unauthenticated POST to /tokens can fill up disk/logs
>
> Status in OpenStack Identity (Keystone):
> In Progress
> Status in Keystone folsom series:
> In Progress
>
> Bug description:
> A remote unauthenticated keystone user could potentially fill up the
> disk on a Keystone server by running the following python script:
>
> -----------------------
> from keystoneclient.v2_0 import client
>
> PASSWORD='foobar'
> TENANT='blah'
> USER = '00000' * 9999999
>
> keystone = client.Client(username=USER,
> password=PASSWORD,
> tenant_name=TENANT,
> auth_url='http://localhost:5000/v2.0')
>
> -----------
>
> Running this script will increase the log file size by 100 MB per
> request. NOTE: This happens when running keystone at the default log
> levels:
>
> # verbose = False
> # debug = False
>
>
> Version-Release number of selected component (if applicable):
>
> openstack-keystone-2012.2.1-1.el6ost.noarch (Red Hat)
>
> How reproducible:
>
> *always*
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/keystone/+bug/1098307/+subscriptions
>

ETOOMANYPATCHES!

Okay. The latest essex and folsom patches should fix up the issues dolph noticed. I think we are all set now? :)

+1!

+1!

Refreshed grizzly patch

Are the config.CONF.signing -> CONF.signing changes in the grizzly patch actually necessary for the fix ? Minimal fix is preferred for security patches.

About to push patchsets to downstream stakeholders with following impact description:
----------
Title: Keystone denial of service through invalid token requests
Reporter: Dan Prince (Red Hat)
Products: Keystone
Affects: All versions

Description:
Dan Prince of Red Hat reported a vulnerability in token creation error handling in Keystone. By requesting lots of invalid tokens, an unauthenticated user may fill up logs on Keystone API servers disks, potentially resulting in a denial of service attack against Keystone.
----------

please +1 this impact description

+1 for impact description

+1 on the impact description

Sent to downstream stakeholders. Proposed public disclosure date/time:
*Tuesday February 5th, 1500UTC*

Thanks for the patches. Testing here so far indicates that patches are good. Small nit: the folsom and grizzly patches have a misspelled word:
--- a/keystone/config.py
+++ b/keystone/config.py
@@ -141,6 +141,9 @@ register_str('policy_file', default='policy.json')
 register_str('policy_default_rule', default=None)
 #default max request size is 112k
 register_int('max_request_body_size', default=114688)
+register_int('max_param_size', default=64)
+# we allow tokens to be a bit larger to accomidate PKI
+register_int('max_token_size', default=8192)

'accomidate' should be 'accommodate'.

Jamie: Thanks. I got the same feedback from our packaging guys. Since we didn't catch the misspelling during review I think we should just go with it for now. We can fix it upstream after the fact...

+1 from stable-maint on the stable/folsom patch

Proposed as:
grizzly: https://review.openstack.org/#/c/21213/
folsom: https://review.openstack.org/#/c/21215/
essex: https://review.openstack.org/#/c/21216/

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