[OSSA 2015-012] Adding 0.0.0.0/0 to allowed address pairs breaks l2 agent (CVE-2015-3221)

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.

I can confirm this gives users the power to break chunks of an openstack cloud. Basically, if the network where the port where the bogus address is called "x", every host with a port on network "x" will pretty much stop working, as they will be unable to boot VMs, apply security group updates, etc.

I'm going to see if I can work a quick patch to do some validation checking here, so assigning to myself at the moment.

What was the original intention by the author of this bug? Is adding 0.0.0.0/0 as allowed address fullfilling a genuine use case? Or was a 'malicious' input?

If the former case, validation aimed at preventing the input might not be desirable, and we need to delve into this further to come up with a suitable solution.

Armando, look at the description, it looks like it's invalid input per the ipset manpage:

This does not work - the ipset man page says that zero prefix size is not allowed for type hash:net.

Also here: http://ipset.netfilter.org/ipset.man.html (search for "Hash:Net").

The use case is running a router in a VM. But the anti-spoofing will stop the router forwarding packets with source IP addresses not on the router ports or allowed address pairs. I'm using an openstack version that does not have the portsecurity extension.

A router is expected to forward IP packets from anywhere, so I tried to add all IPv4 address with a subnet with prefix length zero. The API allows subnets, so why not?

If you accept that 0.0.0.0/0 is valid for the API, then one solution would be to apply it behind the scenes as a special case using 2 ipset rules with prefix length one - 0.0.0.0/1 and 128.0.0.0/1. Otherwise we can just reject it at the API level, and users will still be able to allow all with
neutron port-update $PORT_ID --allowed_address_pairs list=true type=dict ip_address=0.0.0.0/1 ip_address=128.0.0.0/1

Cool, that's what I wanted to hear. Thanks Darragh!

I like that solution Darragh. I think we should allow 0.0.0.0/0 . I'm happy to implement this in the morning if you wanna pass it to me Kyle.

Thanks for volunteering Aaron! I was hoping to get some time to do this, but I failed miserably yesterday afternoon. I've assigned it to you now, I'll circle back tomorrow with you as well.

It seems to affect Juno and Icehouse too

@Darragh O'Reilly, do you know off hand what the address work around would need to be for ipv6? It looks like ::/0 triggers the same issue.

::/1' and '8000::/1' ?

Let me know what you guys think of this. I tested it and seems to do the trick.

I'll provide a backport Juno/icehouse after this gets a review.

Thanks for the quick turnaround Aaron! The workaround looks ok to me.

thanks Aaron. I'll give it a test run now.

* All neutron setups are affected right ?
* Darragh, your community profile says you're still working at HP, is this correct ?

Assuming yes at those two question, here is the impact description draft #1:

Title: Neutron L2 agent DoS through incorrect allowed address pairs
Reporter: Darragh O'Reilly (HP)
Products: Neutron
Affects: 2014.1 versions through 2014.1.4, and 2014.2 versions through 2014.2.3 and 2015.1.0 version

Description:
Darragh O'Reilly from HP reported a vulnerability in Neutron. By adding an invalid allowed address pairs, an authenticated user may crash the Neutron L2 agent resulting in a denial of service attack. All Neutron setups are affected.

Icehouse should not be affected by this, we introduced the ipset code in Juno. Thus, only 2014.2-2014.2.3 and 2015.1.0 are affected.

Ok great, moreover Icehouse might be EOLed by the time we disclose this... So the Affect line should read:

Affects: 2014.2 versions through 2014.2.3 and 2015.1.0 version

Tristan - yes I'm with HP.

I'm not usre if all Neutron setups are affected. Setups that use the IPTables firewall driver are. These include openvswitch, linuxbridge and ofagent agents - but there are probably others.

The patch works for me.

there is still a problem if you have prevent_arp_spoofing=True

