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

Bug #1354208 reported by Brant Knudson
260
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Medium
Tristan Cacqueray
Havana
Medium
Tristan Cacqueray
Icehouse
Medium
Tristan Cacqueray
OpenStack Security Advisory
Medium
Tristan Cacqueray

Bug Description

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",
...

CVE References

Revision history for this message
David Stanek (dstanek) wrote :

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.

Revision history for this message
Dolph Mathews (dolph) wrote :

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?

Changed in keystone:
status: New → Triaged
importance: Undecided → Medium
Revision history for this message
Thierry Carrez (ttx) wrote :

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.

Changed in ossa:
status: New → Incomplete
Revision history for this message
Nathan Kinder (nkinder) wrote :

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.

Revision history for this message
Morgan Fainberg (mdrnstm) wrote :

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).

Revision history for this message
Bryan D. Payne (bdpayne) wrote :

+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.

Revision history for this message
Jeremy Stanley (fungi) wrote :

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.

Revision history for this message
Bryan D. Payne (bdpayne) wrote :

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?

Revision history for this message
Dolph Mathews (dolph) wrote :

Yes, they are an important priority.

Revision history for this message
Dolph Mathews (dolph) wrote :

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

Revision history for this message
Nathan Kinder (nkinder) wrote :

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)

Revision history for this message
Dolph Mathews (dolph) wrote :

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

Revision history for this message
Dolph Mathews (dolph) wrote :
Revision history for this message
Dolph Mathews (dolph) wrote :
Revision history for this message
Dolph Mathews (dolph) wrote :
Revision history for this message
Dolph Mathews (dolph) wrote :

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

Revision history for this message
Nathan Kinder (nkinder) wrote :

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

Revision history for this message
Adam Young (ayoung) wrote :

patches looks good

Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

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 ?

Revision history for this message
Brant Knudson (blk-u) wrote :

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

Revision history for this message
David Stanek (dstanek) wrote :

master

Revision history for this message
David Stanek (dstanek) wrote :
Revision history for this message
David Stanek (dstanek) wrote :
Revision history for this message
David Stanek (dstanek) wrote :

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.

Revision history for this message
Brant Knudson (blk-u) wrote :

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

Revision history for this message
Morgan Fainberg (mdrnstm) wrote :

The patches in 23-25 LGTM.

Thierry Carrez (ttx)
Changed in ossa:
importance: Undecided → Medium
status: Incomplete → Confirmed
Thierry Carrez (ttx)
Changed in keystone:
status: Triaged → In Progress
Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

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.

Revision history for this message
Brant Knudson (blk-u) wrote :

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.

Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

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.

Revision history for this message
Brant Knudson (blk-u) wrote :

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.

Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote :

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.

Changed in ossa:
status: Confirmed → In Progress
summary: - Catalog replacement allows reading config
+ Catalog replacement allows reading config (CVE-2014-3621)
Revision history for this message
Tristan Cacqueray (tristan-cacqueray) wrote : Re: Catalog replacement allows reading config (CVE-2014-3621)

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!

Revision history for this message
David Stanek (dstanek) wrote :

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

Revision history for this message
Brant Knudson (blk-u) wrote :

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

Changed in ossa:
status: In Progress → Fix Committed
assignee: nobody → Tristan Cacqueray (tristan-cacqueray)
summary: - Catalog replacement allows reading config (CVE-2014-3621)
+ [OSSA 2014-029] Catalog replacement allows reading config
+ (CVE-2014-3621)
information type: Private Security → Public Security
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (master)

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

Changed in keystone:
assignee: nobody → Tristan Cacqueray (tristan-cacqueray)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/icehouse)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/havana)

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

Changed in keystone:
milestone: none → juno-rc1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (stable/icehouse)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

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

Changed in keystone:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (stable/havana)

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

Changed in ossa:
status: Fix Committed → Fix Released
Revision history for this message
Matthew Thode (prometheanfire) wrote :

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

Revision history for this message
David Stanek (dstanek) wrote :

@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?

Revision history for this message
Dolph Mathews (dolph) wrote :

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 (?).

Thierry Carrez (ttx)
Changed in keystone:
status: Fix Committed → Fix Released
Revision history for this message
Ray C Horn (raychorn) wrote :

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.

Revision history for this message
Dolph Mathews (dolph) wrote :

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

Thierry Carrez (ttx)
Changed in keystone:
milestone: juno-rc1 → 2014.2
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers