Metadata iptables rules never inserted upon exception on router creation

Bug #1735724 reported by Daniel Alvarez
270
This bug affects 4 people
Affects Status Importance Assigned to Milestone
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned
neutron
Fix Released
High
Unassigned

Bug Description

We've been debugging some issues being seen lately [0] and found out that there's a bug in l3 agent when creating routers (or during initial sync). Jakub Libosvar and I spent some time recreating the issue and this is what we got:

Especially since we bumped to ovsdbapp 0.8.0, we've seen some jobs failing due to errors when authenticating using PK to a VM. The TCP connection to the SSH port was successfully established but the authentication failed. After debugging further, we found out that metadata rules in qrouter namespace which redirect traffic to haproxy (which replaced old neutron-ns-metadata-proxy) were missing, so VM's weren't fetching metadata (hence, public key).

These rules are installed by metadata driver after a router is created [1] on the AFTER_CREATE notification. Also, they will get created during the initial sync of the l3 agent (since it's still unknown for the agent) [2]. Here, if we don't know the router yet, we'll call _proccess_added_router() and if it's a known router we'll call _process_updated_router().
After our tests, we've seen that iptables rules are never restored if we simulate an
Exception inside ri.process() at [3] even though the router is scheduled for resync [4]. The reason why this happens is because we've already added it to our router info [5] so even though
ri.process() fails at L481 and it's scheduled for resync, next time _process_updated_router()
will get called instead of _process_added_router() thus not pushing the notification into
metadata driver to install iptables rules and they never get installed.

In conclusion, if an error occurs during _process_added_router() we might end up losing
metadata forever until we restart the agent and this call succeeds. Worse, we will be
forwarding metadata requests via br-ex which could lead to security issues (ie. we could be injecting wrong metadata from the outside or the metadata server running in the underlying cloud may respond).

With ovsdbapp 0.9.0 we're minimizing this because if a port fails to be added to br-int, ovsdbapp will enqueue the transaction instead of throwing an Exception but there could be still some other exceptions I guess that reproduces this scenario outside of ovsdbapp so we need to fix it
in Neutron.

Thanks
Daniel Alvarez

---

[0] https://bugs.launchpad.net/tripleo/+bug/1731063
[1] https://github.com/openstack/neutron/blob/02fa049c5f5a38a276bec6e55c68ac19cd08c59f/neutron/agent/metadata/driver.py#L288
[2] https://github.com/openstack/neutron/blob/02fa049c5f5a38a276bec6e55c68ac19cd08c59f/neutron/agent/l3/agent.py#L472
[3] https://github.com/openstack/neutron/blob/02fa049c5f5a38a276bec6e55c68ac19cd08c59f/neutron/agent/l3/agent.py#L481
[4] https://github.com/openstack/neutron/blob/02fa049c5f5a38a276bec6e55c68ac19cd08c59f/neutron/agent/l3/agent.py#L565
[5] https://github.com/openstack/neutron/blob/02fa049c5f5a38a276bec6e55c68ac19cd08c59f/neutron/agent/l3/agent.py#L478

Revision history for this message
Daniel Alvarez (dalvarezs) wrote :

A possible solution would be moving the iptables rules installation for metadata right after the namespace gets created or right before we plug the ports to br-int instead of doing it in the AFTER_CREATE router notification handler.

Changed in neutron:
assignee: nobody → Brian Haley (brian-haley)
importance: Undecided → High
status: New → Confirmed
Revision history for this message
Ihar Hrachyshka (ihar-hrachyshka) wrote :

This bug has a security edge (metadata requests leaking to outside / metadata responses reaching us from the outside), so I marked it accordingly. The bug will stay in public since it was reported like that in the first place.

information type: Public → Public Security
Revision history for this message
Jeremy Stanley (fungi) wrote :

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.

Changed in ossa:
status: New → Incomplete
Changed in neutron:
status: Confirmed → In Progress
Revision history for this message
Brian Haley (brian-haley) wrote :

https://review.openstack.org/#/c/524406/ is a partial fix for this issue, I just added the bug to the commit message later so it hasn't added a link here.

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/527210

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

Reviewed: https://review.openstack.org/524406
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=69419778276a722c3f376046d1e0cc7fc4684e99
Submitter: Zuul
Branch: master

commit 69419778276a722c3f376046d1e0cc7fc4684e99
Author: Brian Haley <email address hidden>
Date: Thu Nov 30 16:57:51 2017 -0500

    Add iptables metadata marking rule on router init

    Move the iptables metadata marking rule earlier in
    router init, that way any stray metadata requests
    that arrive before the filter metadata redirect rule is
    installed will just be dropped. We do this irregardless
    of whether we will be running the metadata proxy.

    Partial-bug: #1735724

    Change-Id: I8982523dbb94a7c5b8a4db88a196fabc4dd2873f

Revision history for this message
Slawek Kaplonski (slaweq) wrote : auto-abandon-script

This bug has had a related patch abandoned and has been automatically un-assigned due to inactivity. Please re-assign yourself if you are continuing work or adjust the state as appropriate if it is no longer valid.

Changed in neutron:
assignee: Brian Haley (brian-haley) → nobody
status: In Progress → New
tags: added: timeout-abandon
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.openstack.org/527210
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

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

Fix proposed to branch: stable/queens
Review: https://review.openstack.org/598979

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

Fix proposed to branch: stable/pike
Review: https://review.openstack.org/598980

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

Reviewed: https://review.openstack.org/598979
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=e55ff3ec35566a718a325b198fc45b890d129d24
Submitter: Zuul
Branch: stable/queens

commit e55ff3ec35566a718a325b198fc45b890d129d24
Author: Brian Haley <email address hidden>
Date: Thu Nov 30 16:57:51 2017 -0500

    Add iptables metadata marking rule on router init

    Move the iptables metadata marking rule earlier in
    router init, that way any stray metadata requests
    that arrive before the filter metadata redirect rule is
    installed will just be dropped. We do this irregardless
    of whether we will be running the metadata proxy.

    Partial-bug: #1735724

    Change-Id: I8982523dbb94a7c5b8a4db88a196fabc4dd2873f
    (cherry picked from commit 69419778276a722c3f376046d1e0cc7fc4684e99)

tags: added: in-stable-queens
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (stable/pike)

Reviewed: https://review.openstack.org/598980
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=6a8a1f81aea35933996494674157454e7abfb1ee
Submitter: Zuul
Branch: stable/pike

commit 6a8a1f81aea35933996494674157454e7abfb1ee
Author: Brian Haley <email address hidden>
Date: Thu Nov 30 16:57:51 2017 -0500

    Add iptables metadata marking rule on router init

    Move the iptables metadata marking rule earlier in
    router init, that way any stray metadata requests
    that arrive before the filter metadata redirect rule is
    installed will just be dropped. We do this irregardless
    of whether we will be running the metadata proxy.

    Partial-bug: #1735724

    Change-Id: I8982523dbb94a7c5b8a4db88a196fabc4dd2873f
    (cherry picked from commit 69419778276a722c3f376046d1e0cc7fc4684e99)

tags: added: in-stable-pike
Revision history for this message
Jeremy Stanley (fungi) wrote :

Are modern versions of Neutron still sufficiently vulnerable to this to warrant publication of an advisory once it's closed? Is a fix similar to the abandoned https://review.opendev.org/527210 still needed?

Revision history for this message
Mithil Arun (arun-mithil) wrote :

We continue to see this on Rocky where, often times, routers created are missing the REDIRECT rule in iptables for metadata-proxy.

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

Train is our oldest still maintained release, at least for a few more months. Does anyone know if this still happens on Train and newer versions?

Revision history for this message
Slawek Kaplonski (slaweq) wrote :

Hi,

I don't know for sure but I will check that on Monday and will update this bug report.

Revision history for this message
Slawek Kaplonski (slaweq) wrote :

I was trying to reproduce that issue today and I couldn't.
Looking at the code it seems for me that after Brian's change [1] those rules are now added to the iptables_manager during creation of the router_info instance. So it's way before ri.process() is really called. If there will be any issue in that constuctor, there will be even no namespace created at all for the router.

[1] https://review.openstack.org/524406

Changed in neutron:
status: New → Fix Released
Revision history for this message
Jeremy Stanley (fungi) wrote :

Thanks for digging into the report. Based on your analysis, the VMT has no plans to issue an advisory, since none of our supported releases is considered vulnerable to this any longer. If new information is brought to light which indicates there is still a means to exploit this flaw in more recent releases, we're happy to reconsider the decision at that time.

Changed in ossa:
status: Incomplete → Won't Fix
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

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