l3 agent metadata proxy allows access to metadata from any available network

Bug #1865036 reported by Radosław Piliszek
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned
neutron
Fix Released
High
Brian Haley

Bug Description

Tested with Train release, by quick code check it affects also at least Rocky, Stein and Ussuri (current master).

The security issue is than one can access metadata of an arbitrary other instance if the following conditions are met (let's call the "other instance" a "victim", it can be in any other project not normally available to the attacker):
1) Victim's port fixed IP address is known.
2) Victim's port network ID is known.
3) Attacker can use a network with access to l3 agent metadata proxy (i.e. can use routers) and deploy instances.

The scenario is as follows:
1) create a self-service network including the targeted address
2) create an instance with the same fixed IP address
3) create a router and wire it up with that network (other connections irrelevant)
4) boot up the instance (make sure to drop the potential route to dhcp agent metadata proxy if used)
5) run e.g.:
curl -H "X-Neutron-Network-ID: $VICTIM_NET_ID" 169.254.169.254/openstack/latest/meta_data.json

Observed behaviour:
Normally-secret information disclosure.

Expected behaviour:
Proxy ignores (removes) that extra header and proceeds as if nothing happened (most expected)
OR proxy returns an error (and logs it / sends a notification about it)
OR proxy blocks the request and calls the police as you are a bad boy :-) (least expected... but nice)

Initial code analysis:

1) the haproxy config is inadequate:
https://opendev.org/openstack/neutron/src/commit/6b9765c991da8731fe39f7e7eed1ed6e2bca231a/neutron/agent/metadata/driver.py#L68
^ this should replace all current headers in the current trust model

2) the reason this works with l3 agent (and so far not with dhcp agent unless there is some other general header exploit in the stack) are the following lines:
https://opendev.org/openstack/neutron/src/commit/6b9765c991da8731fe39f7e7eed1ed6e2bca231a/neutron/agent/metadata/agent.py#L146-L152

Revision history for this message
Brian Haley (brian-haley) wrote :

So if I'm reading https://www.haproxy.com/documentation/hapee/1-7r1/traffic-management/http-rewrite/ correctly, making this small change in the metadata agent driver should fix this:

s/add-header/set-header

set-header removes first any header in existing field that matches <name>.

If others agree I could send out a patch.

Revision history for this message
Radosław Piliszek (yoctozepto) wrote :

This is one thing. The second is to unset the other, unexpected, header. I did not propose code solution because I don't know what you would prefer and didn't really want to disclose this security vulnerability. I can propose something if you want.

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.

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

This seems similar (though not identical) to bug 1563954, and given we assert that "Nova metadata service should not be used for sensitive information" per https://wiki.openstack.org/wiki/OSSN/OSSN-0074 I feel like we should be able to continue this work in public. Any objections?

Revision history for this message
Radosław Piliszek (yoctozepto) wrote :

I am not entirely sure. By all means one should not put top secret stuff in metadata (due to other drawbacks there, like access by any process on that instance, potential ip address spoofing with ironic deploys etc.), but I could not find in docs that exact recommendation. Also, what is the purpose of request validation via metadata_proxy_shared_secret which eliminates the X-Forwarder-For issue already?

Revision history for this message
Radosław Piliszek (yoctozepto) wrote :

Reworded: people setting this up with TLS and shared secret may expect sensitive information can be transferred this way and *not* disclosed (yoctozepto included).

Revision history for this message
Radosław Piliszek (yoctozepto) wrote :

Side note: I analysed also ovn codepaths and they look fine (secure against this issue) - this is because they have only one header.

Changed in neutron:
status: New → Confirmed
assignee: nobody → Brian Haley (brian-haley)
tags: added: l3-dvr-backlog
removed: l3-agent metadata
Revision history for this message
Brian Haley (brian-haley) wrote :

I don't have any objections to changing this to public, we can wait for another core to vote as well.

The reason I say this is because in order for an attacker to carry this out easily they must know the network ID of the victim, which is only available with admin credentials.

I was able to reproduce this and have a fix, needed to use del-header and set-header, but I now see the correct instance ID in the metadata request forwarded to nova.

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

If the network ID is a type 4 UUID then it sounds like we can safely categotize this as a class C1 report per https://security.openstack.org/vmt-process.html#incident-report-taxonomy .

Revision history for this message
Radosław Piliszek (yoctozepto) wrote :

It is UUID v4, and most likely has to be guessed.

