Port's device_owner field should not be editable

Bug #1337801 reported by Jaume Devesa
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
neutron
Invalid
Medium
Vishal Agarwal

Bug Description

According to 'delete_router' code, a router can not be deleted if there is a user port attached to it with device_owner==network:router_interface :

        device_owner = self._get_device_owner(context, router) // device owner is actually the string network:router_interface
        device_filter = {'device_id': [router_id],
                         'device_owner': [device_owner]}
        port_count = self._core_plugin.get_ports_count(
            admin_ctx, filters=device_filter)
        if port_count:
            raise l3.RouterInUse(router_id=router_id)

At this is what happens when you try to delete a router with the admin user:

    vagrant@devstack-single:~ > neutron router-delete 83d2c3d7-1665-41a1-99cf-9e6dfc24dcb7
    Router 83d2c3d7-1665-41a1-99cf-9e6dfc24dcb7 still has ports

However, if you switch user and edit the device_owner of the port attached to the router:

    vagrant@devstack-single:~ > export OS_USERNAME=demo
    vagrant@devstack-single:~ > neutron port-list -c id -c fixed_ips -c device_owner
    +--------------------------------------+---------------------------------------------------------------------------------+--------------------------+
    | id | fixed_ips | device_owner |
    +--------------------------------------+---------------------------------------------------------------------------------+--------------------------+
    | 4f82e9a3-5044-4d31-96c1-f0128fb8ff77 | {"subnet_id": "ea64b97a-626a-436a-a474-fa0dc082a80a", "ip_address": "10.0.0.1"} | network:router_interface |
    vagrant@devstack-single:~ > neutron port-update 4f82e9a3-5044-4d31-96c1-f0128fb8ff77 --device_owner dummy_owner
    Updated port: 4f82e9a3-5044-4d31-96c1-f0128fb8ff77

The condition that avoids the router deletion does not exist anymore. Hence, you can switch back to admin user and delete the router:

     vagrant@devstack-single:~ > export OS_USERNAME=admin
     vagrant@devstack-single:~ > neutron router-delete 83d2c3d7-1665-41a1-99cf-9e6dfc24dcb7
     Deleted router: 83d2c3d7-1665-41a1-99cf-9e6dfc24dcb7

From the point of view of the raw user, the port still exist, and with the same device_id:

    vagrant@devstack-single:~ > export OS_USERNAME=demo
    vagrant@devstack-single:~ > neutron port-list -c id -c fixed_ips -c device_owner -c device_id
    +--------------------------------------+---------------------------------------------------------------------------------+--------------+--------------------------------------+
    | id | fixed_ips | device_owner | device_id |
    +--------------------------------------+---------------------------------------------------------------------------------+--------------+--------------------------------------+
    | 4f82e9a3-5044-4d31-96c1-f0128fb8ff77 | {"subnet_id": "ea64b97a-626a-436a-a474-fa0dc082a80a", "ip_address": "10.0.0.1"} | dummy_owner | 83d2c3d7-1665-41a1-99cf-9e6dfc24dcb7 |
    +--------------------------------------+---------------------------------------------------------------------------------+--------------+--------------------------------------+

I would suggest:

* Don't let edit the device_owner field
* Modify the first chunck of code to count ports based on device id instead of device_owner

or both...

Tags: api

CVE References

Jaume Devesa (devvesa)
description: updated
Vishal Agarwal (vishala)
Changed in neutron:
assignee: nobody → Vishal Agarwal (vishala)
Revision history for this message
yong sheng gong (gongysh) wrote :

device_owner should be editable, it is important for us to implement 'attach interface to vm' in nova.

Revision history for this message
yong sheng gong (gongysh) wrote :

But I think we should prevent user from updating device_id device_owner of port if the port is not for nova's VM.

Revision history for this message
Jaume Devesa (devvesa) wrote :

yong:

if the device_owner has to be editable, we can try just modify the filter. The tenant_id of the port attached to the router is the tenant id of the user that can modify the device_owner. The router_gateway port does not belong to any tenant. So maybe changing the filter to:

       device_filter = {'device_id': [router_id],
                                     'tenant_ids': [list of tenant ids with access to the router]}
        port_count = self._core_plugin.get_ports_count(
            admin_ctx, filters=device_filter)

Revision history for this message
Eugene Nikanorov (enikanorov) wrote :

Can this be fixed with changing policy.json?

tags: added: api
Changed in neutron:
importance: Undecided → Medium
status: New → Confirmed
Revision history for this message
Vishal Agarwal (vishala) wrote :

Initially I started fixing the bug by modifying the existing condition and making it to:

rport_qry = context.session.query(models_v2.Port)
        port = rport_qry.filter_by(
                device_id=router_id,
                device_owner!=DEVICE_OWNER_ROUTER_GW).one()

But on a second thought if we give user the power of modifying the device_owner, I can see many other things break which includes router-port-list, router-interface-delete etc. Code heavily relies on the device_owner value.

Please suggest how to take this forward. Should the device_owner field be checked upon before modification (so that it falls under permissible values).

Revision history for this message
Carl Baldwin (carl-baldwin) wrote :

I started down the path of not allowing editing of device_owner a few months ago when CVE-2014-0056 came out. I'm glad someone is picking this up.

The problem that I ran in to at the time was what gongysh pointed out. It appears that Nova will sometimes use admin for this but not always. See this code snippet from nova/network/neutronv2/api.py: allocate_for_instance():

            port_req_body = {'port': {'device_id': instance['uuid'],
                                      'device_owner': zone}}
            try:
                port = ports.get(network_id)
                self._populate_neutron_extension_values(instance,
                                                        port_req_body)
                # Requires admin creds to set port bindings
                port_client = (neutron if not
                               self._has_port_binding_extension() else
                               neutronv2.get_client(context, admin=True))
                if port:
                    port_client.update_port(port['id'], port_req_body)
                    touched_port_ids.append(port['id'])
                else:
                    created_port_ids.append(self._create_port(
                            port_client, instance, network_id,
                            port_req_body, fixed_ips.get(network_id),
                            security_group_ids, available_macs, dhcp_opts))

This is where I stopped and ran out of time to drive this to conclusion.

So, maybe nova could always use admin? Even if nova changed, there would be a period of time where Neutron should allow this for older versions of Nova because we can't assume that nova will be upgraded at the same time Neutron is.

I'm not too keen on an implementation that allows editing of some but not others.

Revision history for this message
Jaume Devesa (devvesa) wrote :

Hi Vishal,

have you taken into account my proposal of filter by tenant_id instead of device owner?

device_filter = {'device_id': [router_id],
                                     'tenant_ids': [list of tenant ids with access to the router]}

Revision history for this message
Mark McClain (markmcclain) wrote :

device_owner should remain editable for the v2 API. There is another fix in the works that closes this loophole better.

Revision history for this message
Eugene Nikanorov (enikanorov) wrote :

Closing as Invalid per comments.

Changed in neutron:
status: Confirmed → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.