[OSSA 2014-029] Catalog replacement allows reading config (CVE-2014-3621)

[Brant Knudson]

Anyone that can create endpoints can read any value out of the config file. Some of the values in the config file are passwords and things that shouldn't be made available.

For example, I'm running and cloud and I allow someone to define their own endpoints for whatever reason... maybe they control their own subcloud. If the admin creates an endpoint with $(admin_token), then queries the catalog, they can read the admin token out of the config file.

I don't know what the fix is. Maybe a whitelist of config options? Maybe leaving "secret=True" config options out of the dict is good enough?

Here's steps to recreate:

$ openstack service create --type test test
+-------------+----------------------------------+
| Field | Value |
+-------------+----------------------------------+
| description | None |
| enabled | True |
| id | 9251fab0a273454d84046c810b3503a7 |
| name | test |
| type | test |
+-------------+----------------------------------+

# Define an endpoint with $(admin_token)s.

$ openstack endpoint create 9251fab0a273454d84046c810b3503a7 --publicurl 'http://something/$(admin_token)s'
+--------------+----------------------------------+
| Field | Value |
+--------------+----------------------------------+
| id | 164abc0b305e4ad998ac732cda569cd6 |
| publicurl | http://something/$(admin_token)s |
| region | None |
| service_id | 9251fab0a273454d84046c810b3503a7 |
| service_name | test |
| service_type | test |
+--------------+----------------------------------+

# Get a scoped token using curl.

$ curl -i -H "Content-Type: application/json" -d '
{ "auth": {
    "identity": {
      "methods": ["password"],
      "password": {
        "user": {
          "name": "admin",
          "domain": { "id": "default" },
          "password": "mypassword"
        }
      }
    },
    "scope": {
      "project": {
        "name": "demo",
        "domain": { "id": "default" }
      }
    }
  }
}' http://localhost:35357/v3/auth/tokens ; echo

... Look for the admin token in the catalog in the response:
{"endpoints": [{"url": "http://something/myadmintoken",
...

[David Stanek (dstanek)]

I like whitelists because we can carefully select what can be used and it's much harder to accidentally have bad things fall through. Also there are things like IPs, ports, usernames, etc. not marked as secret that can used used in part as a social engineering attack.

On the other hand this could be very tedious to manage, but I can't imagine there is more than a handful of things that could be used in endpoints.

[Dolph Mathews (dolph)]

This has always seemed like a dumb feature to me - I'd love to simply drop it if no one is seriously using it, but that's obviously not a backportable solution. Maybe we can deprecate it for Juno, and introduce the whitelist as the simplest possible backportable solution?

[Thierry Carrez (ttx)]

It's a difficult fix, since it's one of those cases where a feature has corner case security consequences. You can't really remove the "feature" in stable release, since that would be changing behavior that people might rely on. So my suggestion would be to drop the "feature" in future versions, and document the corner case security issue ("don't let anyone create endpoints!") in a OSSN...

Adding keystone-coresec and ossg-coresec for more input on this.

[Nathan Kinder (nkinder)]

The difficulty here is that we don't know how frequently this replacement/interpolation functionality is used, or how exactly it is being used.

For existing stable releases, we can certainly recommend that you don't delegate the ability to create endpoints to anyone other than the system admin. The problem is that some deployments may truly need to allow delegation of endpoint creation. In these cases, there is really no secure option to allow delegation of endpoint creation. Allowing a whitelist, or even having the ability to turn off the replacement/interpolation would at least give us a secure option for stable releases. These approaches are new configuration though, which is something we try to avoid for stable releases. It also requires getting the word out that configuration changes need to be made. It feels like we should do something for stable releases here.

[Morgan Fainberg (mdrnstm)]

We have moved away from the "nova compute" string substitution part of this issue but we do still use it for keystone ports iirc.

I think we could safely (with documentation) drop or greatly limit this feature (I agree it's dumb) in Juno. OSSN for stable/* releases.

We could *possibly* also check the option secret flag before trying to do the substitution (which might be backportable).

[Bryan D. Payne (bdpayne)]

+1 for dropping the feature in Juno.

+1 for doing a OSSA with a whitelisting fix for current stable releases.

I agree with Nathan that it is unacceptable to simply tell people to not use this feature as that will put some installations in a bind where they simply can't resolve the issue. A security fix is really necessary here.

[Jeremy Stanley (fungi)]

I suppose whether we release an advisory depends on whether or not anyone does the work to propose backportable fixes to supported releases. I agree that if we do patch/work around this in stable for safety reasons, then an OSSA is warranted. In the meantime hopefully someone is making sure it's properly ripped out of master so we don't have to carry a workaround past Icehouse.

[Bryan D. Payne (bdpayne)]

Aren't OSSA-level issues seen as an important priority with PTLs? It seems to me that OpenStack has matured to the point where we need to fix security problems. Simply hoping that someone will volunteer to backport something seems wrong to me. What do we need to do to improve this state of affairs?

[Dolph Mathews (dolph)]

Yes, they are an important priority.

[Dolph Mathews (dolph)]

David Stanek is on vacation today, but sent me this master patch for review. This provides a whitelist configuration option with a reasonable default whitelist to support safe use cases.

It feels weird to review a patch that I'm also uploading, but +2

[Nathan Kinder (nkinder)]

The patch looks good from a security perspective, so +1.

A minor thing that should probably be addressed is to protect against the whitelist being None. In WhitelistedFormatter:__init__(), the set() function will fail if whitelist is None. This would require one to explicitly set the config to None instead of an empty list, but it's possible and we should protect against it. Here is the code I'm referring to:

+class WhiteListedFormatter(object):
+
+ def __init__(self, whitelist, data):
+ self._whitelist = set(whitelist)

[Dolph Mathews (dolph)]

In backporting the above patch to stable/icehouse, I also noticed that compute_port should also be added to the default whitelist.

[Dolph Mathews (dolph)]

Addressed the issues identified in comments #11 and #12 in the above patches.

[Nathan Kinder (nkinder)]

The patches look good to me. Thanks for addressing my comment!

[Adam Young (ayoung)]

patches looks good

[Tristan Cacqueray (tristan-cacqueray)]

Thanks you folks for fixing this in stable too. Even if the stable patches introduce a new config option, considering the sane default I think we could now confirm the OSSA task.

@VMT thought ?

[Brant Knudson (blk-u)]

master patch in comment #13:

- tox -e py27,pep8
  - passed

- visual inspection

I'd prefer it if tenant_id and user_id weren't in the whitelist config option but were always allowed. These are values set by keystone and not from the config file, so the help text is kind of confusing (it says the values are in the config file) and a user might think they can remove those values, but it's probably going to break just about every deployment if they do.

Some minor suggestions on the patches:
1) It would be nice if the list in the default value for endpoint_substitution_whitelist was alphabetized.
2) The help text for the option should say that it's deprecated since config replacement will be removed in Juno.
3) I'd assume that WhiteListedFormatter does formatting, but it doesn't, so rename to FilterWhiteListedItems or something.
4) The sample config file should be updated.