2015-06-04 15:48:39.256 ERROR neutron.agent.common.ovs_lib [req-ed50624a-0797-42d0-9494-35608d0e1ca9 None None] Unable to execute ['ovs-ofctl', 'add-flows', 'br-int', '-']. Exception:
Command: ['ovs-ofctl', 'add-flows', 'br-int', '-']
Exit code: 1
Stdin: hard_timeout=0,idle_timeout=0,priority=2,arp,arp_spa=0.0.0.0/0,table=24,in_port=9,actions=normal
Stdout:
Stderr: ovs-ofctl: -:1: 0.0.0.0/0: network prefix bits not between 1 and 32

Might be a silly question, but is it sensible to have a device with 0.0.0.0/0 and at the same time have arp spoofing on?

Yup, I noticed that same issue. I actually pinged Kevin (he wrote the arp flow above) on it and he said it's not a problem. He has a patch to fix the error from installing the flow with 0.0.0.0.0/0 though was going to wait to push it to prevent from exposing this bug to other people who may notice it. Currently the code ignores errors raised there so it's just showing an error in the log but it doesn't matter

Can someone subscribe Kevin to this bug and ask him to attach his patch here ?

Subscribed Kevin. Kevin, can you attach the patch Aaron is referencing in comment #23?

Yeah, the patch I have isn't necessary for the security fix. It just fails to setup arp spoofing protection for the tenant but that's exactly what 0.0.0.0/0 in an allowed address pair does. It just prevents log noise.

Here is the patch, but I was planning on just uploading it after the fix for this vulnerability goes in.

okay, I guess it's best to keep the patch for vulnerability small, and the arp patch can be done separately.

Thanks Kevin, so considering the arp00000.patch fix something that is already broken, this can surely be proposed separately after the vulnerability patch is merged.

Arosen, do you think the backport is going to be difficult ?

Here is an updated impact description:

Title: Neutron L2 agent DoS through incorrect allowed address pairs
Reporter: Darragh O'Reilly (HP)
Products: Neutron
Affects: 2014.2 versions through 2014.2.3 and 2015.1.0 version

Description:
Darragh O'Reilly from HP reported a vulnerability in Neutron. By adding an invalid allowed address pairs, an authenticated user may crash the Neutron L2 agent resulting in a denial of service attack. Neutron setups using the IPTables firewall driver are affected.

@Tristan, Here's the backport for kilo. There was a small conflict but it's good.

Should be "By adding an invalid allowed address pair" (not "pairs"). Also the term "invalid" seems vague here without identifying what's qualifying that validity. Clearly the Neutron API thinks a 0-length mask is valid (until this bug is fixed), but ipset does not. How about something like "By adding an address pair which is rejected as invalid by the ipset tool, ..."?

Jeremy good point. How about:

Darragh O'Reilly from HP reported a vulnerability in Neutron. By adding a valid allowed address pairs of 0.0.0.0/0, an authenticated user may crash the Neutron L2 agent resulting in a denial of service attack. This is cause by the ipset tool rejecting the input of 0.0.0.0/0.

Thanks for the review, we'll use Jeremy proposal in order to keep the technical details low in the description.

Can someone add series tasks for Neutron Kilo and Juno please ?

Title: Neutron L2 agent DoS through incorrect allowed address pairs
Reporter: Darragh O'Reilly (HP)
Products: Neutron
Affects: 2014.2 versions through 2014.2.3 and 2015.1.0 version

Description:
Darragh O'Reilly from HP reported a vulnerability in Neutron. By adding an address pair which is rejected as invalid by the ipset tool, an authenticated user may crash the Neutron L2 agent resulting in a denial of service attack. Neutron setups using the IPTables firewall driver are affected.

The updated impact description in comment #34 looks good to me. Thanks!

Doh, I didn't did do it for juno. In juno the ipset manager was part of the iptables_firewall diver :( . Looking now to see how hard this is going to be.

Added Juno and Kilo series.

I spend a few hours on the juno backport of this patch. Unfortunately in juno the ipset manager is combined in with the iptables manager and I'll need to put the check in several places (and refactor the tests back in). It will probably take me 4 more hours or so to fix this and test the code with juno. I don't have the time to do that today though.

Do you guys think it would be okay to change the default option in juno to disable ipsets and to log a warning message if someone has them enabled?

Aaron - how about rejecting /0 in Juno?

That works for me. Though we'll also have to tell people that they need to delete any 0.0.0.0/0 rules they already have installed.

Aaron, do you mean that Juno user could have such 0/0 addr pair without breaking their L2 agent ?

About the patch, would 0.0.0.0/00 bypass the check and still break ipset ?

Also, please include a commit message and use git format-patch for attached fix here.

Will do,

I just want to make sure that is an acceptable fix for anyone running juno. The proposed solution is to prevent the address pair 0.0.0.0/0 from being installed in juno and reject it at the api layer. If we do this one still has to delete any 0.0.0.0/0 rules that have been installed. Alternatively, we could also say do not use the ipset driver in juno and use the iptables one. What do you guys think?

I think saying not to use ipset is risky if someone misses the notice. I'm for the simple block at the API layer. Maybe we can publish a small SQL query with the release to run to clear out any 0.0.0.0/0 entries?

K,

Okay I'll push a patch based on Darragh's tomorrow to do this for just juno. I think it's better to delete the address pairs that are /0 via the neutron api directly rather than targeting the db directly.

Tristan, yes, a user can have a port with an allowed address pair with ip_address ending in /0 without breaking the L2 agent, if none of the port's security groups have their remote_group_id set. If a user has a port like this, and a patch to block new /0 address pairs is applied, the user could still break the agent by changing the port's security groups.

I'm not sure about deleting them - this could affect users. Maybe we could supply a python script for operators that would search for ports with /0 and update them with 2x /1 ?

I like the idea of having a clean-up script, could it be part of the patch, and put it in neutron/tools ?

For Juno, the risk is to break a previously valid workflow. I can't tell if using /0 address pair on a port without its security_group's remote_group_id set is a realistic use-case. Maybe a good documentation note for Juno user is acceptable, thought ?

Can someone confirm Juno is really affected ?

Trying to investigate if a fix in _validate_allowed_address_pairs would work, I realize the bug does not reproduce in my devstack setup.

In q-agt log, there is the TRACE neutron.plugins.openvswitch.agent.ovs_neutron_agent Stderr: 'ipset v6.20.1: The value of the CIDR parameter of the IP address is invalid\n'
but instance connectivity is not impacted.

Right but I think new instances you spin up won't get networking on that host ?

I'm setting up juno now to test and nail this down aas well.

Darragh O'Reilly (darragh-oreilly) so you're saying that your patch for juno isn't enough to prevent this?

Okay, here's the patch for juno. What do you guys think? I provided a tool to update addresses that are 0.0.0.0/0 to the two address pairs.

Thanks Aaron, other than a typo in the name of the script, I'd say it looks good!

Doh,

fixed.

The proposed patch for Juno doesn't catch 0.0.0.0/00...

Also, we are concerned about this behavior change for Juno, can the same patch logic be used within the validate function (e.g. deflate /0 address into two /1), wouldn't that work and be transparent for Juno users ?

Aaron - were you able to confirm that is a security issue with Juno? Tristan was not able to rcreate it. I don't have much time, but I can try Juno later.

@Tristan, the problem is that there are several places that need this deflate method in Juno. I think disabling /0 allowed address pairs is an okay hack to solve this security issue for juno. We can always a better fix later (though gerrit which will be easier to test). It does break an API workflow people could have previously been using, that said the dataplane slide of it would not have worked in juno because of this bug.

@Darragh - yes this issue exists in juno with the ip set driver.

Updated patchset to account for /00, I also added in the note about renabling /0 for juno once this lands upstream.

@neutron-coresec, please review proposed fix (comments #13 for Liberty, #31 Kilo and #57 for Juno).

For Juno, since this bug prevented to use the zero_prefixed address pairs workflow correctly, I would say we can go with the proposed patch,

The note for affected Juno users would be: "Zero prefixed address pair are not accepted by the Juno API, users need to use 0.0.0.0/1 and 128.0.0.1/1 or ::/1' and '8000::/1. The fix_zero_length_ip_prefix.py tool can be used to clean ports that were mis-configured with a zero prefixed address"

Aaron - I can't seem to recreate the security issue with Juno. The l2-agent gives an error, but seems to work okay otherwise. I tried your juno patch and script and they worked for me.

FWIW, I couldn't reproduced it on Juno devstack with:

disable_service n-net
enable_service q-svc
enable_service q-agt
enable_service q-dhcp
enable_service q-l3
enable_service q-meta
enable_service q-fwaas
Q_SERVICE_PLUGIN_CLASSES=neutron.services.firewall.fwaas_plugin.FirewallPlugin
Q_USE_SECGROUP=True

The ipset command do fails but it does not prevent new instances to boot nor impact previous instance connectivity.

@Tristan, what if you restart the l2-agent after you have the 0.0.0.0/0 rule installed?

It looks to me that, that is going to prevent the correct rules from getting initially pushed down again.

trying to get my juno instance back up to verify as well.

When neutron-openvswitch-agent is restarted (q-agt devstack tab), it reprint the ipset trace, but it does not break like in Kilo. Though it may have other unnoticed effects...

@Tristan, unfortunately the bulk add of the rules won't get added correctly so the security rules won't correctly be applied anymore :(. New rules though are correctly applied but the ones needed before reset aren't. We'll need the juno patch as well to prevent this from occurring.

@Aaron, fair enough.

Since patch have been approved, here is the proposed public disclosure date/time:
2015-06-23, 1500UTC

Sounds good. Sorry for all the back and forth here!

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/194696

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/194697

Related fix proposed to branch: stable/kilo
Review: https://review.openstack.org/194742

Reviewed: https://review.openstack.org/194696
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=d9c78880d2b7e5b0544e821c45e6ad3700a06b9a
Submitter: Jenkins
Branch: stable/juno

commit d9c78880d2b7e5b0544e821c45e6ad3700a06b9a
Author: Aaron Rosen <email address hidden>
Date: Thu Jun 11 13:58:16 2015 -0700

    Disable allowed_address_pair ip 0.0.0.0/0 ::/0 for ipset

    Previously, the ipset_manager would pass in 0.0.0.0/0 or ::/0 if
    these addresses were inputted as allowed address pairs. This causes
    ipset to raise an error as it does not work with zero prefix sizes.
    To solve this problem we use two ipset rules to represent this.

    This was correctly fixed in a backport to kilo though we did not have the
    cycles to backport this exact fix to juno as in juno additional work needs to
    be done because the iptable and ipset code are interleaved together. This
    patch fixes this issue by disabling one from creating an address pair of
    zero lenght. This patch also provides a small tool which one should run:
    tools/fix_zero_length_ip_prefix.py which changes all zero length address_pair
    rules into two address pair rules of:

    Ipv4: 0.0.0.0/1 and 128.0.0.1/1
    IPv6: ::/1' and '8000::/1

    to avoid the problem.
    After this patch is merged into juno it will be easier for us to apply
    a better change to allow /0 addresses again in juno.

    Change-Id: I8c6a08e0cf3b5b5386fe03af9f2174c666b8ac75
    Closes-bug: 1461054
    Co-Authored-by: Darragh O'Reilly <email address hidden>

Change abandoned by Cedric Brandily (<email address hidden>) on branch: master
Review: https://review.openstack.org/195228

Related fix proposed to branch: feature/qos
Review: https://review.openstack.org/196097

Reviewed: https://review.openstack.org/196097
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=1cfed745d54a6ce9cb3dd4e6f454666d9e6676c2
Submitter: Jenkins
Branch: feature/qos

commit ba7d673d1ddd5bfa5aa1be5b26a59e9a8cd78a9f
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:31:38 2015 -0700

    Remove duplicated call to setup_coreplugin

    The test case for vlan_transparent was calling setup_coreplugin
    before calling the super setUp method which already calls
    setup_coreplugin. This was causing duplicate core plugin fixtures
    which resulted in patching the dhcp periodic check twice.

    Change-Id: Ide4efad42748e799d8e9c815480c8ffa94b27b38
    Partial-Bug: #1468998

commit e64062efa3b793f7c4ce4ab9e62918af4f1bfcc9
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:29:37 2015 -0700

    Remove double mock of dhcp agent periodic check

    The test case for the periodic check was patching a target
    that the core plugin fixture already patched out. This removes
    that and exposes the mock from the fixture so the test case
    can reference it.

    Change-Id: I3adee6a875c497e070db4198567b52aa16b81ce8
    Partial-Bug: #1468998

commit 25ae0429a713143d42f626dd59ed4514ba25820c
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:24:10 2015 -0700

    Remove double fanout mock

    The test_mech_driver was duplicating a fanout mock already setup
    in the setUp routine.

    Change-Id: I5b88dff13113d55c72241d3d5025791a76672ac2
    Partial-Bug: #1468998

commit 993771556332d9b6bbf7eb3f0300cf9d8a2cb464
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 17:55:16 2015 -0700

    Remove double callback manager mocks

    setup_test_registry_instance() in the base test case class gives
    each test its own registry by mocking out the get_callback_manager.
    The L3 agent test cases were duplicating this.

    Partial-Bug: #1468998
    Change-Id: I7356daa846524611e9f92365939e8ad15d1e1cd8

commit 0be1efad93734f11cd63fb3b7bd2983442ce1268
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 16:57:30 2015 -0700

    Remove ensure_dirs double-patch

    test_spawn_radvd called mock.patch on ensure_dirs after the
    setup method already patched it out. This causes issues when
    mock.patch.stopall() is called because the mocks are stored
    as a set and are unwound in a non-deterministic fashion.[1]
    So some of the time they will be undone correctly, but others
    will leave a monkey-patched in mock, causing the ensure_dir
    test to fail.

    1. http://bugs.python.org/issue21239

    Closes-Bug: #1467908
    Change-Id: I321b5fed71dc73bd19b5099311c6f43640726cd4

commit 0a2238e34e72c17ca8a75e36b1f56e41a3ece74e
Author: Sukhdev Kapur <email address hidden>
Date: Thu Jun 25 15:11:28 2015 -0700

    Fix tenant-id in Arista ML2 driver to support HA router

    When HA router is created, the framework creates a network and does
    not specify the tenant-id. This casuse Arista ML2 driver to fail.
    This patch sets the tenant-id when it is not passed explicitly by
    by the network_create() call from the HA r...

Related fix proposed to branch: feature/pecan
Review: https://review.openstack.org/196701

Change abandoned by Kyle Mestery (<email address hidden>) on branch: feature/pecan
Review: https://review.openstack.org/196701
Reason: This is lacking the functional fix [1], so I'll propose a new merge commit which includes that one.

[1] https://review.openstack.org/#/c/196711/

Related fix proposed to branch: feature/pecan
Review: https://review.openstack.org/196920

Reviewed: https://review.openstack.org/196920
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=7f759c077f8f860c13db92d2ea6b353ef6b70900
Submitter: Jenkins
Branch: feature/pecan

commit 8123144fadd7c5d5e6e56a76ea860512619a2cf6
Author: Moshe Levi <email address hidden>
Date: Sun Jun 28 14:37:14 2015 +0300

    Fix Consolidate sriov agent and driver code

    This patch add mising __init to mech_sriov/mech_driver/
    and update the setup.cfg to the new agent entrypoint

    Trivial Fix

    Change-Id: I53a527081feb78472f496675bbb3c5121d38a14a

commit 8942fccf02e6e179d47582fdb2792a1ca972da21
Author: Assaf Muller <email address hidden>
Date: Mon Jun 29 11:38:51 2015 -0400

    Remove failing SafeFixture tests

    The fixtures 1.3 release attempted to fix the fixtures resource
    leak issue, but failed to do so completely. Our own SafeFixture
    is still needed: The 1.3 release broke our SafeFixture tests,
    but not the usage of SafeFixture itself. This patch removes
    those failing tests for now to unbreak the gate. Jakub reported
    a bug on fixtures 1.3:
    https://bugs.launchpad.net/python-fixtures/+bug/1469759

    We will continue to use SafeFixture until that bug is fixed
    in fixtures, at which point we will be able to require
    fixtures > 1.3.

    Change-Id: I59457c3bb198ff86d5ad55a1e623d008f0034b8f
    Closes-Bug: #1469734

commit 71dffb0a2c1720cd8233a329d32958a0160dd6f5
Author: Kevin Benton <email address hidden>
Date: Mon Jun 29 08:27:41 2015 +0000

    Revert "Removed test_lib module"

    This reverts commit 9a6536de6e1a7fe9b2552adc142e254426b82b6f.

    We pulled all of the plugins out of the tree, many of which still inherit
    from neutron test classes. This change then stated that we no longer
    support testing other plugins. I think this is a bit premature and should
    have been discussed under the subject
    "Neutron plugins can't use neutron plugin unit tests" or something
    similar.

    Change-Id: I68318589f010b731574ea3bfa8df98492bab31fc

commit b20fd81dbd497e058384a0af065dd0f1fdc4c728
Author: Jakub Libosvar <email address hidden>
Date: Fri Jun 5 14:32:51 2015 +0000

    Refactor NetcatTester class

    Following capabilities were added:
       - used transport protocol is passed as a constant instead of bool
       - src port for testing was added
       - connection can be established explicitly
       - change constructor parameters of NetcatTester

    As a part of removing bool for protocol definition
    get_free_namespace_port() was also modified to match the behavior.

    Change-Id: Id2ec322e7f731c05a3754a65411c9a5d8b258126

commit 83e37980dcd0b2bad6d64dd2cb23bcd2891cafca
Author: jingliuqing <email address hidden>
Date: Sat Jun 27 13:41:54 2015 +0800

    Use REST rather than ReST

    Change-Id: I06c9deaab58c5ec13bfeec39fb8fd4b1fe21f42d

commit 1b60df85ba3ad442c2e4e7e52538e1b9a1bf9378
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:34:38 2015 -0700

    Add a double-mock guard to the base test case

    Use mock to patch mock with a check to prevent multiple active
    patches to the...

Reviewed: https://review.openstack.org/194697
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=9ff6138c47c95034ba845e9448ddffd147b51f38
Submitter: Jenkins
Branch: stable/kilo

commit 9ff6138c47c95034ba845e9448ddffd147b51f38
Author: Aaron Rosen <email address hidden>
Date: Wed Jun 3 16:19:39 2015 -0700

    Provide work around for 0.0.0.0/0 ::/0 for ipset

    Previously, the ipset_manager would pass in 0.0.0.0/0 or ::/0 if
    these addresses were inputted as allowed address pairs. This causes
    ipset to raise an error as it does not work with zero prefix sizes.
    To solve this problem we use two ipset rules to represent this:

    Ipv4: 0.0.0.0/1 and 128.0.0.1/1
    IPv6: ::/1' and '8000::/1

    All of this logic is handled via _sanitize_addresses() in the ipset_manager
    which is called to convert the input.

    Conflicts:
     neutron/agent/linux/ipset_manager.py
     neutron/tests/unit/agent/linux/test_ipset_manager.py

    Change-Id: I8c6a08e0cf3b5b5386fe03af9f2174c666b8ac75
    Closes-bug: 1461054

Reviewed: https://review.openstack.org/194742
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=88d76bc37f242ab89865cbe4e25cc5e928495d91
Submitter: Jenkins
Branch: stable/kilo

commit 88d76bc37f242ab89865cbe4e25cc5e928495d91
Author: Kevin Benton <email address hidden>
Date: Wed Jun 3 15:20:27 2015 -0700

    Skip ARP protection if 0.0.0.0/0 in addr pairs

    Don't setup ARP protection on ports with allowed address pairs
    that allow them to use any IP address. This is necessary because
    OVS doesn't support the /0 prefix in rules that match on ARP headers.

    Related-Bug: #1461054
    Closes-Bug: #1468009
    Change-Id: I913a86f22b228aa11fa3dabd9493c3995198f7ec
    (cherry picked from commit 747738d36572079307f228a861a067ca0cd815c2)

only this works for me
1.0.0.0.0/1
128.0.0.0/1

This solution did not worked for me.
0.0.0.0.0/1
128.0.0.0/1