create_subnet policy allows users to create subnet in the shared networks

[Oleksandr Kozachenko]

## Context
We normally provide external network as a shared resource so any users can use it.
But with this new scoped policy, users can create subnets in that external network even if they are not the member of admin project.
```
"create_subnet": "(rule:admin_only) or (role:member and project_id:%(project_id)s) or rule:network_owner"
```
If i remove `(role:member and project_id:%(project_id)s)` partial rule or change `project_id:%(project_id)s` to `project_id:%(network:project_id)s`, then it works as expected. i.e. users cannot create subnets in the external network.

## Expected result
Users should not be able to create subnets in shared networks or default networks if they are not the member of the networks' owned projects.

## Version infor
release: stable/zed
I was able to reproduce it in zed Devstack also. Btw, master Devstack worsk as expected.

## Workaround
We use deprecated rule `"create_subnet":"rule:admin_or_network_owner"` and it works without any issue.

## Concern
- I am not sure why we need `(role:member and project_id:%(project_id)s)` rule.
- I didn't have a chance to check other new policies if they also have such a perm gap.

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete
security advisory task has been added while the core security
reviewers for the affected project or projects confirm the bug and
discuss the scope of any vulnerability along with potential
solutions.

[Brian Haley (brian-haley)]

I will have to defer to Slawek on the policy checking, but think the behavior might be different on master due to the code maybe being different? Locally I see the OVN metadata port creation fail, triggering an exception:

ERROR neutron.plugins.ml2.managers Traceback (most recent call last):
ERROR neutron.plugins.ml2.managers File "/usr/local/lib/python3.10/dist-packages/neutron_lib/plugins/utils.py", line 306, in _fixup_res_dict
ERROR neutron.plugins.ml2.managers attr_ops.populate_project_id(context, res_dict, True)
ERROR neutron.plugins.ml2.managers File "/usr/local/lib/python3.10/dist-packages/neutron_lib/api/attributes.py", line 257, in populate_project_id
ERROR neutron.plugins.ml2.managers _validate_privileges(context, res_dict)
ERROR neutron.plugins.ml2.managers File "/usr/local/lib/python3.10/dist-packages/neutron_lib/api/attributes.py", line 32, in _validate_privileges
ERROR neutron.plugins.ml2.managers raise exc.HTTPBadRequest(msg)
ERROR neutron.plugins.ml2.managers webob.exc.HTTPBadRequest: Specifying 'project_id' or 'tenant_id' other than the authenticated project in request requires admin or advsvc privileges
ERROR neutron.plugins.ml2.managers
ERROR neutron.plugins.ml2.managers The above exception was the direct cause of the following exception:
ERROR neutron.plugins.ml2.managers
ERROR neutron.plugins.ml2.managers Traceback (most recent call last):
ERROR neutron.plugins.ml2.managers File "/opt/stack/neutron/neutron/plugins/ml2/managers.py", line 497, in _call_on_drivers
ERROR neutron.plugins.ml2.managers getattr(driver.obj, method_name)(context)
ERROR neutron.plugins.ml2.managers File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py", line 633, in create_subnet_postcommit
ERROR neutron.plugins.ml2.managers self._ovn_client.create_subnet(context.plugin_context,
ERROR neutron.plugins.ml2.managers File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py", line 2197, in create_subnet
ERROR neutron.plugins.ml2.managers mport_updated = self.update_metadata_port(
ERROR neutron.plugins.ml2.managers File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py", line 2363, in update_metadata_port
ERROR neutron.plugins.ml2.managers metadata_port = self.create_metadata_port(context, network)
ERROR neutron.plugins.ml2.managers File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py", line 2331, in create_metadata_port
ERROR neutron.plugins.ml2.managers return p_utils.create_port(self._plugin, context, port)
ERROR neutron.plugins.ml2.managers File "/usr/local/lib/python3.10/dist-packages/neutron_lib/plugins/utils.py", line 335, in create_port
ERROR neutron.plugins.ml2.managers port_data = _fixup_res_dict(context, port_apidef.COLLECTION_NAME,
ERROR neutron.plugins.ml2.managers File "/usr/local/lib/python3.10/dist-packages/neutron_lib/plugins/utils.py", line 312, in _fixup_res_dict
ERROR neutron.plugins.ml2.managers raise ValueError(e.detail) from e
ERROR neutron.plugins.ml2.managers ValueError: Specifying 'project_id' or 'tenant_id' other than the authenticated p...

[Oleksandr Kozachenko (okozachenko)]

yes, @Brian. In devstack stable/zed, subnet creation is failing with 500 HTTP response code because of that ovn port creation failure, but it means hook.PolicyHook has been passed.
When I override the default policy using deprecated one `rule:admin_or_network_owner`, it fails with 403 (disallowed subnet creation) correctly at the prehook step.

Btw, in the devstack master branch, I can see that subnet creation fails with 403 with the default policy.
So I can say this issue has been fixed in the master branch.

[Lajos Katona (lajos-katona)]

Brian, Oleksandr: thanks for the investigation.
We backported most of the RBAC things to stable/zed, is it possible that it is just something we missed to backport?
I mean perhaps something is missing which made it work not just with sRBAC but with legacy policies also.

[Slawek Kaplonski (slaweq)]

Thx for reporting this issue. I will check it in stable/zed to see what can be missing there comparing to master branch.

[Slawek Kaplonski (slaweq)]

@Oleksandr, can You additionally write details about exact neutron and neutron-lib versions where You have this issue?

[Slawek Kaplonski (slaweq)]

Another question related to this issue: do You have any other custom policies in Your policy.yaml file? If so, can You share it with us?

I was trying to reproduce the issue You reported and I wasn't able to do that but it was actually failing for me on GET network request which is done internally during create subnet action.
But regardless I was able to reproduce it or not, I think You found mistake in the default new policies and I will propose patch to change that.

@fungi: I don't think we should treat this issue as private security issue - this new policy is used by default just in master branch, in any stable versions it's not enabled by default. It also can be simply overwritten in the custom policy file if needed.

[Jeremy Stanley (fungi)]

Thanks, in that case this falls into our B1 report class so I've switched it to a regular public bug tagged security-related and marked our advisory task won't fix. https://security.openstack.org/vmt-process.html#report-taxonomy

[Oleksandr Kozachenko (okozachenko)]

Slawek: I was able to reproduce this issue in both devstack zed environment and our staging environment deployed by openstack-helm.

Here are the versions.
- devstack stable/zed
git+https://opendev.org/openstack/neutron@e8a00b9c52a658a57d4d0ec7e5cdcd39c581afd0#egg=neutron
neutron-lib==3.6.1

- our staging env
git+https://opendev.org/openstack/neutron@e8a00b9c52a658a57d4d0ec7e5cdcd39c581afd0#egg=neutron
neutron-lib==3.1.2

Regarding the custom policy,
- I don't override the default policy in our staging env.
- in devstack zed, there is one custom policy. "context_is_admin": "role:admin or user_name:neutron"

[Tobias Urdin (tobias-urdin)]

I can confirm that we have the same issue on Yoga 20.3.0 I will check further if we can use workaround proposed here.

[Tobias Urdin (tobias-urdin)]

Neutron 20.3.0
Neutron-lib 2.20.1

[Tobias Urdin (tobias-urdin)]

We tested and rolled out the workaround reverting to the deprecated rule and it works fine, we will wait and follow the progress of this bug. Hopefully the same issue does not exist for more rules.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/886231

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/886231
Committed: https://opendev.org/openstack/neutron/commit/6e3525188fdfbe7fabd665e21df2068280471689
Submitter: "Zuul (22348)"
Branch: master

commit 6e3525188fdfbe7fabd665e21df2068280471689
Author: Slawek Kaplonski <email address hidden>
Date: Thu Jun 15 12:59:03 2023 -0700

    [S-RBAC] Fix policies for CUD subnets APIs

    In new, secure RBAC policies for create subnet there was
    rule "ADMIN_OR_PROJECT_MEMBER" used and that was wrong as this rule is
    basically allows any member (PROJECT_MEMBER) create subnet in networks
    visible to them, not necessarily this project needs to be owner of that
    network. So it allowed users to create new subnets in the shared or
    provider networks as well.
    Now policy for create subnet is ADMIN OR NET_OWNER_MEMBER to avoid that.

    Additionally this patch also fixes policies for update and delete subnet
    APIs where there was rule NET_OWNER used and that effectively allowed to
    update or delete subnet to the network owner who has READER role only.
    Now this is also fixed by using NET_OWNER_MEMBER rule instead.

    Closes-Bug: #2023679

    Change-Id: Ia494872b58f368581fb29fa40b7da17e1071db22

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/2023.1
Review: https://review.opendev.org/c/openstack/neutron/+/886744

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/zed
Review: https://review.opendev.org/c/openstack/neutron/+/886680

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron-lib/+/886746

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron 22.0.2 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron 21.1.2 release.

[Lukasz Zalewski (lzal3wski)]

Hi all
We have discovered the exact same issue in Yoga. Any chance for the fix to be backported to it?

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/889153
Committed: https://opendev.org/openstack/neutron/commit/0741a0d5a55024787c7324d060bd3f5a79ffcb0e
Submitter: "Zuul (22348)"
Branch: master

commit 0741a0d5a55024787c7324d060bd3f5a79ffcb0e
Author: Slawek Kaplonski <email address hidden>
Date: Fri Jul 21 10:11:47 2023 +0200

    Add NET_OWNER_MEMBER and NET_OWNER_READER policy rules

    Initially when [1] was proposed the idea was to use PARENT_OWNER_* rules
    for the subnet APIs, in the same way as it is done currently for e.g.
    FIP PF, QoS rules and some other resources.
    But after some more thinking about it, it's better to keep
    NET_OWNER rules for MEMBER and READER. Those rules are basically very
    similar to the PARENT_OWNER_*, the only difference is that it relies on
    the "network_id" attribute always.
    The reason why it's better to have NET_OWNER_* rules and use them for
    subnets and ports is that subnets and ports are actually top level API
    resources in neutron. They aren't actually childs of the network so
    using PARENT_OWNER for them could be missleading and confusing for
    users.

    Related-Bug: #2023679

    [1] https://review.opendev.org/c/openstack/neutron/+/886231

    Change-Id: I52fc92f76842f9f075e9e4c49262785ca099bdf8

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron 23.0.0.0b3 development milestone.

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Slawek Kaplonski <email address hidden>" on branch: stable/yoga
Review: https://review.opendev.org/c/openstack/neutron/+/889799
Reason: This review is > 4 weeks without comment, and failed Zuul jobs the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.