- test
  - passed, no endpoint with admin token.

icehouse patch in comment #16:

- tox -e py27,pep8
  - I had to add oslo.utils to my venv for this to work... maybe we have to wait for a change to stable/icehouse for this?

- check backport
  - looks good.

- test
  - passed... get a 501 error now.

havana patch in comment #17:

- tox -e py27,pep8
  - fails, right at the beginning: None ERROR 0.00

======================================================================
ERROR: Failure: ImportError (cannot import name fixture)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/stack/keystone/.tox/py27/local/lib/python2.7/site-packages/nose/loader.py", line 414, in loadTestsFromName
    addr.filename, addr.module)
  File "/opt/stack/keystone/.tox/py27/local/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/opt/stack/keystone/.tox/py27/local/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/opt/stack/keystone/keystone/tests/unit/catalog/test_core.py", line 13, in <module>
    from oslo.config import fixture as config_fixture
ImportError: cannot import name fixture

-- this change (and the icehouse one?) should use keystone/openstack/common/fixture/config.py instead.

- check backport (generally looks correct, but the tests failed so didn't look too closely).

- test
  - passed, get a 501 now

[David Stanek (dstanek)]

master

[David Stanek (dstanek)]

I've uploaded new patches that address Brant's comments. All three branches have slightly different ways to handle tests so each has a slightly different test setup.

[Brant Knudson (blk-u)]

Tried out the patches in 23-25 and they all worked for me.

master:

- tox py27,pep8
-- passed

- visual inspection

I'd prefer it if tenant_id and user_id weren't in the whitelist config option but were always allowed. These are values set by keystone and not from the config file, so the help text is kind of confusing (it says the values are in the config file) and a user might think they can remove those values, but it's probably going to break just about every deployment if they do.

I'm also fine with the change as is.

- try it
-- Worked... no endpoint.

stable/icehouse:

- tox py27,pep8
-- passed

- visual inspection
-- LGTM

- try it
-- worked, got 500 error

stable/havana:

- tox py27,pep8

- visual inspection
-- LGTM

- try it
-- 500 error

[Morgan Fainberg (mdrnstm)]

The patches in 23-25 LGTM.

[Tristan Cacqueray (tristan-cacqueray)]

Impact description draft #1:

Title: Configuration option leak through Keystone catalog
Reporter: Brant Knudson (IBM)
Products: Keystone
Versions: up to 2013.2.3 and 2014.1 versions up to 2014.1.2

Description:
Brant Knudson from IBM reported a vulnerability in Keystone catalog url replacement. By creating a malicious endpoint a privileged user may reveal configuration options resulting in sensitive information, like master admin_token, being exposed through service url. All Keystone setups that allow non-admin user to create endpoint are affected.

[Brant Knudson (blk-u)]

a couple of nits:

change "exposed through service url" to "exposed through the service url" (add the)

change "to create endpoint are" to "to create endpoints are" (add s)

otherwise looks good.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks!

Impact description draft #2:

Title: Configuration option leak through Keystone catalog
Reporter: Brant Knudson (IBM)
Products: Keystone
Versions: up to 2013.2.3 and 2014.1 versions up to 2014.1.2

Description:
Brant Knudson from IBM reported a vulnerability in Keystone catalog url replacement. By creating a malicious endpoint a privileged user may reveal configuration options resulting in sensitive information, like master admin_token, being exposed through the service url. All Keystone setups that allow non-admin user to create endpoints are affected.

[Brant Knudson (blk-u)]

Another nit I should have mentioned before:

change "allow non-admin user to" to "allow non-admin users to" (add s), or "allow a non-admin user to" (add a), either one works for me.

The rest works looks good to me.

[Tristan Cacqueray (tristan-cacqueray)]

Thanks!

CVE requested with this impact description:

Title: Configuration option leak through Keystone catalog
Reporter: Brant Knudson (IBM)
Products: Keystone
Versions: up to 2013.2.3 and 2014.1 versions up to 2014.1.2

Description:
Brant Knudson from IBM reported a vulnerability in Keystone catalog url
replacement. By creating a malicious endpoint a privileged user may
reveal configuration options resulting in sensitive information, like
master admin_token, being exposed through the service url. All Keystone
setups that allow non-admin users to create endpoints are affected.

[Tristan Cacqueray (tristan-cacqueray)]

The stable/icehouse patch does not apply anymore. @dstanek can you please provide an updated version ?

The pre-OSSA will be sent after with a disclosure date set to:
2014-09-16, 1500UTC

Thanks!

[David Stanek (dstanek)]

The patch didn't apply cleanly because of some test issues.

[Brant Knudson (blk-u)]

I ran the tests on the file in comment 35 and they passed. visual inspection looks ok to me.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/121891

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/121890
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=878f12e16017cf328bc91fe75b5db680e37df583
Submitter: Jenkins
Branch: stable/icehouse

commit 878f12e16017cf328bc91fe75b5db680e37df583
Author: David Stanek <email address hidden>
Date: Fri Aug 15 11:57:07 2014 -0500

    Adds a whitelist for endpoint catalog substitution

    Change-Id: If02327d70d0143d805969fe927898f08eb84c4c2
    Closes-Bug: #1354208

[OpenStack Infra (hudson-openstack)]

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

commit 2989ff257e4fde6a168e25b926805e700406aa80
Author: David Stanek <email address hidden>
Date: Fri Aug 15 14:59:48 2014 +0000

    Adds a whitelist for endpoint catalog substitution

    Change-Id: If02327d70d0143d805969fe927898f08eb84c4c2
    Closes-Bug: #1354208

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/121891
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=52714633c9a4dae5e60279217090859aa6dbcb4f
Submitter: Jenkins
Branch: stable/havana

commit 52714633c9a4dae5e60279217090859aa6dbcb4f
Author: David Stanek <email address hidden>
Date: Fri Aug 15 13:30:33 2014 -0500

    Adds a whitelist for endpoint catalog substitution

    Change-Id: If02327d70d0143d805969fe927898f08eb84c4c2
    Closes-Bug: #1354208

[Matthew Thode (prometheanfire)]

do we have a patchset that works against 2014.1.2.1? the stable/icehose commit doesn't apply cleanly.

[David Stanek (dstanek)]

@Matthew, It likely doesn't apply cleanly because it builds on other changes. Do we need to come up with another patch for 2014.1.2.1 that doesn't include the other changes?

[Dolph Mathews (dolph)]

The patch for stable/icehouse (2014.1) merged with https://review.openstack.org/#/c/121890/ -- we shouldn't have any work remaining on that branch (?).

[Ray C Horn (raychorn)]

I need the source files for the patch that fixes this issue so I can manually apply the patch to the following environment:

ii nova-api-ec2 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - EC2 API frontend
ii nova-api-metadata 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - metadata API frontend
ii nova-api-os-compute 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - OpenStack Compute API frontend
ii nova-cert 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - certificate management
ii nova-common 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - common files
ii nova-conductor 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - conductor service
ii nova-consoleauth 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - Console Authenticator
ii nova-novncproxy 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - NoVNC proxy
ii nova-scheduler 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute - virtual machine scheduler
ii python-nova 1:2013.2.3-0ubuntu1~cloud0 OpenStack Compute Python libraries
ii python-novaclient 1:2.15.0-0ubuntu1~cloud0 client library for OpenStack Compute API
________________________________________

Thanks.

[Dolph Mathews (dolph)]

Ray: patches for havana are linked above as patch files, gerrit reviews, and cgit commits.