[OSSA 2014-008] Routers can be cross plugged by other tenants (CVE-2014-0056)

[Aaron Rosen]

The l3-agent does not check tenant_id and allows for tenants to be able to plug ports into other's routers if the device_id is set to another tenants router.

# become admin tenant
arosen@arosen-desktop:~/devstack$ source openrc admin admin

# Create router as admin:
arosen@arosen-desktop:~/devstack$ neutron router-create admin-router
Created a new router:
+-----------------------+--------------------------------------+
| Field | Value |
+-----------------------+--------------------------------------+
| admin_state_up | True |
| external_gateway_info | |
| id | 80ffe19a-649c-4fc9-a0d9-2a3d67c5f600 |
| name | admin-router |
| status | ACTIVE |
| tenant_id | 04e94acfe69f4960a69c6a78d39466c4 |
+-----------------------+--------------------------------------+

# Become demo tenant
arosen@arosen-desktop:~/devstack$ source openrc demo demo

#create port with correct device_id and device_owner

arosen@arosen-desktop:~/devstack$ neutron port-create private --device-id 80ffe19a-649c-4fc9-a0d9-2a3d67c5f600 --device-owner network:router_interface
Created a new port:
+-----------------------+---------------------------------------------------------------------------------+
| Field | Value |
+-----------------------+---------------------------------------------------------------------------------+
| admin_state_up | True |
| allowed_address_pairs | |
| device_id | 80ffe19a-649c-4fc9-a0d9-2a3d67c5f600 |
| device_owner | network:router_interface |
| fixed_ips | {"subnet_id": "5786a0a6-24c8-4156-b981-cc817011c6a7", "ip_address": "10.0.0.3"} |
| id | 895cf428-4bfb-4c79-86c2-d40af9bf3587 |
| mac_address | fa:16:3e:21:33:6c |
| name | |
| network_id | 4de8b4f6-ac11-4836-aefb-7ed4f49ab9a7 |
| security_groups | |
| status | DOWN |
| tenant_id | ad069ea620614cce9c4b6f088d39d03e |
+-----------------------+---------------------------------------------------------------------------------+

Now when the l3-agent is restarted or enters its periodic sync state:

arosen@arosen-desktop:~/devstack$ sudo ip netns exec qrouter-80ffe19a-649c-4fc9-a0d9-2a3d67c5f600 ifconfig
lo Link encap:Local Loopback
          inet addr:127.0.0.1 Mask:255.0.0.0
          inet6 addr: ::1/128 Scope:Host
          UP LOOPBACK RUNNING MTU:16436 Metric:1
          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
          TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0
          RX bytes:0 (0.0 B) TX bytes:0 (0.0 B)

qr-895cf428-4b Link encap:Ethernet HWaddr fa:16:3e:21:33:6c
          inet addr:10.0.0.3 Bcast:10.0.0.255 Mask:255.255.255.0
          inet6 addr: fe80::f816:3eff:fe21:336c/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
          RX packets:4 errors:0 dropped:0 overruns:0 frame:0
          TX packets:5 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0
          RX bytes:300 (300.0 B) TX bytes:398 (398.0 B)

[Aaron Rosen (arosen)]

Note the above patch applies cleanly to stable/havana as well.

[Aaron Rosen (arosen)]

Fix for grizzly

[Mark McClain (markmcclain)]

I've confirmed the vulnerabilities and am testing the fix.

[Jeremy Stanley (fungi)]

Mark, it looks (based on the backport patches) that we need Neutron series tasks for Grizzly and Havana added to the bug for tracking purposes, when you get a moment.

[Thierry Carrez (ttx)]

Here you go

[Thierry Carrez (ttx)]

@neutron-core: please approve proposed patches -- feel free to add anyone relevant for that review by subscribing them to the bug

[Akihiro Motoki (amotoki)]

I reviewed the fix and confirmed it works as expected.

The change looks good (it is worth +2).
One minor nit on a comment.

> neutron/tests/unit/test_l3_plugin.py
> class L3AgentDbTestCaseBase(L3NatTestCaseMixin):
> + # create a network owned by a normal tenant

I think it is better to change "a normal tenant" to "another tenant".
router is created by a default tenant ("test-tenant" is the default tenant is UT)
and 'tenant' tenant is a different tenant.

(Beyond this fix, it may be better to discuss who can specify/update device_owner/device_id? Now anyone can update device_id/device_owner of his/her ports, but it may be better only admin can specify them.)

[Aaron Rosen (arosen)]

Hi Akihiro,

Thanks for reviewing! On second thought I think that my patch does break/change a current behavior. Currently in neutron we support (or maybe not intentionally but ... but the api accepts it...) as the admin user can attach other tenants subnets to routers. With my patch this functionality no longer works for plugins using the L3 agent. I'm not sure we really want to support this type of thing though. We have this same issue all over though.

For example, as admin a tenant can attach another tenants security_group to another tenants ports (thus that tenant won't be able to see the rules in the profile.) Same with floatingips.

I personally think we should remove this type of functionality though. I'm not sure there is a large use case for two different tenants to be connected to the same router (though I think others would probably disagree).

An alternative approach we can use to solve this problem is to not allow create/update_port to be called with device_owner=("network:router_interface") in order to do this though we'll have to add another param to create/update_port to account for when the create port call comes from router-interface-add.

 What are you thoughts?

[Akihiro Motoki (amotoki)]

Hi Aaron,

Ah.. Good catch. I agree that we need to use another approach. I am not sure there is a case where multiple tenants connects to one router too.

Your alternative approach to not allow create/update_port to be called with device_owner router_interface sounds reasonable.
In general device_id and device_owner attributes of ports created by Neutron internally should not be specified by a regular user. I think it is no problem admin user can specify device_owner router_interface.

An approach in my mind is a bit different from your original one. It is to not allow create/update_port with device_owner router_interface **for normal users.** In this approach we can use elevated context for create_port from router-interface-add and an additional parameter is unnecessary.

Thought?

[yong sheng gong (gongysh)]

we can add a way to do verification. for example, we can add a hook registry on port update and creation according to field and operation type.

L3 service plugin should add a hook into the registry. the device_owner and device_id hook will call the hook, which will do the verification.

[Thierry Carrez (ttx)]

We need a second neutron-core +2 on those before we can move on

[Akihiro Motoki (amotoki)]

@ttx As Aaron's comment #9 says, the first approach has a problem, so I remove my +2. We need another patch.

[Grant Murphy (gmurphy)]

Any progress on a new patch for this?

[Aaron Rosen (arosen)]

Grant - I'll upload a patch to launchpad tomorrow that addresses this issue.

[Aaron Rosen (arosen)]

Hi Akihiro

I took your approach of using context.elevated() thus this will still work for admin users. Let me know what you think.

Thanks Aaron

[Aaron Rosen (arosen)]

Whoops forgot to upload the grizzly backport as well. It would be great if we could get these reviewed and the ball rolling on this. Thanks!

[Thierry Carrez (ttx)]

@neutron core developers: please review & test Aaron's patches ASAP !
Post your approval on this bug only -- this is an embargoed vulnerability.

[Akihiro Motoki (amotoki)]

Thanks Aaron for the updated patch.

-1 from me.

Checking device_owner works as expected, but we need to check device_id too. I tested the following scenario and succeeded a router interface to admin router with demo tenant. In this scenario a regular tenant (demo) first adds a router interface to its router and then changes device_id of the router interface to a router of another tenant (admin).

I think we need to check if a tenant can access a router with a specified device_id (= router_id) if (new) device_owner is network:router_interface unless a tenant is admin.

-----
[admin tenant]
neutron net-create admin-net1
neutron subnet-create --name admin-subnet1 admin-net1 10.2.2.0/24
neutron router-create admin-router
# make sure to schedule router to l3-agent
neutron router-interface-add admin-router admin-subnet1

[demo tenant]
neutron net-create net1
neutron subnet-create --name subnet1 net1 10.1.1.0/24
neutron router-interface-add router1 subnet1

# Change device_id of the above router interface to admin_router
neutron port-update <port-id of router1 interface> --device_id <admin-router ID>

After restarting l3-agent, I saw an interface corresponding to the router interface from demo tenant in admin-router namespace.

[restart

[yong sheng gong (gongysh)]

    add device_id and device_owner check func

    This patch provides an unified way to validate the device owner and
    device id to avoid cross plugging ports from other tenants.

    This patch just provides router interface check, but VIP, even
    nova instance owner can be easily implemented.

[yong sheng gong (gongysh)]

Hope it does not make the thing complicated, but I think my patch provides an alternative (maybe better) way to this issue.

thanks

[yong sheng gong (gongysh)]

   add device_id and device_owner check func

    This patch provides an unified way to validate the device owner and
    device id to avoid cross plugging ports from other tenants.

    This patch just provides router interface check, but VIP, even
    nova instance owner can be easily implemented.

[yong sheng gong (gongysh)]

to use the patch, git apply 0001-add-device_id-and-device_owner-check-func.patch

[Thierry Carrez (ttx)]

Neutron core: does that patch look better to you ?

[Grant Murphy (gmurphy)]

Draft impact description.

----

Title: Routers can be cross plugged by other tenants
Reporter: Aaron Rosen (VMWare)
Products: Neutron
Affects: All supported versions

Description:
Aaron Rosen from VMWare reported a vulnerability where Neutron fails
to perform proper authorization checks when creating ports. By
choosing a device id of a router from a different tenant when
creating a port, an authenticated user can access the network
of other tenants. This affects all supported versions of Neutron.

[Thierry Carrez (ttx)]

If someone can confirm that this affects all deployments of Neutron (whatever the plugin used), then impact desc looks fine.

[Grant Murphy (gmurphy)]

@neturon-core

Can you please confirm this latest patch is ok and also that it affects all deployments of neturon (as per ttx comment above).

[Armando Migliaccio (armando-migliaccio)]

@Grant: as per Aaron's commit message, the only Neutron deployments affected by this vulnerability are the ones that use plugins which rely on the the l3-agent. I'll give a look at the patch once more to confirm the patch addresses the issue without side effects.

Thanks,
Armando

[Grant Murphy (gmurphy)]

Thanks. How about?

----

Title: Routers can be cross plugged by other tenants
Reporter: Aaron Rosen (VMWare)
Products: Neutron
Affects: All supported versions

Description:
Aaron Rosen from VMWare reported a vulnerability where Neutron fails
to perform proper authorization checks when creating ports. By
choosing a device id of a router from a different tenant when
creating a port, an authenticated user can access the network
of other tenants. This affects deployments of Neutron using plugins
relying on the l3-agent.

---

I might also add Aaron's note (below) in the OSSA -

       One should perform and audit of the ports that are already
       attached to routers after applying this patch and remove ports
       that a tenant may have cross plugged.

---

If others are happy with that impact description I'll get a CVE assigned.

[Thierry Carrez (ttx)]

+1 on impact description. You can add Aaron's note in a separate "Note: " below description.

[Jeremy Stanley (fungi)]

Grant's latest proposed impact description in comment #30 looks good. Also, I agree on adding the note in the OSSA (though of course it's not needed for the CVE request text).

[Grant Murphy (gmurphy)]

Affects version - 2012.2 to 2013.1.4, and 2013.2 versions up to 2013.2.1

[Anita Kuno (anteaya)]

After talking with Mark McClain and Aaron Rosen on irc, I have learned that there was a call with Mark, Aaron and Salvatore where this bug was discussed and a better permanent solution was agreed upon. Aaron remembers the conversation and has not had the time to put it into code as of yet.

He told me that he is putting some time aside this week to address this bug. Thank you, Aaron.

[Aaron Rosen (arosen)]

Btw in the patch i needed to create a new admin_context otherwise the following exception would be raised:

 2014-03-18 12:15:06.119 ERROR neutron.api.v2.resource [req-946a316d-74f7-477e-bcfd-8e5070dcc7ea demo c166d9316f814891bcb66b96c4c891d6] update failed
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource Traceback (most recent call last):
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/opt/stack/neutron/neutron/api/v2/resource.py", line 87, in resource
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource result = method(request=request, **args)
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/opt/stack/neutron/neutron/api/v2/base.py", line 505, in update
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource obj = obj_updater(request.context, id, **kwargs)
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/opt/stack/neutron/neutron/plugins/ml2/plugin.py", line 656, in update_port
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource port)
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/opt/stack/neutron/neutron/db/db_base_plugin_v2.py", line 1417, in update_port
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource port.update(self._filter_non_model_columns(p, models_v2.Port))
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/usr/local/lib/python2.7/dist-packages/sqlalchemy/orm/session.py", line 447, in __exit__
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource self.rollback()
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/usr/local/lib/python2.7/dist-packages/sqlalchemy/util/langhelpers.py", line 61, in __exit__
2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource compat.reraise(type_, value, traceback)
2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/usr/local/lib/python2.7/dist-packages/sqlalchemy/orm/session.py", line 447, in __exit__
2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource self.rollback()
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/usr/local/lib/python2.7/dist-packages/sqlalchemy/orm/session.py", line 396, in rollback
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource self._restore_snapshot(dirty_only=self.nested)
2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource File "/usr/local/lib/python2.7/dist-packages/sqlalchemy/orm/session.py", line 246, in _restore_snapshot
 2014-03-18 12:15:06.119 TRACE neutron.api.v2.resource assert self._is_transaction_boundary

[Armando Migliaccio (armando-migliaccio)]

lgtm

[Aaron Rosen (arosen)]

Thanks for the review armando. I had to make a small change to this patch. Hopefully this one should be good too:

[Aaron Rosen (arosen)]

patch rebased on havana code base..

[Aaron Rosen (arosen)]

patch rebased on grizzly

[Aaron Rosen (arosen)]

@Grant Murphy -- you're description looks good to me thanks! Sorry this took a **ridiculous** about of time to fix.

[Thierry Carrez (ttx)]

@neutron-coresec, please +2 the proposed patches here

[Armando Migliaccio (armando-migliaccio)]

lgtm

[Akihiro Motoki (amotoki)]

@Aaron, thanks for the patch. The patch itself works.

One question: The patch checks there is a router with specified device_id and we can set non-existing UUID.
Even though the possibility is very very low, if other tenant creates a router with that UUID, the port is connected to the router. The possibility completely depends on randomness of UUID. It is different from the original case. The original case reports in this bug is if someone can know UUID of other tenant routers, he/she can connect a port to other tenant's router. I am not sure we should cover the case I mentioned here in this bug report, but it is worth considered once.

The below is minor suggestions to improve the codes.
----
+class DeviceIDNotOwnedByTenant(Conflict):
+ message = _("The following device_id %(device_id)s is not owned by your "
+ "tenant or matches another tenants router.")

I think this exception is generic and not specific to "router".
How about passing "router" as a parameter.

  message = _("...... matches another tenants %(resource)s.")

-----
update_port()
- "new_device_owner" looks better than "current_device_owner" because a device_owner is not updated yet. IMO new and prev/old prefix looks better in update operation. "current_" is a bit ambiguous.
- changed_device_id is set to True when 'device_id' is not specified in update_port. This means if device_id is not specified in update_port changed_device_id would be set to True.

----
_enforce_device_owner_not_router_intf_or_device_id
- get_service_plugins() handles a case where core plugin supports L3. We can write it as follows:

if device_id:
    l3_plugin = (
        manager.NeturonManager.get_service_plugins().get(
            service_constants.L3_ROUTER_NAT))
    if l3_plugin:
        try:
            ctx_admin = ctx.get_admin_context()
            router = l3plugin.get_router(ctx_admin, device_id)
        except l3.RouterNotFound:
            return
    else:
        # raise as extension doesn't support L3 anyways.
        raise n_exc.DeviceIDNotOwnedByTenant(....)

[Mark McClain (markmcclain)]

+2 to releasing these security patches as they exist today. Once merged into master, we can openly address the exception and method name comments. Does this work?

[Salvatore Orlando (salvatore-orlando)]

+2 with the same comments as Mark.

Leaving my comment as well as I'm not sure Armando's 'lgtm' is techinically binding!

[Akihiro Motoki (amotoki)]

Mark, it works.
+2 from me too.

All my comments can be clean up in later patch.
My question is just talking about the possibility and it does not practically happen.

[Kyle Mestery (mestery)]

+2 from me as well, nice work on addressing this one @Aaron!

[Aaron Rosen (arosen)]

Thanks guys,

@Akihiro - Right it's possible for a router to be created that matches the uuid of another tenants router later down the road. That said this is pretty much impossible to occur though I think we should probably address this anyways after we merge this. I thought about this case and the reason I didn't address it here is we would need something that would regenerate uuid's on routers if there was a conflict similar to mac_addresses (which isn't possible today as some backends generate their uuid's themselves).

Ship it!

[Anita Kuno (anteaya)]

I've spoken to fungi who will talk to gmurphy who is the vmt member assigned to this bug.

Gmurphy will take the next steps and coordinate the release of this patch according to the steps found here: https://wiki.openstack.org/wiki/Vulnerability_Management

When it is time for disclosure of the patch, core members need to be available to review/approve it.

Thanks to all for their attention to this matter.

[Grant Murphy (gmurphy)]

pre-OSSA sent.

Proposed public disclosure date/time:
2014-03-27, 1500UTC

[Alan Pevec (apevec)]

Re. grizzly - last planned release 2013.1.5 was just released yesterday, does this require quick Quantum 2013.1.6 release?
Fungi planned to EOL stable/grizzly and remove grizzly CI jobs next week!

[Jeremy Stanley (fungi)]

I think it's safe to omit the grizzly patch from the pre-OSSA and advisory. The VMT was supposed to stop considering new stable/grizzly backports after I-3 (which was a couple weeks ago, March 6).

[Jeremy Stanley (fungi)]

Oh, I guess a stable/grizzly backport for this got included in the pre-OSSA, but I suppose we could still leave it out of the official advisory.

[Thierry Carrez (ttx)]

master https://review.openstack.org/#/c/83391/
havana https://review.openstack.org/#/c/83393/
grizzly https://review.openstack.org/#/c/83394/

[Thierry Carrez (ttx)]

[OSSA 2014-008]

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/83393
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=1faec8354a0fab953524eaeb6042ad38461a58bc
Submitter: Jenkins
Branch: stable/havana

commit 1faec8354a0fab953524eaeb6042ad38461a58bc
Author: Aaron Rosen <email address hidden>
Date: Wed Mar 26 16:36:56 2014 -0700

    Prevent cross plugging router ports from other tenants

    Previously, a tenant could plug an interface into another tenant's
    router if he knew their router_id by creating a port with the correct
    device_id and device_owner. This patch prevents this from occuring
    by preventing non-admin users from creating ports with device_owner
    network:router_interface with a device_id that matches another tenants router.
    In addition, it prevents one from updating a ports device_owner and device_id
    so that the device_id won't match another tenants router with device_owner
    being network:router_interface.

    NOTE: with this change it does open up the possiblity for a tenant to discover
    router_id's of another tenant's by guessing them and updating a port till
    a conflict occurs. That said, randomly guessing the router id would be hard
    and in theory should not matter if exposed. We also need to allow a tenant
    to update the device_id on network:router_interface ports as this would be
    used for by anyone using a vm as a service router. This issue will be fixed in
    another patch upstream as a db migration is required but since this needs
    to be backported to all stable branches this is not possible.

    NOTE: The only plugins affect by this are the ones that use the l3-agent.

    NOTE: **One should perform and audit of the ports that are already
            attached to routers after applying this patch and remove ports
            that a tenant may have cross plugged.**

    Closes-bug: #1243327

    Conflicts:
        neutron/common/exceptions.py
        neutron/db/db_base_plugin_v2.py

    Change-Id: I8bc6241f537d937e5729072dcc76871bf407cdb3

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/83391
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=843e60b7901deb3ecbff81c2e057b8d186b9fc34
Submitter: Jenkins
Branch: master

commit 843e60b7901deb3ecbff81c2e057b8d186b9fc34
Author: Aaron Rosen <email address hidden>
Date: Wed Mar 26 16:40:09 2014 -0700

    Prevent cross plugging router ports from other tenants

    Previously, a tenant could plug an interface into another tenant's
    router if he knew their router_id by creating a port with the correct
    device_id and device_owner. This patch prevents this from occuring
    by preventing non-admin users from creating ports with device_owner
    network:router_interface with a device_id that matches another tenants router.
    In addition, it prevents one from updating a ports device_owner and device_id
    so that the device_id won't match another tenants router with device_owner
    being network:router_interface.

    NOTE: with this change it does open up the possiblity for a tenant to discover
    router_id's of another tenant's by guessing them and updating a port till
    a conflict occurs. That said, randomly guessing the router id would be hard
    and in theory should not matter if exposed. We also need to allow a tenant
    to update the device_id on network:router_interface ports as this would be
    used for by anyone using a vm as a service router. This issue will be fixed in
    another patch upstream as a db migration is required but since this needs
    to be backported to all stable branches this is not possible.

    NOTE: The only plugins affect by this are the ones that use the l3-agent.

    NOTE: **One should perform and audit of the ports that are already
            attached to routers after applying this patch and remove ports
            that a tenant may have cross plugged.**

    Change-Id: I8bc6241f537d937e5729072dcc76871bf407cdb3
    Closes-bug: #1243327

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/153452

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/153452
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=89025a8dd93918ac2967726ec7bb8ee5aa22d924
Submitter: Jenkins
Branch: master

commit 89025a8dd93918ac2967726ec7bb8ee5aa22d924
Author: armando-migliaccio <email address hidden>
Date: Thu Feb 5 16:27:52 2015 -0800

    Fix lack of device ownership enforcement for DVR routers

    The enforcement rule was applied to centralized router interfaces, to avoid
    a potential security vulnerabilty.

    Even though DVR routers are fundamentally different from centralized routers,
    there is no good reason as to why the rule should be skipped for DVR interfaces.

    This patch sanitizes the insanity a bit and closes this potential loophole by
    preventing the operation for DVR routers too.

    Related-bug: #1243327
    Closes-bug: #1410984

    Change-Id: I048e6e3926e1c74cf9ecb63cfb53a0b1afb3c579

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/juno
Review: https://review.openstack.org/154355

[OpenStack Infra (hudson-openstack)]

Change abandoned by Armando Migliaccio (<email address hidden>) on branch: stable/juno
Review: https://review.openstack.org/154355