I don't agree IDs are only available with admin credentials. One can look up IDs for own networks without being admin so it's technically possible to snoop it from regular user (and it's not meant to be sensitive in the first place).

I would strengthen the proxy code as well - it should reject requests bearing both the headers - it is not supported.

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

I agree with Brian that it can be public.
@Jeremy yes, network ID is type 4 UUID so IMHO we can categorize it as class C1 report.

Jeremy Stanley (fungi)
description: updated
information type: Private Security → Public
Changed in ossa:
status: Incomplete → Won't Fix
tags: added: security
Revision history for this message
Brian Haley (brian-haley) wrote :

What I was trying to say in my previous comment was that a tenant can't lookup a network ID of another tenant, just their own and any that are shared. Someone with admin creds can lookup all the network IDs.

I can send my patch once this is made public, but it will delete the portions of a request that shouldn't be there - if it's expecting a network ID it will delete any router ID header, and vice-versa.

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

Switched to public, treating as a class C1 report for now (impractical to exploit, no advisory required, someone might assign a CVE, security note may be warranted) but we can adjust the classification if new information comes to light.

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

I think this won't apply to ML2/OVN as metadata is always fetched via static route to the metadata port and not through a router, so that traffic will never exit the logical switch (Neutron network) to another metadata proxy.

Changed in neutron:
importance: Undecided → High
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.opendev.org/710458

Changed in neutron:
status: Confirmed → In Progress
Revision history for this message
Radosław Piliszek (yoctozepto) wrote :

@Daniel - yeah, I confirmed that in comment #7.

@Brian - ack, agreed.

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

Reviewed: https://review.opendev.org/710458
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=5af046fd4e6387cdbf8bf65ea4c2039a4019b64b
Submitter: Zuul
Branch: master

commit 5af046fd4e6387cdbf8bf65ea4c2039a4019b64b
Author: Brian Haley <email address hidden>
Date: Thu Feb 27 17:33:28 2020 -0500

    Remove extra header fields in proxied metadata requests

    If a user specifies a header in their request for metadata,
    it could override what the proxy would have inserted on their
    behalf. Make sure to remove any headers we don't want, and
    override something that might be present in the request.
    If the agent somehow gets a request with both headers it will
    silently drop it.

    Change-Id: Id6c103b7bcebe441c27c6049d349d84ba7fd15a6
    Closes-bug: #1865036

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

Fix proposed to branch: stable/train
Review: https://review.opendev.org/711100

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

Fix proposed to branch: stable/stein
Review: https://review.opendev.org/711101

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

Fix proposed to branch: stable/rocky
Review: https://review.opendev.org/711102

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.opendev.org/711103

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

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

commit b8de1e6ef01a45f75ad14d0d96cbcf40a8767f5d
Author: Brian Haley <email address hidden>
Date: Thu Feb 27 17:33:28 2020 -0500

    Remove extra header fields in proxied metadata requests

    If a user specifies a header in their request for metadata,
    it could override what the proxy would have inserted on their
    behalf. Make sure to remove any headers we don't want, and
    override something that might be present in the request.
    If the agent somehow gets a request with both headers it will
    silently drop it.

    Change-Id: Id6c103b7bcebe441c27c6049d349d84ba7fd15a6
    Closes-bug: #1865036
    (cherry picked from commit 5af046fd4e6387cdbf8bf65ea4c2039a4019b64b)

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

Reviewed: https://review.opendev.org/711101
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=4dc0a61cd5aaa8a0448b19c08b9e8f04d2d11eb4
Submitter: Zuul
Branch: stable/stein

commit 4dc0a61cd5aaa8a0448b19c08b9e8f04d2d11eb4
Author: Brian Haley <email address hidden>
Date: Thu Feb 27 17:33:28 2020 -0500

    Remove extra header fields in proxied metadata requests

    If a user specifies a header in their request for metadata,
    it could override what the proxy would have inserted on their
    behalf. Make sure to remove any headers we don't want, and
    override something that might be present in the request.
    If the agent somehow gets a request with both headers it will
    silently drop it.

    Change-Id: Id6c103b7bcebe441c27c6049d349d84ba7fd15a6
    Closes-bug: #1865036
    (cherry picked from commit 5af046fd4e6387cdbf8bf65ea4c2039a4019b64b)

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

Reviewed: https://review.opendev.org/711100
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=bcc4f98a3d7e8f0e23cbf953cc828506a8a8002d
Submitter: Zuul
Branch: stable/train

commit bcc4f98a3d7e8f0e23cbf953cc828506a8a8002d
Author: Brian Haley <email address hidden>
Date: Thu Feb 27 17:33:28 2020 -0500

    Remove extra header fields in proxied metadata requests

    If a user specifies a header in their request for metadata,
    it could override what the proxy would have inserted on their
    behalf. Make sure to remove any headers we don't want, and
    override something that might be present in the request.
    If the agent somehow gets a request with both headers it will
    silently drop it.

    Change-Id: Id6c103b7bcebe441c27c6049d349d84ba7fd15a6
    Closes-bug: #1865036
    (cherry picked from commit 5af046fd4e6387cdbf8bf65ea4c2039a4019b64b)

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

Reviewed: https://review.opendev.org/711102
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=1c13ad05dd937b65330c389301c2f095b7bedcd3
Submitter: Zuul
Branch: stable/rocky

commit 1c13ad05dd937b65330c389301c2f095b7bedcd3
Author: Brian Haley <email address hidden>
Date: Thu Feb 27 17:33:28 2020 -0500

    Remove extra header fields in proxied metadata requests

    If a user specifies a header in their request for metadata,
    it could override what the proxy would have inserted on their
    behalf. Make sure to remove any headers we don't want, and
    override something that might be present in the request.
    If the agent somehow gets a request with both headers it will
    silently drop it.

    Change-Id: Id6c103b7bcebe441c27c6049d349d84ba7fd15a6
    Closes-bug: #1865036
    (cherry picked from commit 5af046fd4e6387cdbf8bf65ea4c2039a4019b64b)

tags: added: in-stable-rocky
Revision history for this message
Nick Tait (nickthetait) wrote :

I agree with C1 (or lower) classification for this.

tags: added: neutron-proactive-backport-potential
tags: removed: neutron-proactive-backport-potential
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/neutron queens-eol

This issue was fixed in the openstack/neutron queens-eol release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/neutron rocky-eol

This issue was fixed in the openstack/neutron rocky-eol release.

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.