protected property change not rejected if a subsequent rule match accepts them

Bug #1271426 reported by Mark Washenberger
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
High
Thomas Leaman
Havana
High
Thomas Leaman
OpenStack Security Notes
High
Nathan Kinder

Bug Description

See initial report here: http://lists.openstack.org/pipermail/openstack-dev/2014-January/024861.html

What is happening is that if there is a specific rule that would reject an action and a less specific rule that comes after that would accept the action, then the action is being accepted. It should be rejected.

This is because we iterate through the property protection rules rather than just finding the first match. This bug does not occur when policies are used to determine property protections, only when roles are used directly.

Feilong Wang (flwang)
Changed in glance:
status: New → Confirmed
importance: Undecided → High
Changed in glance:
status: Confirmed → Triaged
milestone: none → icehouse-3
Revision history for this message
Stuart McLaren (stuart-mclaren) wrote :

This is a candidate for backporting to havana stable.

Changed in glance:
assignee: nobody → Thomas Leaman (thomas-leaman)
Changed in glance:
status: Triaged → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to glance (master)

Reviewed: https://review.openstack.org/68420
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=b6dd538569ebf0f1580c8e1fadc5e0f8054c9b08
Submitter: Jenkins
Branch: master

commit b6dd538569ebf0f1580c8e1fadc5e0f8054c9b08
Author: Thomas Leaman <email address hidden>
Date: Wed Jan 22 16:02:56 2014 +0000

    Check first matching rule for protected properties

    When using roles to define protected properties, the first matching rule
    in the config file should be used to grant/deny access. This change
    enforces that behaviour.

    Fixes bug 1271426

    Change-Id: I11ece25ae85ff868516bcd1839a4e430e9c51370

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

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

Revision history for this message
Grant Murphy (gmurphy) wrote :

This seems like something that might catch out unsuspecting sysadmins. Do you think it is worth issuing an OSSN for this?

Revision history for this message
Robert Clark (robert-clark) wrote :

I'm happy to draft up an OSSN, I'll work with Stuart to get the details right.

-Rob

Changed in ossn:
status: New → Confirmed
importance: Undecided → High
assignee: nobody → Robert Clark (robert-clark)
Revision history for this message
Robert Clark (robert-clark) wrote :

So the remediation step for implementations that are not backported or up to date is to order properties such that specific property rules should follow generic ones?

To use the example given here: http://lists.openstack.org/pipermail/openstack-dev/2014-January/024861.html if the foo_property settings were placed after the [.*] settings the problem would be avoided?

Revision history for this message
Grant Murphy (gmurphy) wrote :

@robert-clark That is my understanding.

Thierry Carrez (ttx)
Changed in glance:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in glance:
milestone: icehouse-3 → 2014.1
Revision history for this message
Robert Clark (robert-clark) wrote :

Just finishing up the OSSN now, please accept my apologies for it taking so long, dropped off the radar for some reason.

How many versions back does this need to go?

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

This has been published as OSSN-0013 to the mailing lists (openstack and openstack-dev), and the OpenStack wiki:

  https://wiki.openstack.org/wiki/OSSN/OSSN-0013

Changed in ossn:
status: Confirmed → Fix Released
Revision history for this message
Nathan Kinder (nkinder) wrote :

Reopening this OSSN bug. The workaround in the OSSN has been reported to not work. Details from the reporter to come shortly.

Changed in ossn:
status: Fix Released → In Progress
Revision history for this message
Mathieu Gagné (mgagne) wrote :

The workaround proposed in OSSN-0013 does not work (tested against 2013.2):

"Users of affected releases should review and reorder the entries in property-protections.conf to place the most open permissions at the start of the configuration and more restrictive ones at the end, as demonstrated below."

The implementation (in 2013.2) will stop the processing of rules at the first positive (authorized) match. This means by placing the most generic rule first, all following rules will be ignored.

One possible workaround would be to replace the generic rule [.*] by a very complex regex crafted in a way to NOT match all the other rules protecting your properties.

As explained above, the faulty implementation WON'T stop processing the rules if a matching rule (property regex) is found. You therefore don't want a protected property to be authorized by a wildcard. Hopefully, it is made so if NO match is found, the property is still protected.

Example of such workaround:

[^provider_.*$]
create = admin
read = _member_
update = admin
delete = admin

# Note the trailing $ to make sure all the string is matched
[^((?!provider_).)*$]
create = _member_
read = _member_
update = _member_
delete = _member_

Revision history for this message
Mathieu Gagné (mgagne) wrote :

It should also be noted that the @ wildcard is not available in Havana.

Revision history for this message
Mathieu Gagné (mgagne) wrote :

admin role needs to be added to read permission to be able to update it. (In the case where your user does not have the _member_ role, only admin)

New example:

[^provider_.*$]
create = admin
read = admin,_member_
update = admin
delete = admin

# Note the trailing $ to make sure all the string is matched
[^((?!provider_).)*$]
create = _member_
read = _member_
update = _member_
delete = _member_

Revision history for this message
Robert Clark (robert-clark) wrote :

Thank you for catching this and providing guidance on the improved content.

I'll cut a new OSSN for this

Revision history for this message
Nathan Kinder (nkinder) wrote :
Download full text (3.4 KiB)

I have tested the new proposed workaround listed in comment #13 using the 2013.2 release (in devstack). The workaround is behaving correctly for me:

[nkinder@localhost devstack]$ source openrc admin admin
[nkinder@localhost devstack]$ glance image-create --property provider_foo=foo --name foo
+-------------------------+--------------------------------------+
| Property | Value |
+-------------------------+--------------------------------------+
| Property 'provider_foo' | foo |
| checksum | None |
| container_format | None |
| created_at | 2014-05-31T00:04:44 |
| deleted | False |
| deleted_at | None |
| disk_format | None |
| id | a8fbd50a-5871-433d-9097-5d3320339d98 |
| is_public | False |
| min_disk | 0 |
| min_ram | 0 |
| name | foo |
| owner | 6cad2594f37549798970593027b587b3 |
| protected | False |
| size | 0 |
| status | queued |
| updated_at | 2014-05-31T00:04:44 |
| virtual_size | None |
+-------------------------+--------------------------------------+

[nkinder@localhost devstack]$ source openrc demo demo
[nkinder@localhost devstack]$ glance image-create --property provider_bar=bar --name bar
403 Forbidden
Property 'provider_bar' is protected
    (HTTP 403)
[nkinder@localhost devstack]$ glance image-create --property junk_bar=bar --name bar
+---------------------+--------------------------------------+
| Property | Value |
+---------------------+--------------------------------------+
| Property 'junk_bar' | bar |
| checksum | None |
| container_format | None |
| created_at | 2014-05-31T00:05:46 |
| deleted | False |
| deleted_at | None |
| disk_format | None |
| id | 4ce3b1de-d5ad-46ac-acbe-2d0ed50d53c8 |
| is_public | False |
| min_disk | 0 |
| min_ram | 0 |
| name | bar |
| owner | 6cad2594f37549798970593027b587b3 |
| protected | False |
| size | 0 |
...

Read more...

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

I have proposed a patch to correct OSSN-0013 here:

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

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

The new revision of OSSN-0013 has been published to the mailing lists and the wiki:

  https://wiki.openstack.org/wiki/OSSN/OSSN-0013

Changed in ossn:
assignee: Robert Clark (robert-clark) → Nathan Kinder (nkinder)
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers