Some APIs don't check the owner policy

Bug #1714858 reported by wangxiyuan on 2017-09-04
258
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Critical
TommyLike
OpenStack Security Advisory
Undecided
Unassigned

Bug Description

For the policy "admin or owner", If the request is not admin, it should compare the owner between "context.project" and "target.project", but actually for many APIs, it doesn't check it at all.

The "volume show" API does the correct way. But others, such as "snapshot show" and "backup show" doesn't.
Both the incorrect APIs check the policy like the way "context.project == context.project" which is wrong.

I guess some other APIs are wrong as well. Feel free to find them out.

wangxiyuan (wangxiyuan) on 2017-09-04
summary: - some API doesn't check the owner policy
+ Some APIs doesn't check the owner policy
Changed in cinder:
assignee: nobody → TommyLike (hu-husheng)
status: New → In Progress
Eric Harney (eharney) on 2018-03-01
Changed in cinder:
importance: Undecided → Critical
Eric Harney (eharney) on 2018-03-01
information type: Public → Public Security

I see you've switched this from Public to Public Security bug type indicating you believe it describes a vulnerability. Unfortunately, the security implications of this report are unclear (at least to me). Can someone elaborate on the associated risks and a possible exploit scenario or two? Having a more thorough list of which "some other APIs" are guessed to be similarly wrong would also be appreciated.

Changed in ossa:
status: New → Incomplete
TommyLike (hu-husheng) wrote :

@Eric Harney, @Jeremy Stanely, I think this bug not belongs to a Public Security or even Security/Critial bug as non-administrator still can not query the resource which doesn't in his tenant.

This bug is more about should we make our policy enforcement system work exactly as what is define, for instance:
we have our policy rule: admin_or_owner:
```
policy.RuleDefault('admin_or_owner',
                       'is_admin:True or (role:admin and '
                       'is_admin_project:True) or project_id:%(project_id)s',
                       description="Default rule for most non-Admin APIs."),
```
and enforce policy in code:
```
    def get_snapshot(self, context, snapshot_id):
        context.authorize(snapshot_policy.GET_POLICY)
        snapshot = objects.Snapshot.get_by_id(context, snapshot_id)
```
the problem is when non-administrator wants to query resource which doesn't below to his tenant, the authorize will always succeed and he will get a 404 rather than 403.

Changed in cinder:
importance: Critical → Medium
Changed in ossa:
status: Incomplete → Invalid
Eric Harney (eharney) wrote :

This needs more investigation on the exact impact.

Changed in cinder:
importance: Medium → Critical
Changed in ossa:
status: Invalid → Incomplete
TommyLike (hu-husheng) wrote :
Download full text (5.6 KiB)

After investigation in cinder, found one API that would have a potential security issue: reset volume's status, this is the process to reproduce.

1. assume create a volume in demon1 project with non-admin user:
```
+--------------------------------+--------------------------------------+
| Property | Value |
+--------------------------------+--------------------------------------+
| attachments | [] |
| availability_zone | nova |
| bootable | false |
| consistencygroup_id | None |
| created_at | 2018-03-15T03:13:39.000000 |
| description | None |
| encrypted | False |
| group_id | None |
| id | 074c4b78-af13-4b7c-a8ef-29fa5604197f |
| metadata | {} |
| migration_status | None |
| multiattach | False |
| name | None |
| os-vol-host-attr:host | None |
| os-vol-mig-status-attr:migstat | None |
| os-vol-mig-status-attr:name_id | None |
| os-vol-tenant-attr:tenant_id | 61feeebaad1d4e4382c1c764f2033a79 |
| provider_id | None |
| replication_status | None |
| size | 1 |
| snapshot_id | None |
| source_volid | None |
| status | creating |
| updated_at | 2018-03-15T03:13:39.000000 |
| user_id | 7bf2008b095d4961a9f4bc92a475c54e |
| volume_type | lvmdriver-1 |
```
2. swith into another project demo2 with non administrator which doesn't have any volumes:
```
+----+--------+------+------+-------------+----------+-------------+
| ID | Status | Name | Size | Volume Type | Bootable | Attached to |
+----+--------+------+------+-------------+----------+-------------+
+----+--------+------+------+-------------+----------+-------------+
```
3. try to reset the new created volume's status in this project, and this is the result:
```
{
    "os-reset_status": {
        "status": "errpr"
    }
}
RESPONSE:
{
    "forbidden": {
        "message": "Policy doesn't allow volume_extension:volume_admin_actions:reset_status to be performed.",
        "code": 403
    }
}
```
4. update the reset_status's policy rule into admin_or_owner [1] and restart the service
5. try to reset the status again, cinder...

Read more...

Reviewed: https://review.openstack.org/548650
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=7391070474269dc247fc1d1f43520087a6a10267
Submitter: Zuul
Branch: master

commit 7391070474269dc247fc1d1f43520087a6a10267
Author: TommyLike <email address hidden>
Date: Wed Feb 28 16:14:17 2018 +0000

    Add missing 'target_obj' when perform policy check

    Generally, we have to pass target object to ``authorize``
    when enforce policy check, but this is ignored during
    our develop and review process for a long time, and the
    potential issue is anyone can handle the target resource
    as ``authorize`` will always succeed if rule is defined
    ``admin_or_owner`` [1]. Luckily, for most of those APIs
    this security concern is protected by our database access
    code [2] that only project scope resource is allowed.

    However, there is one API that do have security issue when
    administrator change the rule into "admin_or_owner".

    1. "volume reset_status", which cinder will update the
    resource directly in the database, procedure to reproduce
    bug is described on the launchpad.

    This patch intends to correct most of cases which can be
    easily figured out in case of future code changes.

    [1]:
    https://github.com/openstack/cinder/blob/73e6e3c147fc357031834d0ac28478d061e6120c/cinder/context.py#L206
    [2]:
    https://github.com/openstack/cinder/blob/73e6e3c147fc357031834d0ac28478d061e6120c/cinder/db/sqlalchemy/api.py#L3058
    [3]:
    https://github.com/openstack/cinder/blob/73e6e3c147fc357031834d0ac28478d061e6120c/cinder/api/contrib/admin_actions.py#L161

    Partial-Bug: #1714858
    Change-Id: I351b3ddf8dfe29da8d854d4038d64ca7be17390f

TommyLike, the merged patch was marked as a partial fix for this. Were there more cases identified that we still need to fix, or should that have been Closes-bug instead?

If that was it, please mark this as Fix Released or comment on here on the status.

summary: - Some APIs doesn't check the owner policy
+ Some APIs don't check the owner policy
Sean McGinnis (sean-mcginnis) wrote :

Marking as fixed, but please change the state back if there is something more to this.

Changed in cinder:
status: In Progress → Fix Released

Reviewed: https://review.openstack.org/593675
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=76d3c644f3f5866a5c07f80b66132d81b8b57e78
Submitter: Zuul
Branch: stable/queens

commit 76d3c644f3f5866a5c07f80b66132d81b8b57e78
Author: TommyLike <email address hidden>
Date: Wed Feb 28 16:14:17 2018 +0000

    Add missing 'target_obj' when perform policy check

    Generally, we have to pass target object to ``authorize``
    when enforce policy check, but this is ignored during
    our develop and review process for a long time, and the
    potential issue is anyone can handle the target resource
    as ``authorize`` will always succeed if rule is defined
    ``admin_or_owner`` [1]. Luckily, for most of those APIs
    this security concern is protected by our database access
    code [2] that only project scope resource is allowed.

    However, there is one API that do have security issue when
    administrator change the rule into "admin_or_owner".

    1. "volume reset_status", which cinder will update the
    resource directly in the database, procedure to reproduce
    bug is described on the launchpad.

    This patch intends to correct most of cases which can be
    easily figured out in case of future code changes.

    [1]: http://git.openstack.org/cgit/openstack/cinder/tree/cinder/context.py?id=73e6e3c147fc357031834d0ac28478d061e6120c#n206
    [2]: http://git.openstack.org/cgit/openstack/cinder/tree/cinder/db/sqlalchemy/api.py?id=73e6e3c147fc357031834d0ac28478d061e6120c#n3058
    [3]: http://git.openstack.org/cgit/openstack/cinder/tree/cinder/api/contrib/admin_actions.py?id=73e6e3c147fc357031834d0ac28478d061e6120c#n161

    Conflicts:
        cinder/api/contrib/volume_image_metadata.py

    Partial-Bug: #1714858
    Change-Id: I351b3ddf8dfe29da8d854d4038d64ca7be17390f
    (cherry picked from commit 7391070474269dc247fc1d1f43520087a6a10267)
    Signed-off-by: Sean McGinnis <email address hidden>

tags: added: in-stable-queens
Gage Hugo (gagehugo) wrote :

Was this issue only in Rocky/Queens, or does it exist in the other stable branches? It looks like the authorize code changed between Pike -> Queens.

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