Neutron firewall anti-spoofing does not prevent ARP poisoning

Bug #1274034 reported by Édouard Thuleau
126
This bug affects 17 people
Affects Status Importance Assigned to Milestone
OpenStack Security Advisory
Invalid
Undecided
Unassigned
OpenStack Security Notes
Fix Released
Undecided
Tim Kelsey
neutron
Fix Released
High
Kevin Benton
Juno
Fix Released
Undecided
Unassigned
Kilo
Fix Released
Undecided
Unassigned

Bug Description

The neutron firewall driver 'iptabes_firawall' does not prevent ARP cache poisoning.
When anti-spoofing rules are handled by Nova, a list of rules was added through the libvirt network filter feature:
- no-mac-spoofing
- no-ip-spoofing
- no-arp-spoofing
- nova-no-nd-reflection
- allow-dhcp-server

Actually, the neutron firewall driver 'iptabes_firawall' handles only MAC and IP anti-spoofing rules.

This is a security vulnerability, especially on shared networks.

Reproduce an ARP cache poisoning and man in the middle:
- Create a private network/subnet 10.0.0.0/24
- Start 2 VM attached to that private network (VM1: IP 10.0.0.3, VM2: 10.0.0.4)
- Log on VM1 and install ettercap [1]
- Launch command: 'ettercap -T -w dump -M ARP /10.0.0.4/ // output:'
- Log on too on VM2 (with VNC/spice console) and ping google.fr => ping is ok
- Go back on VM1, and see the VM2's ping to google.fr going to the VM1 instead to be send directly to the network gateway and forwarded by the VM1 to the gw. The ICMP capture looks something like that [2]
- Go back to VM2 and check the ARP table => the MAC address associated to the GW is the MAC address of VM1

[1] http://ettercap.github.io/ettercap/
[2] http://paste.openstack.org/show/62112/

Jeremy Stanley (fungi)
Changed in neutron:
status: New → Incomplete
status: Incomplete → New
Changed in ossa:
status: New → Incomplete
description: updated
description: updated
Revision history for this message
Édouard Thuleau (ethuleau) wrote :

I propose a draft patch (without UT) that corrects this bug (attached to that bug).
iptables is not able to prevent the ARP cache poisoning but ebtables could.

My proposed patch implements an ebtables manager and add a 'NWFilterFirewall' class that iptables firewall driver instantiates when security are enabled on a port.
I move MAC and IP spoofing protection to that class and I propose to delegate all the fist hop security (FHS) rules to the 'NWFilterFirewall' class.

Note: The security groups mix-in [1] implements by default some provider default rules (DHCP and RA). Should we delegate that to the 'NWFilterFirewall' class?

I also want to point you other IPv6 FHS rules we should prevent too (especially for IPv6, I think we have enough for IPv4 protection(IP, MAC, ARP, DHCPv4)).
I link [2][3] CISCO documents that list this FHS rules that their equipments implement by default.

My proposed patch is rebased on the Nachi's review [4]. I modified the list of 'vif_security' attributes to:
- require_iptables: Neutron requires an external service to support iptables,
- prevent_spoofing: Neutron requires an external service to prevent base spoofing,
- require_securitygroup: Neutron requires an external service to support security group.
Any though?

For the backport, I though we could use together the 'NWFilterFirewall' [5] driver as firewall_driver for Nova and the 'IptablesFirewallDriver' as Neutron firewall driver.
I just made a rapid test but I think Nova will need a small patch to be able to do that (some mother class 'FirewallDriver' methods need to be implemented by the 'NWFilterFirewall' class). Any though?
This bug should impact Havana and Grizzly stable releases.

[1] neutron/db/securitygroups_rpc_base.py line 284
[2] http://www.cisco.com/en/US/prod/collateral/iosswrel/ps6537/ps6553/aag_c45-707354.pdf
[3] http://docwiki.cisco.com/wiki/FHS
[4] https://review.openstack.org/#/c/21946/
[5] https://github.com/openstack/nova/blob/master/nova/virt/libvirt/firewall.py#L35

Revision history for this message
Édouard Thuleau (ethuleau) wrote :
Revision history for this message
Thierry Carrez (ttx) wrote :

@Mark: could you confirme the issue, or subscribe someone who can ?

Changed in ossa:
status: Incomplete → Confirmed
status: Confirmed → Incomplete
Revision history for this message
Édouard Thuleau (ethuleau) wrote :

For the proposed backport solution, we lost 'Allow Address Pairs' extension functionality.

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

@ttx: This is an issue... the issue is whether is worthy of security fix or something we should just fix openly.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Traditionally, mechanisms for mitigating ARP spoofing/cache poisoning have been considered security features, not vulnerability fixes. The complicating factor here, as I read this, is that we have a lack of security feature parity from nova-network's behavior. If Neutron documentation or configuration explicitly say it implements filters for these then this is a vulnerability (security feature not working as advertised), otherwise I would lean toward still considering this a security hardening measure on Neutron's road toward achieving feature parity with nova-network.

Revision history for this message
Thierry Carrez (ttx) wrote :

I tend to agree with Jeremy's reasoning here.
@Mark: does that make sense ? (considering this a security hardening measure on Neutron's road toward achieving feature parity with nova-network ?)

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

Yeah we've never claimed to have mechanisms to mitigate ARP spoofing/cache poisoning especially. Shared networks, while useful, have lots of security tradeoffs when used. I think we address this in master and backport as normal to the stable branch.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Switched to public following discussion with Mark.

information type: Private Security → Public
tags: added: security
Changed in ossa:
status: Incomplete → Invalid
Changed in neutron:
status: New → Triaged
tags: added: l3-ipam-dhcp
tags: added: havana-backport-potential
Revision history for this message
Kevin Bringard (kbringard) wrote :

As another potential intermediate triage to this, we have added another spoofing rule to the spoof chains and call the spoof chain in the ingress rules. Effectively, if the DST IP address isn't the address we assigned to the VM, then drop it.

In this scenario, the malicious VM can can still poison the ARP cache and effectively DoS the victim, but it shouldn't be able to see any of the victim's traffic as the packets headed to the malicious VM will get dropped at the hypervisor.

I've attached a patch to the iptables_firewall.py from stable/havana which implements this. If folks don't disagree with this approach I'll submit the patch to be reviewed as well as look into getting it into icehouse (or maybe Juno, depending on if it's considered a new feature or bug fix).

Revision history for this message
Mathieu Rohon (mathieu-rohon) wrote :

this looks like a good patch for a backport in havana, but if a VM clains that it owns the gateway IP, other VMs of the network will loose connectivity to other networks. I think that the ebtablemanager proposed by edouard is the long term solution.

Revision history for this message
Kevin Bringard (kbringard) wrote :

Thanks for the feedback Mathieu, and I absolutely agree. It's meant to be a mitigation approach as it only filters L3 and prevents the malicious VM from actually gathering any data. They can still DoS the L2 segment by hijacking traffic destined for the gateway (or any other IP for that matter), and L2 filtering (which the ebtables approach would accomplish) is the only way to stop that aspect of it.

There are also a few other things we're looking at to help beef up this area. One thing would be working to implement more robust port-security like features in the ovs-plugin-agent. As of OVS 1.10 they've implemented a drop_spoofed_arp flow action, which, while wouldn't necessarily address this specific issue, probably couldn't hurt to put on ports.

Regardless, we're doing some more testing in house to make sure it doesn't have any unintended effects, but assuming that all goes well I'll work on getting this patch into havana/icehouse. The processing overhead of one more chain should have a miniscule effect on throughput, and it can't hurt to have another validation that the packets are indeed coming and going to the correct place.

Changed in neutron:
assignee: nobody → Kevin Bringard (kbringard)
status: Triaged → In Progress
Akihiro Motoki (amotoki)
tags: added: icehouse-backport-potential
Revision history for this message
Kevin Bringard (kbringard) wrote :

Review https://review.openstack.org/#/c/83845/ is in progress to add L3 destination filtering to the spoof chain. Once that gets accepted we can work on porting that back to icehouse and havana.

If anyone wants to go take a look at it and get it pushed through that'd be useful :-D

We'll still need to keep this bug open as triaged, because we need to implement true L2 filtering along the lines of what Édouard outlined in the initial report.

Revision history for this message
Xu Han Peng (xuhanp) wrote :

For your information, I registered blueprint [1] for RA guard and IPv6 snooping after discussing with IPv6 sub team.

[1] https://blueprints.launchpad.net/neutron/+spec/security-group-ipv6-ra-guard

Revision history for this message
Édouard Thuleau (ethuleau) wrote :

@Kevin: Thanks for your backportable patch. I still need to rebase and proposed my patch (some UT need to be coded)

@Xu Han Peng: Thanks to create that patch to prevent RA and FHS IPv6 directly to the egress traffic port.

When I writing my patch, I though it could be better to separate first hop security port (spoofing, ARP, DHCP, RA, ND...) to the security group. I think it's two different things. For example, actually, to protect DHCP spoofing, we add provider security group to the security group of a port. But that security group is not visible by the user.
To separate FHS to SG, we need to implement specific RPC calls between API servers and agents. It's a huge work.
Any thoughts ?

Changed in neutron:
assignee: Kevin Bringard (kbringard) → nobody
Changed in neutron:
assignee: nobody → Juergen Brendel (jbrendel)
Revision history for this message
Robert Clark (robert-clark) wrote :

I could have read this incorrectly but my understanding is that this failure in tenant network isolation is not being considered a severe security issue?

Revision history for this message
Jeremy Stanley (fungi) wrote :

I believe the assertion is that Neutron's flat networking implementation does not provide layer 2 filtering guarantees between tenants on the same broadcast domain, unlike Nova's, and that this is a design shortcoming/feature parity gap which should be better documented for existing releases and fixed in new releases as a priority feature for security hardening reasons.

Revision history for this message
Robert Clark (robert-clark) wrote :

Personally I think this is pretty severe as it's a failure to provide fundamental separation that is a base requirement of a multi-tenant compute platform.

However, I've added it to the OSSN queue to document the issue.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Mostly going on Mark's comment about the "security tradeoffs" of shared networks, suggesting that this should be a known risk to most network-savvy deployers and so we ought to make sure it's clearly documented... I agree the result, for an unsuspecting operator, is fairly severe, however I think it's better to have these design discussions out in the open (for instance, the current patch proposed to solve this for Juno is blocked on lack of a blueprint and design specification).

If it turns out that this risk can be mitigated cleanly on stable branches, to the satisfaction of the stable maintainers and Neutron core reviewers alike, then it might be possible to revisit an advisory. In the meantime, visibly documenting this issue can only help improve the security of our user base so that operators who have not already can take preventative action.

Kyle Mestery (mestery)
Changed in neutron:
importance: Undecided → High
Revision history for this message
Yaguang Tang (heut2008) wrote :

It's essential to have this feature available in Neutron ASAP, It‘s fundamental requirement for a cloud to have tenant isolation, could we have an exception to get this in the latest release ?

Tim Kelsey (tim-kelsey)
Changed in ossn:
assignee: nobody → Tim Kelsey (tim-kelsey)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/119031

Tim Kelsey (tim-kelsey)
Changed in ossn:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/119352

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Juergen Brendel (<email address hidden>) on branch: master
Review: https://review.openstack.org/119031
Reason: After feedback decided to replace this patch with two separate patches instead:

1. Implement ebtables manager:
      https://review.openstack.org/#/c/119343/

2. Implement ARP fix:
      https://review.openstack.org/#/c/119352/

Tim Kelsey (tim-kelsey)
Changed in ossn:
status: In Progress → Fix Committed
Revision history for this message
Nathan Kinder (nkinder) wrote :

This was published as OSSN-0027:

  https://wiki.openstack.org/wiki/OSSN/OSSN-0027

Changed in ossn:
status: Fix Committed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Salvatore Orlando (<email address hidden>) on branch: master
Review: https://review.openstack.org/83845
Reason: This patch has been inactive long enough that I think it's safe to abandon.
The author can resurrect it if needed.

Revision history for this message
Jian Wen (wenjianhn) wrote :

For anyone is interested in NWFilterFirewall in nova, see bug 1316621 (ebtables calls can race with libvirt).

Revision history for this message
Jian Wen (wenjianhn) wrote :

ethuleau,the patch for NWFilterFirewall is not small, because we need
 to support neutron addresses pair.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Kyle Mestery (<email address hidden>) on branch: master
Review: https://review.openstack.org/119352
Reason: This change is old enough and hasn't seen any updates since September 5, 2014. Abandoning it, please revive it if you plan to work on it again.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Kyle Mestery (<email address hidden>) on branch: master
Review: https://review.openstack.org/119976
Reason: This change is old enough and hasn't seen any updates since September 10, 2014. Abandoning it, please revive it if you plan to work on it again.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/141130

Revision history for this message
George Shuklin (george-shuklin) wrote :

There is abandoned blueprint to implement security groups via OVS: https://review.openstack.org/#/c/89712/

OVS supports ARP filtering. It is rather elegant, because it accepts nw_src/nw_dst (IPs) for arp rules and check them against ARP payload (TPA/SPA).

Etables brings more dependencies on neutron ovs agent. May be sticking to OVS only is better idea?

Kyle Mestery (mestery)
Changed in neutron:
milestone: none → kilo-3
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/158491

Kyle Mestery (mestery)
Changed in neutron:
milestone: kilo-3 → none
Kyle Mestery (mestery)
Changed in neutron:
milestone: none → liberty-1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/171003

Changed in neutron:
assignee: Juergen Brendel (jbrendel) → Kevin Benton (kevinbenton)
Revision history for this message
Kyle Mestery (mestery) wrote :

Kevin has a fix we should seriously look at for Kilo.

Changed in neutron:
milestone: liberty-1 → kilo-rc1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (master)

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

commit aa7356b729f9672855980429677c969b6bab61a1
Author: Kevin Benton <email address hidden>
Date: Sun Mar 29 03:37:25 2015 -0700

    Add simple ARP spoofing protection

    Adds an option to setup OVS rules that will prevent
    ports attached to the agent from sending any ARP responses
    that contain an IP address not belonging to the port
    (in fixed IPs or allowed_address_pairs).

    It is disabled by default and requires an OVS version that
    can match on ARP fields. If it is too old, traffic will
    still flow but it won't have ARP spoofing protection.
    There is a sanity check to verify that ARP header matching
    is supported.

    This prevention is specific to OVS so it will not help with
    other plugins that use the reference iptables filtering. A
    non-OVS-specific general approach will require something like
    the ebtables integration in Ibc6d3d520c1383cf7e00f4bdeb7853a41ac4b14b.

    Details:
    A new table is added for ARP spoofing prevention. All ARP traffic
    on the local switching table is sent to this spoofing table.
    The spoofing table will allow all ARP requests because we aren't
    interested in them. It will then install an ARP response allow rule
    for each IP address the port is assigned. All other ARP responses are
    dropped.

    DocImpact
    SecurityImpact
    Partial-Bug: #1274034

    Change-Id: I7c079b779245a0af6bc793564fa8a560e4226afe

Revision history for this message
Kyle Mestery (mestery) wrote :

Kevin's fix for ML2+OVS merged in Kilo, moving this out to Liberty to examine the ebtables fix.

Changed in neutron:
milestone: kilo-rc1 → liberty-1
Revision history for this message
Kevin Benton (kevinbenton) wrote :

As Kyle mentioned, this is fixed for OVS, but not for Linux bridge or other vswitches.

Changed in neutron:
assignee: Kevin Benton (kevinbenton) → nobody
Changed in ossa:
assignee: nobody → Juergen Brendel (jbrendel)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (master)

Reviewed: https://review.openstack.org/141130
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=2414834ffeb8ba7ce2401236d01c88702fec5a14
Submitter: Jenkins
Branch: master

commit 2414834ffeb8ba7ce2401236d01c88702fec5a14
Author: Édouard Thuleau <email address hidden>
Date: Tue Feb 10 13:43:34 2015 +1300

    ARP spoofing patch: Low level ebtables integration

    ARP cache poisoning is not actually prevented by the firewall
    driver 'iptables_firewall'. We are adding the use of the ebtables
    command - with a corresponding ebtables-driver - in order to create
    Ethernet frame filtering rules, which prevent the sending of ARP
    cache poisoning frames.

    The complete patch is broken into a set of smaller patches for easier review.

    This patch here is th first of the series and includes the low-level ebtables
    integration, unit and functional tests.

    Note:
        This commit is based greatly on an original, now abandoned patch,
        presented for review here:

            https://review.openstack.org/#/c/70067/

        Full spec can be found here:

            https://review.openstack.org/#/c/129090/

    SecurityImpact

    Change-Id: I9ef57a86b1a1c1fa4ba1a034c920f23cb40072c0
    Implements: blueprint arp-spoof-patch-ebtables
    Related-Bug: 1274034
    Co-Authored-By: jbrendel <email address hidden>

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

commit f77c17ef9993ea8c545dc044ad2ac013a28dbc22
Author: Juergen Brendel <email address hidden>
Date: Thu Feb 26 13:51:04 2015 +1300

    ARP spoofing patch: Data structures for rules.

    ARP cache poisoning is not actually prevented by the firewall
    driver 'iptables_firewall'. We are adding the use of the ebtables
    command - with a corresponding ebtables-driver - in order to create
    Ethernet frame filtering rules, which prevent the sending of ARP
    cache poisoning frames.

    The complete patch is broken into smaller patch sets for easier review.

    This patch set here includes the some classes for the maintenance of ebtable
    chains and rules.

    Note:
        This commit is based greatly on an original, now abandoned patch,
        presented for review here:

            https://review.openstack.org/#/c/70067/

    Full spec can be found here: https://review.openstack.org/#/c/129090/

    SecurityImpact

    Change-Id: I3c66e92cbe8883dcad843ad243388def3a96dbe5
    Implements: blueprint arp-spoof-patch-ebtables
    Related-Bug: 1274034
    Co-Authored-By: jbrendel <email address hidden>

tags: added: juno-backport-potential kilo-backport-potential
Revision history for this message
Cedric Brandily (cbrandily) wrote :

neutron kilo and juno do not depend on ebtables and their dependencies are frozen ... so the backport is not possible.

tags: removed: havana-backport-potential icehouse-backport-potential juno-backport-potential kilo-backport-potential
Revision history for this message
George Shuklin (george-shuklin) wrote :

But this is a security issue, isn't it?

Revision history for this message
Jeremy Stanley (fungi) wrote :

It _may_ be a security issue in your environment if you haven't mitigated it through other means already. That Neutron didn't do if for you in earlier releases doesn't mean it's a vulnerability in Neutron however, just that it was not a problem Neutron's anti-spoofing rules were originally designed to solve (much in the same way that a you wouldn't consider a helmet flawed just because it fails to protect your knees).

As previously discussed Neutron developers and the OpenStack Vulnerability Management Team have chosen not to consider a lack of Nova Network feature parity in Neutron a security vulnerability, just an incomplete design which could stand to be improved.

Revision history for this message
George Shuklin (george-shuklin) wrote :

I consider broken antispoofing as a serious flaw, because it allows to interrupt activity of innocent tenants by malicious activity of the unprivileged tenant.

If you insist that it is 'not a security issue, just imperfect design', ok, ok. But don't get upset if this bug will be used by competitors to demonstrate how neglected security issues are in Openstack.

Two versions of Openstack had been released with known security bug And after bugfix was finally released it was not ported to currently supported versions.

Nice work!

Revision history for this message
Cedric Brandily (cbrandily) wrote :

That's why Kevin Benton provides a partial correction for OVS agent in kilo (https://review.openstack.org/171003) ... this one can be backported.

But it's not a security bug (https://bugs.launchpad.net/neutron/+bug/1274034/comments/9)

Revision history for this message
Kris Lindgren (klindgren) wrote :

I completely agree with Geroge on this. You have a use case when neutron fails to correctly isolate on multi-tenants networks. This "incomplete feature" set was never called in documentation as a possible trade off. So if nothing you have an known issue that causes neutron not provide appropriate isolation under specific configurations, in a trivially to reproduce manner. This would lead to things that would be at a minimum considered bugs and most likely vulnerabilities.

Without a patch this "incomplete feature" allows trivial man in the middle attacks, taking vm's offline of any tenant at will, taking over the metadata id, from there one could easily change/spoof peoples metadata including changing it to add credentials/users for other tenants vm's. This could also lead to someone breaking vm provisioning (metadata/userdata) scripts for other tenants. One could also trivially takeover the gateway for flat networked tenants allowing a vm to see all the routed traffic on that network. If one also managed to spin up a vm on the shared public network that peoples "correctly isolated" private l2 routers attach to one could also takeover traffic/floating ip destined to routers that neutron should be handling. I have seen on the mailing list people wanting to support both private and shared networks so this is a completely plausible configuration.

Re: comment #9. Comment #8 specifically talks about back porting this change to latest stable --- which would be kilo/juno - no? and previous comments dealt more about handling this issue in the open as opposed to behind closed doors (IE only the security team and people involved in the fix can see the bug). Kevin Bentons patch only works on OVS. Last time I checked ml2 supported more than just OVS. Where this patch fixes it no mater the switch technology being used.

Revision history for this message
Kris Lindgren (klindgren) wrote :

Re: just that it was not a problem Neutron's anti-spoofing rules were originally designed to solve (much in the same way that a you wouldn't consider a helmet flawed just because it fails to protect your knees).

Considering this commit when allowed address pairs were added/refactored and the name previous name of this function: https://github.com/openstack/neutron/commit/b67b20832a5bfccd1bbf8d1e63ebcd7061856881

Or if thats not good enough - the original commit that added security group rules to begin with:
https://github.com/openstack/neutron/commit/f14af5dc755706c7297a96fa504acdfe15ac1957#diff-65b266f9e013df37c4934f0b1007897cR168

The original function of that code piece was specifically called out to do ARP SPOOFING filtering/prevention. It's just that the person who originally did it probably didn't realize that you cant correctly filter arp via iptables. So lets call a spade a spade here. Its not an "imperfect design", its not an "incomplete design", it not that "neutron or quantum didn't try to filter or have features to prevent arp spoofing/cache poisoning. Its a bug going back since security groups were implemented in neutron(actually quantum). This got masked by a few code refactors when allowed address pairs was added, but the intent to do arp filter since the "dawn of time" is clearly there.

So I would say based upon the code and the intent with the applied rules, this is more of the case of complaining because the helmet that you were wearing (that you were told is specifically suppose to protect you in the event of something bad) failed to protect your head and the kneepads that you were also wearing also failed to protect you knees.

Lets do the right thing here. Backport the fix to the stable versions. Admit that the protections we thought we original added 2+ years ago failed to actually do what we thought they did. And move on with bigger and better problems. Jeremy you even said in post #6 that if neutron documentation or config options says it specifically implements code to do the filter that it would be a vulnerability. Well the original code says it was suppose to filter ARP spoofing, it doesn't.

Revision history for this message
Jeremy Stanley (fungi) wrote :

I sympathize with the frustration some people have expressed over this situation--I'm not thrilled with it either. Unfortunately the currently proposed patches risk introducing behavior changes in already released versions of Neutron. To those who are interested in seeing fixes for this applied to stable release branches, please provide a suitable alternative implementation.

Revision history for this message
Darragh O'Reilly (darragh-oreilly) wrote :

Maybe I'm not understanding something, but I'm not convinced that the man-in-the-middle attack as described in the bug report is actually possible. How can VM1 forward the packets that come back from google.fr onto VM2? They will have the source ip of google.fr, so the anti-IP-address spoofing should drop them. I will test ...

Revision history for this message
George Shuklin (george-shuklin) wrote :

Formally: There is a security hole in Openstack and it will not be closed for the nearest half of the year and will not applied to existing supporting installation.

I do not understand why fix to _SECURITY_ bug is rejected because it will change behaviour? Obviously it will change behaviour, it will break ability for malicious user to break multitenancy in Openstack.

Please, care about malicious hackers, please do not port security fixes to the existing versions! Otherwise they would find that Openstack is no longer vulnerable.

Revision history for this message
George Shuklin (george-shuklin) wrote :

Darragh O'Reilly, they can not use fake address (I've tested this), but they can announce it rendering any host in the network disabled.

Or they can announce fake IP and listen for any non-stream protocols (f.e. UDP). They still will not be able to retransmit it to original or reply, but can intercept any unidirectional UDP (f.e. pieces of voice conversations in RTP, or even, pieces of TCP (with cookies! yum!)). Legitimate host will ask to retransmit them, but malicious VM will receive one copy of data.

If it will do this sporadically for short time (like once in 10s) it will not disturb work of the legitimate host significantly (sometimes TCP will be really slow or stuck, but recover eventually), but still allows interception of pieces of traffic.

I think this is a clear vulnerability in neutron without any 'but you can try to mitigate this' (HOW?).

Revision history for this message
Darragh O'Reilly (darragh-oreilly) wrote :

George - I was pointing out that the bug report at the top of this page is suspect. I have just tried, and I cannot recreate what it says, as the existing anti-spoofing rules do indeed prevent forwarding. Only after turning off anti-spoofing with iptable -F does the attack work as the bug claims.

Revision history for this message
Kris Lindgren (klindgren) wrote :

So for man in the middle while I have not fully POC'd this. The following does/should work:
1.) Spin up a vm on a shared network with other tenants
2.) arpping for the gateway with your own mac or that of another vm.
3.) Add default gateway to your vm or another vm
3.) update the allowed ip address via allowed-address-pairs extension (which is enabled by default and is permited by the default rules) to add the default gateway to the your vm or another vm. Allowed address pairs does zero bounds checking on ip's that you want to allow on a vm. Also, until: https://github.com/openstack/neutron/commit/927399c011409b7d152b7670b896f15eee7d0db3 is backported is also a security issue, since by default anyone was allowed to hit the allowed address pairs extension. Also this allows you to directly spoof other peoples mac/ips and allow this traffic though the anti-spoofing rules.
4.) Profit. At this point you are garping for the default gateway and you have a vm that will allow traffic to pass.

Without allowed-address-pairs one would be limited to bringing down an entire subnet/guest and/or seeing half of the network connectivity. Is a DoS also considered a security vulnerability?

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (neutron-pecan)

Related fix proposed to branch: neutron-pecan
Review: https://review.openstack.org/185072

Revision history for this message
Juergen Brendel (jbrendel) wrote :

We had proposed a blueprint for a fix to this bug: https://blueprints.launchpad.net/neutron/+spec/arp-spoof-patch-ebtables

The fix was implemented and presented in the form of four patches. The first two have been accepted and merged:

https://review.openstack.org/#/c/141130/
https://review.openstack.org/#/c/157097/

The two remaining patches, which would have integrated the patch with the existing iptables code, however, were rejected:

https://review.openstack.org/#/c/157634/
https://review.openstack.org/#/c/158491/

Marc McClain suggested a different approach and did not want to have the ebtables manager in its current form in the code. Since the remaining two patches now do not have a chance of being accepted any more, I am following Henry Gessau's recommendation: I am abandoning the remaining patches and assign this bug to Marc, who will propose and implement a different solution.

We will be happy to review the proposed new solution once we see a blueprint. The acceptance requirement is simply to have a platform independent solution, which prevents ARP cache poisoning on shared networks, as described in the bug report.

Changed in ossa:
assignee: Juergen Brendel (jbrendel) → nobody
Henry Gessau (gessau)
Changed in neutron:
assignee: nobody → Mark McClain (markmcclain)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Juergen Brendel (<email address hidden>) on branch: master
Review: https://review.openstack.org/158491
Reason: Abandoned, since Marc McClain would like to implement a different solution for the problem. Also see my comment in the bug report.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Juergen Brendel (<email address hidden>) on branch: master
Review: https://review.openstack.org/157634
Reason: Abandoned, since Marc McClain would like to implement a different solution for the problem. Also see my comment in the bug report.

Revision history for this message
Kris Lindgren (klindgren) wrote :

Juergen - Can you provide a link to that discussion?

Revision history for this message
Cedric Brandily (cbrandily) wrote :

@Henry, Juergen: should we revert merged changes?

https://review.openstack.org/141130
https://review.openstack.org/157097

Thierry Carrez (ttx)
Changed in neutron:
milestone: liberty-1 → liberty-2
Changed in neutron:
assignee: Mark McClain (markmcclain) → Kevin Benton (kevinbenton)
Revision history for this message
Chet Burgess (cfb-n) wrote :

Whats the current state of the planned work for this?

I know that jbrendel's work got partially merged and partially abandoned with reference to an alternate approach. Is there a link to a BP or discussion about the alternate approach thats being taken?

Revision history for this message
Kevin Benton (kevinbenton) wrote :

This is the alternate approach: https://review.openstack.org/#/c/196986/

Very small isolated patch that can be back-ported to Kilo and can be applied by operators to even earlier versions like Juno as well. I think we won't be able to back-port upstream to Juno because the config value was added in Kilo, but the stable team might be able to be convinced otherwise.

Revision history for this message
Kevin Benton (kevinbenton) wrote :

Note: This should already be fixed for OVS via https://review.openstack.org/#/c/171003/

Revision history for this message
Chet Burgess (cfb-n) wrote :

kevinbenton,

Excellent! Thanks for the patch.

Just to confirm it looks like your patch is a complete solution and the the previously merged pieces of jbrendel's work are not necessary for your fix?

Revision history for this message
Kevin Benton (kevinbenton) wrote :

Yes, this patch does not depend on the ebtables manager work because I wanted it to be able apply to older codebases.

Changed in neutron:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (feature/pecan)

Fix proposed to branch: feature/pecan
Review: https://review.openstack.org/200163

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (feature/pecan)
Download full text (28.1 KiB)

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

commit a0a022373b90835059b8949a57b097030bcbc37e
Author: John Davidge <email address hidden>
Date: Tue Jul 7 17:00:01 2015 +0100

    Fix issues with allocation pool generation for ::/64 cidr

    Passing a ::/64 cidr to certain netaddr functions without specifying
    the ip_version causes errors. Fix this by specifying ip_version.

    Change-Id: I31aaf9f5dabe4dd0845507f245387cd4186c410c
    Closes-Bug: 1472304

commit c28b6b0ef8606abea00eeea4fde96a4f646da952
Author: Brian Haley <email address hidden>
Date: Tue Jul 7 17:03:04 2015 -0400

    Remove lingering traces of q_

    The rename from Quantum to Neutron left a few q_ strings
    around, let's go ahead and clean them up.

    Change-Id: I06e6bdbd0c2f3a25bb90b5fa291009b9ec2d471d

commit 5b6ca5ce898a2e9a810ec49a1712337a41822788
Author: armando-migliaccio <email address hidden>
Date: Tue Jul 7 11:13:41 2015 -0700

    Make sure path_prefix is set during unit tests

    Change 18bc67d5 broke *-aas unit tests.

    This change ensures that mocking is done correctly, the same way
    it is done for the other plugin attributes

    Change-Id: I4167f18560e3a3aad652aae1ea9d3c6bc34dc796
    Closes-bug: #1472361

commit 13b0f6f8e2fd1e84ff3580cd75bb879e18064da6
Author: Carl Baldwin <email address hidden>
Date: Tue Jul 7 16:41:03 2015 +0000

    Add IP_ANY dict to ease choosing between IPv4 and IPv6 "any" address

    I'm working on a new patch that will add one more case where we need
    to choose between 0.0.0.0/0 and ::/0 based on the ip version. I
    thought I'd add a new constant and simplify a couple of existing uses.

    Change-Id: I376d60c7de4bafcaf2387685ddcc1d98978ce446

commit a863342caf7da9a1c0430549c1ea1e53408b34af
Author: Cyril Roelandt <email address hidden>
Date: Tue Jul 7 14:25:06 2015 +0000

    Python3: cast the result of zip() to list

    The result of get_sorts was a 'zip object' in Python 3, and it was later used
    as a list, which fails. Just cast the result to a list to fix this issue.

    Change-Id: I12017f79cad92b1da4fe5f9939b38436db7219eb
    Blueprint: neutron-python3

commit 8b13609edac2c136e1a0acbc05ad93059bb59fc1
Author: Pavel Bondar <email address hidden>
Date: Thu Jul 2 11:35:18 2015 +0300

    Track allocation_pools in SubnetRequest

    To keep pluggable and non-pluggable ipam implementation consistent
    non-pluggable one has to be switched to track allocation_pools and
    gateway_ip using SubnetRequests.
    SubnetRequest requires allocation_pools to be list of IPRanges.
    Previously allocation_pools were tracked as list of dicts.
    So allocation_pools generating and validating was moved before
    SubnetRequest is created.

    Partially-Implements: blueprint neutron-ipam

    Change-Id: I8d2fec3013b302db202121f946b53a0610ae8321

commit 04197bc4bbf2bc611371060db839028c2686f87a
Author: Kevin Benton <email address hidden>
Date: Mon Jun 29 21:05:08 2015 -0700

    Add ARP spoofing protection for ...

tags: added: in-feature-pecan
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/201650

Changed in neutron:
status: Fix Committed → Fix Released
Revision history for this message
Chet Burgess (cfb-n) wrote :

I don't know if upstream want a back port to juno but for people who want a patch, attached is a patch against the latest stable/juno. I'm happy to submit this as a review to stable/juno if upstream things it back port worth.

NOTE: I don't have a facility for running the functional tests, and the structure seems to have changed quite a bit between master and juno. As a result I didn't back port the functional tests from master but I did write unit tests for juno.

Revision history for this message
Kevin Benton (kevinbenton) wrote :

submit it to gerrit and I will help review it. I think an argument can be made in its favor.

Revision history for this message
Chet Burgess (cfb-n) wrote :

OK I will work on that in the next day or two.

Revision history for this message
Kevin Benton (kevinbenton) wrote :

thanks, when you do be sure to upload it under change-id: I0b0e3b1272472385dff060897ecbd25e93fd78e7 so its easy to find from the original.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (stable/kilo)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (stable/juno)

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

Revision history for this message
Chet Burgess (cfb-n) wrote :

Opps.. let me go fix the change ID. Sorry.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (stable/juno)

Change abandoned by Chet Burgess (<email address hidden>) on branch: stable/juno
Review: https://review.openstack.org/209707
Reason: Replaced by https://review.openstack.org/#/c/209708/

Revision history for this message
Chet Burgess (cfb-n) wrote :

OK fixed.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (master)

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

commit b62b92da9b9dbba953573bc212279c719e08f3ef
Author: Cedric Brandily <email address hidden>
Date: Tue Sep 8 15:23:49 2015 +0000

    Remove ebtables_driver/manager dead code

    Previous changes[1] have been merged as enablers[2] to fix the bug
    1274034 but an alternative solution has been choosen and now we can
    consider the introduced code as dead code.

    This changes removes [2], associated tests and rootwrap filters.

    [1] I9ef57a86b1a1c1fa4ba1a034c920f23cb40072c0
        I3c66e92cbe8883dcad843ad243388def3a96dbe5
    [2] neutron.agent.linux.ebtables_driver
        neutron.agent.linux.ebtables_manager

    Closes-Bug: #1493422
    Related-Bug: #1274034
    Change-Id: I61e38fc0d8cf8e79252aabc19a70240be57e4a32

Revision history for this message
Chet Burgess (cfb-n) wrote :

It occurs to me that no one has done a kilo patch yet. I assume that would be in the works. If there is interest I can do kilo as well.

Revision history for this message
Kevin Benton (kevinbenton) wrote :
Revision history for this message
Chet Burgess (cfb-n) wrote :

Cool

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (feature/pecan)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (feature/pecan)
Download full text (73.6 KiB)

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

commit fe236bdaadb949661a0bfb9b62ddbe432b4cf5f1
Author: Miguel Angel Ajo <email address hidden>
Date: Thu Sep 3 15:40:12 2015 +0200

    No network devices on network attached qos policies

    Network devices, like internal router legs, or dhcp ports
    should not be affected by bandwidth limiting rules.

    This patch disables application of network attached policies
    to network/neutron owned ports.

    Closes-bug: #1486039
    DocImpact

    Change-Id: I75d80227f1e6c4b3f5fa7762b8dc3b0c0f1abd46

commit db4a06f7caa20a4c7879b58b20e95b223ed8eeaf
Author: Ken'ichi Ohmichi <email address hidden>
Date: Wed Sep 16 10:04:32 2015 +0000

    Use tempest-lib's token_client

    Now tempest-lib provides token_client modules as library and the
    interface is stable. So neutron repogitory doesn't need to contain
    these modules.
    This patch makes neutron use tempest-lib's token_client and removes
    the own modules for the maintenance.

    Change-Id: Ieff7eb003f6e8257d83368dbc80e332aa66a156c

commit 78aed58edbe6eb8a71339c7add491fe9de9a0546
Author: Jakub Libosvar <email address hidden>
Date: Thu Aug 13 09:08:20 2015 +0000

    Fix establishing UDP connection

    Previously, in establish_connection() for UDP protocol data were sent
    but never read on peer socket. That lead to successful read on peer side
    if this connection was filtered. Having constant testing string masked
    this issue as we can't distinguish to which test of connectivity data
    belong.

    This patch makes unique data string per test_connectivity() and
    also makes establish_connection() to create an ASSURED entry in
    conntrack table. Finally, in last test after firewall filter was
    removed, connection is re-established in order to avoid troubles with
    terminated processes or TCP continuing sending packets which weren't
    successfully delivered.

    Closes-Bug: 1478847
    Change-Id: I2920d587d8df8d96dc1c752c28f48ba495f3cf0f

commit e6292fcdd6262434a7b713ad8802db6bc8a6d3dc
Author: YAMAMOTO Takashi <email address hidden>
Date: Wed Sep 16 13:20:51 2015 +0900

    ovsdb: Fix a few docstring

    Change-Id: I53e1e21655b28fe5da60e58aeeb7cbbd103ae014

commit c22949a4449d96a67caa616290cf76b67b182917
Author: fumihiko kakuma <email address hidden>
Date: Wed Sep 16 11:52:59 2015 +0900

    Remove requirements.txt for the ofagent mechanism driver

    It is no longer used.

    Related-Blueprint: core-vendor-decomposition
    https://blueprints.launchpad.net/neutron/+spec/core-vendor-decomposition

    Change-Id: Ib31fb3febf8968e50d86dd66e1e6e1ea2313f8ac

commit d1d4de19d85f961d388c91e70f31b3bafec418c5
Author: Kevin Benton <email address hidden>
Date: Thu Sep 3 20:25:57 2015 -0700

    Always return iterables in L3 get_candidates

    The caller of this function expects iterables.

    Closes-Bug: #1494996
    Change-Id: I3d103e63f4e127a77268502415c0ddb0d804b54a

commit 1ad6ac448067306...

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (stable/juno)

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

commit 1b73fbd70522a751f92120e776471c4deb159d0c
Author: Chet Burgess <email address hidden>
Date: Tue Aug 4 13:10:04 2015 -0700

    Add ARP spoofing protection for LinuxBridge agent

    This is a backport for the fix that went into master to address
    this bug.

    This patch adds ARP spoofing protection for the Linux Bridge
    agent based on ebtables.

    The protection is enabled and disabled with the same
    'prevent_arp_spoofing' agent config flag added for the OVS agent
    in I7c079b779245a0af6bc793564fa8a560e4226afe.

    The protection works by setting up an ebtables chain for each port
    and jumping all ARP traffic to that chain. The port-specific chains
    have a default DROP policy and then have allow rules installed that
    only allow ARP traffic with a source CIDR that matches one of the
    port's fixed IPs or an allowed address pair.

    Change-Id: I0b0e3b1272472385dff060897ecbd25e93fd78e7
    Closes-Bug: #1274034

tags: added: in-stable-juno
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (feature/pecan)

Change abandoned by Doug Wiegley (<email address hidden>) on branch: feature/pecan
Review: https://review.openstack.org/224334

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (stable/kilo)

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

commit bf28c724ddbe8f7e67f91513da5f302d5372314d
Author: Kevin Benton <email address hidden>
Date: Mon Jun 29 21:05:08 2015 -0700

    Add ARP spoofing protection for LinuxBridge agent

    This patch adds ARP spoofing protection for the Linux Bridge
    agent based on ebtables. This code was written to be minimally
    invasive with the intent of back-porting to Kilo.

    The protection is enabled and disabled with the same
    'prevent_arp_spoofing' agent config flag added for the OVS agent
    in I7c079b779245a0af6bc793564fa8a560e4226afe.

    The protection works by setting up an ebtables chain for each port
    and jumping all ARP traffic to that chain. The port-specific chains
    have a default DROP policy and then have allow rules installed that
    only allow ARP traffic with a source CIDR that matches one of the
    port's fixed IPs or an allowed address pair.

    Since this is a back-port to Kilo, it is disabled by default just
    like the protection added for OVS.

    This patch additionally pulls back the required ebtables filter and
    the functional test helpers to support the tests.

    Conflicts:
     neutron/plugins/linuxbridge/agent/linuxbridge_neutron_agent.py
     neutron/plugins/linuxbridge/common/config.py
     neutron/tests/common/machine_fixtures.py

    Closes-Bug: #1274034
    Change-Id: I0b0e3b1272472385dff060897ecbd25e93fd78e7
    (cherry picked from commit 04197bc4bbf2bc611371060db839028c2686f87a)

tags: added: in-stable-kilo
Thierry Carrez (ttx)
Changed in neutron:
milestone: liberty-2 → 7.0.0
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.