Some APIs don't check the owner policy

[wangxiyuan]

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.

[Jeremy Stanley (fungi)]

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.

[TommyLike (hu-husheng)]

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

[Eric Harney (eharney)]

This needs more investigation on the exact impact.

[TommyLike (hu-husheng)]

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

[OpenStack Infra (hudson-openstack)]

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

[Sean McGinnis (sean-mcginnis)]

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.

[Sean McGinnis (sean-mcginnis)]

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

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/queens
Review: https://review.openstack.org/593675

[OpenStack Infra (hudson-openstack)]

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>

[Gage Hugo (gagehugo)]

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.

[Jeremy Stanley (fungi)]

Since the fixes for this merged long enough ago, there are no longer any stable branches under support which were vulnerable to it. As such, I'm setting our security advisory task to Won't Fix.