get_identity_providers policy should be singular

Bug #1703369 reported by Matthew Edmonds on 2017-07-10
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Dashboard (Horizon)
Undecided
Radomir Dopieralski
OpenStack Identity (keystone)
Low
Matthew Edmonds
Newton
Undecided
Lance Bragstad
Ocata
Undecided
Lance Bragstad
OpenStack Security Advisory
Undecided
Unassigned
OpenStack Security Notes
Undecided
Luke Hinds

Bug Description

identity:get_identity_providers should be identity:get_identity_provider (singular) since a GET is targeted on a single provider and the code is setup to check for identity:get_identity_provider (singular). See https://github.com/openstack/keystone/blob/c7e29560b7bf7a44e44722eea0645bf18ad56af3/keystone/federation/controllers.py#L112

found in master (pike)

The ocata default policy.json also has this problem. Unless someone manually overrode policy to specify identity:get_identity_provider (singular), the result would be that the default rule was actually used for that check instead of identity:get_identity_providers. We could go back and fix the default policy.json for past releases, but the default actually has the same value as identity:get_identity_providers, and if nobody has complained it's probably safer to just leave it. It is, after all, just defaults there and anyone can override by specifying the correct value.

But we must fix in pike to go along with the shift of policy into code. Policy defaults in code definitely need to match up with what the code actually checks. There should no longer be any reliance on the default rule.

Changed in keystone:
assignee: nobody → Matthew Edmonds (edmondsw)

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

Changed in keystone:
status: New → In Progress
Boris Bobrov (bbobrov) wrote :

