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

[Radosław Piliszek]

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

[Brian Haley (brian-haley)]

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.

[Radosław Piliszek (yoctozepto)]

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.

[Jeremy Stanley (fungi)]

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.

[Jeremy Stanley (fungi)]

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?

[Radosław Piliszek (yoctozepto)]

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?

[Radosław Piliszek (yoctozepto)]

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

[Radosław Piliszek (yoctozepto)]

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

[Brian Haley (brian-haley)]

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.

[Jeremy Stanley (fungi)]

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 .

[Radosław Piliszek (yoctozepto)]

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.

[Slawek Kaplonski (slaweq)]

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.

[Brian Haley (brian-haley)]

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.

[Jeremy Stanley (fungi)]

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.

[Daniel Alvarez (dalvarezs)]

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.

[OpenStack Infra (hudson-openstack)]

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

[Radosław Piliszek (yoctozepto)]

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

@Brian - ack, agreed.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/queens
Review: https://review.opendev.org/711103

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[Nick Tait (nickthetait)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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