This might be serious for someone. Before this change, get_identity_provider method was protected only by "default" rule, and "get_identity_providers" rule in policy json was protecting nothing (because no such method exist in keystone. If some operator relied on changing the "get_identity_providers" rule, or on changing the "default" rule, they might be affected.

Luckily, it is hard to find out id of an identity provider. Getting an identity provider by an unprivileged user probably doesn't give out any useful info.

Fixing this bug will probably not break anybody; if it will, they probably have security vulnerability in their system.

information type: Public → Public Security
Changed in keystone:
importance: Undecided → Low
description: updated
Jeremy Stanley (fungi) wrote :

Just to confirm, this is only a risk for the upcoming Pike release, not a defect in current supported stable branches? If so, we would not issue a security advisory unless it remains unfixed at release time per report class Y ("Vulnerability only found in development release") in our taxonomy: https://security.openstack.org/vmt-process.html#incident-report-taxonomy

Changed in ossa:
status: New → Incomplete
Matthew Edmonds (edmondsw) wrote :

I don't know that I'd consider this a vulnerability in any release, but I guess you could say that since we ignore "identity:get_identity_providers" in keystone policy.json, someone who changes that value will think they've restricted that API when they have not actually done so. But the default that would have taken effect was admin (for any release), so the only way you could be trying to restrict more than that is to disable it entirely. And someone customizing policy should test that their changes are working, so I don't expect anybody is in this boat today.

Boris Bobrov (bbobrov) wrote :

> Just to confirm, this is only a risk for the upcoming Pike release, not a defect in current supported stable branches?

No, there is risk for all supported branches.

My guess is that it is C1 issue.

Jeremy Stanley (fungi) wrote :

Is there any intent to backport fixes for this issue to newton and ocata, or is the plan to only fix it in pike?

Matthew Edmonds (edmondsw) wrote :

Boris, exactly what do you think the vulnerability is here? Is it what I said in comment 4? If so, does the rest of my comment there not assuage your concerns? Please also check the description, which I updated with reasoning including why I don't think we should backport anything to ocata.

Boris Bobrov (bbobrov) wrote :

> Unless someone manually overrode policy to specify identity:get_identity_provider (singular), the result would be that the default rule was actually used for that check instead of identity:get_identity_providers.

Changing policy.json and default rules is a normal operation for the cloud. We were going to change it in our public cloud, for example.

> We could go back and fix the default policy.json for past releases, but the default actually has the same value as identity:get_identity_providers, and if nobody has complained it's probably safer to just leave it. It is, after all, just defaults there and anyone can override by specifying the correct value.

Or we could rename the controller to get_identity_providers. But i think the risk here would be bigger than if we just ignore it.

We probably will not backport it to prior releases.

Lance Bragstad (lbragstad) wrote :

To recap the conversation and summarize what was discuss in IRC [0].

There is a security issue if a deployer modifies the default policy role required for an operation but wishes to keep the identity:get_identity_providers protected at the "admin-level". This was deemed as unlikely since the default and get_identity_provider were protected with the same admin_required rule.

For the sake of process, we can merge the proposed fix [1] with a detailed release note explaining the case. After that we can propose the patch to stable/ocata as well as stable/newton. Even though a deployer can technically issue this fix without a new release, the process of issuing a release note seems valuable at least for the sake of process.

[0] http://eavesdrop.openstack.org/irclogs/%23openstack-keystone/%23openstack-keystone.2017-07-11.log.html#t2017-07-11T21:26:46
[1] https://review.openstack.org/#/c/482142/

Changed in keystone:
milestone: none → pike-3

I've added an OSSN task to see if a Security Note would make more sense here since this is kind of an insecure default config value (class B2).

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

commit b7119637a04d0a07fa6419a407f433c01bbd1db2
Author: Matthew Edmonds <email address hidden>
Date: Mon Jul 10 09:20:18 2017 -0400

    fix identity:get_identity_providers typo

    Changes identity:get_identity_providers policy rule to
    identity:get_identity_provider to match what is checked by the code.

    Change-Id: I0841abd30fd15c034b5836e42a18938634b509b1
    Closes-Bug: #1703369

Changed in keystone:
status: In Progress → Fix Released
Luke Hinds (lhinds) on 2017-07-12
Changed in ossn:
assignee: nobody → Luke Hinds (lhinds)
status: New → Confirmed
Jeremy Stanley (fungi) wrote :

Since Luke is running with the OSSN task confirmed, I'm going to take that as agreement that this is class B2 and set our OSSA task to won't fix. Thanks!

Changed in ossa:
status: Incomplete → Won't Fix
tags: added: security
information type: Public Security → Public

Reviewed: https://review.openstack.org/485694
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=6c96675d63257d7ef2c27c5598c642efa7a25d08
Submitter: Jenkins
Branch: stable/ocata

commit 6c96675d63257d7ef2c27c5598c642efa7a25d08
Author: Matthew Edmonds <email address hidden>
Date: Mon Jul 10 09:20:18 2017 -0400

    fix identity:get_identity_providers typo

    Changes identity:get_identity_providers policy rule to
    identity:get_identity_provider to match what is checked by the code.

    Conflicts:
      keystone/common/policies/identity_provider.py

    There was a conflict backporting this change since the policy-in-code
    work in new in Pike. The conflict was resolved by removing the
    policy-in-code change and making it manually against the old
    etc/policy.json file.

    Change-Id: I0841abd30fd15c034b5836e42a18938634b509b1
    Closes-Bug: #1703369
    (cherry picked from commit b7119637a04d0a07fa6419a407f433c01bbd1db2)

Reviewed: https://review.openstack.org/485695
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=bd49c3ef6daa474e9c84c0d8721c0f6812ee3d2c
Submitter: Jenkins
Branch: stable/newton

commit bd49c3ef6daa474e9c84c0d8721c0f6812ee3d2c
Author: Matthew Edmonds <email address hidden>
Date: Mon Jul 10 09:20:18 2017 -0400

    fix identity:get_identity_providers typo

    Changes identity:get_identity_providers policy rule to
    identity:get_identity_provider to match what is checked by the code.

    Conflicts:
      keystone/common/policies/identity_provider.py

    There was a conflict backporting this change since the policy-in-code
    work in new in Pike. The conflict was resolved by removing the
    policy-in-code change and making it manually against the old
    etc/policy.json file.

    Change-Id: I0841abd30fd15c034b5836e42a18938634b509b1
    Closes-Bug: #1703369
    (cherry picked from commit b7119637a04d0a07fa6419a407f433c01bbd1db2)
    (cherry picked from commit 8038f545daa31354e08a4063209295439005c0b8)

This issue was fixed in the openstack/keystone 11.0.3 release.

This issue was fixed in the openstack/keystone 10.0.3 release.

This issue was fixed in the openstack/keystone 12.0.0.0b3 development milestone.

Nick Tait (nickthetait) wrote :

I am starting to work on this task.

Nick Tait (nickthetait) wrote :

Have published a draft of this OSSN. Seeking reviews here https://review.openstack.org/#/c/559440/

Is this a comprehensive list of affected releases: Pike, Ocata, and Newton? What about Mitaka?

Nick Tait (nickthetait) wrote :

OSSN has been merged. This bug can be closed now.

The same typo is in Horizon too:

$ git remote -v
origin git://git.openstack.org/openstack/horizon (fetch)
origin git://git.openstack.org/openstack/horizon (push)
$ grep -rl get_identity_providers
doc/source/contributor/topics/policy.rst
openstack_auth/tests/conf/keystone_policy.json
openstack_auth/tests/conf/policy.v3cloudsample.json

Should I submit a new bug?

Luke Hinds (lhinds) wrote :

Sounds right Mircea, but it won't be a security issue this time, as its in docs / unit tests, rather than code that could be used in production. Still needs a bug raised in horizon though, and well spotted.

Changed in ossn:
status: Confirmed → Fix Released

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

Changed in horizon:
assignee: nobody → Radomir Dopieralski (deshipu)
status: New → In Progress

Reviewed: https://review.openstack.org/564150
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=93bb571888a1bff4fa1e110356dbf2cb9fb4ee52
Submitter: Zuul
Branch: master

commit 93bb571888a1bff4fa1e110356dbf2cb9fb4ee52
Author: Radomir Dopieralski <email address hidden>
Date: Wed Apr 25 11:37:05 2018 +0200

    Replace all mentions of get_identity_providers with get_identity_provider

    There was a typo in keystone's policy files, and it has been fixed in
    Keystone already, we should also fix it to match.

    Change-Id: I41e4381765f3bfc5988ca235e6cbeb6d1ba62fc2
    Closes-bug: #1703369

Changed in horizon:
status: In Progress → Fix Released

Reviewed: https://review.openstack.org/564219
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=0253c8f1221594335afd9a2582a310e3e1d2c0a3
Submitter: Zuul
Branch: stable/queens

commit 0253c8f1221594335afd9a2582a310e3e1d2c0a3
Author: Radomir Dopieralski <email address hidden>
Date: Wed Apr 25 11:37:05 2018 +0200

    Replace all mentions of get_identity_providers with get_identity_provider

    There was a typo in keystone's policy files, and it has been fixed in
    Keystone already, we should also fix it to match.

    Change-Id: I41e4381765f3bfc5988ca235e6cbeb6d1ba62fc2
    Closes-bug: #1703369
    (cherry picked from commit 93bb571888a1bff4fa1e110356dbf2cb9fb4ee52)

tags: added: in-stable-queens

This issue was fixed in the openstack/horizon 13.0.1 release.

This issue was fixed in the openstack/horizon 14.0.0.0b2 development milestone.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers