project_id could be overwritten to any value by URI value

fix is pretty easy, but fixing the tests afterwards is really annoying.

diff --git a/nova/api/openstack/wsgi.py b/nova/api/openstack/wsgi.py
index 1ffca9e..5760f88 100644
--- a/nova/api/openstack/wsgi.py
+++ b/nova/api/openstack/wsgi.py
@@ -560,8 +560,10 @@ class Resource(wsgi.Application):
             return Fault(webob.exc.HTTPBadRequest(explanation=msg))

         project_id = args.pop("project_id", None)
- if 'nova.context' in request.environ and project_id:
- request.environ['nova.context'].project_id = project_id
+ if ('nova.context' in request.environ and project_id
+ and project_id != request.environ['nova.context'].project_id):
+ msg = _("Malformed request url")
+ return Fault(webob.exc.HTTPBadRequest(explanation=msg))

         try:
             action_result = self.dispatch(request, action, args)

FYI This change breaks the tests badly. I have fixes for the tests in trunk, but it is kind of big. Included.

#approve

BTW, I saw this patch fix a live environment.

#approve

We'll need to coordinate this one downstream. I'll ask a CVE and send downstream, probably tomorrow.

Time is running short to disclose this in a coordinated fashion before the holiday weeks. It's clearly not best practice to release a security advisory the days before Christmas.

Nachi: are you comfortable with not disclosing this in the next two weeks (and wait for the first days of January for coordinating the disclosure downstream) ? Are you the original finder that should be credited for finding this ?

Brian: what do you mean by "I saw this patch fix a live environment" ? How public is this already ?

Vish: there is no way to make the patch shorter ? :)

I'm preparing the impact statement / CVE request and need a bit more information about impact. My understanding is that an authentified user can issue commands for any project... Am I right in assuming that (1) you need to be authentified, (2) you can issue any command and (3) this affects both EC2 and OSAPI ?

In particular for (2), I suspect that the user cannot issue *any* command, but just the ones that he would be... entitled to on projects belonging to him ? How do we map roles/users/projects usually ?

ttx: Sorry, I should have been a bit clearer in my comment. I helped somebody on my team reproduce this bug in an existing (private) environment then apply the proposed fix and verify the bug was no longer reproducible.

ttx

1) you still need to be authenticated. The keystone middleware doesn't let the request hit the router if you fail to auth.

2) you can issue any openstack api request against any tenant.

Because most logic in the API uses the context (not the URL) to specify the resources you are interacting with (for example list servers uses the tenant from the context)

        instance_list = self.compute_api.get_all(context,
                                                 search_opts=search_opts)

https://github.com/openstack/nova/blob/master/nova/api/openstack/v2/servers.py#L145

So you can create a server in another project but delete, list, show (and most other api calls) will not be able to modify it by via the api. This is because the way that the database layer is written is defensive - it only lets you access data that your context says you have access to.

This is still bad in that it allows creation of resources outside of your project (for quota/billing avoidance) and there could be more tunnels.

3) only affects if you are the openstack api using keystone (not deprecated auth)

It shouldn't affect people who use the default (which is nova's internal legacy auth) or who use the ec2 api

Comments below

On Dec 16, 2011, at 1:30 PM, anotherjesse wrote:

> ttx
>
> 1) you still need to be authenticated. The keystone middleware doesn't
> let the request hit the router if you fail to auth.
>
> 2) you can issue any openstack api request against any tenant.
>
> Because most logic in the API uses the context (not the URL) to specify
> the resources you are interacting with (for example list servers uses
> the tenant from the context)
>
> instance_list = self.compute_api.get_all(context,
> search_opts=search_opts)
>
> https://github.com/openstack/nova/blob/master/nova/api/openstack/v2/servers.py#L145
>
> So you can create a server in another project but delete, list, show
> (and most other api calls) will not be able to modify it by via the api.
> This is because the way that the database layer is written is defensive
> - it only lets you access data that your context says you have access
> to.
>
> This is still bad in that it allows creation of resources outside of
> your project (for quota/billing avoidance) and there could be more
> tunnels.

I believe this is incorrect. The broken code actually sets context.project_id = to whatever was in the url, so I think you can list and terminate instances in other projects as well.
>
> 3) only affects if you are the openstack api using keystone (not
> deprecated auth)
>
> It shouldn't affect people who use the default (which is nova's internal
> legacy auth) or who use the ec2 api

legacy auth is also broken afaik, because it will successfully authenticate and then the authentication will be overwritten by the url. EC2 auth shouldn't be affected.
>
> --
> You received this bug notification because you are a member of Nova
> Core, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/904072
>
> Title:
> project_id could be overwritten to any value by URI value
>
> Status in OpenStack Compute (Nova):
> Confirmed
>
> Bug description:
> project_id could be overwritten to any value by URI value.
> User can create server on any project (even if it is not exist)
> Quota function will not be working because of this bug. (So This bug is a security vulnerability)
>
> --Test condition
> user : demo (not admin)
> project_id: 2
>
> -- Requeest
> stack@freecloud116:~/devstack$ curl -H "Content-type: application/json" -H "x-auth-token: 8bbdb554-a671-446e-a6c2-07326eeb9ad5" -d '{"server": {"min_count": 1, "flavorRef": "1", "name": "test5", "imageRef": "2", "max_count": 1}}' http://localhost:8774/v1.1/tushar2/servers <-- invalid tenant_id
>
> -- Response
> {"server": {"status": "BUILD", "updated": "2011-12-14T03:06:04Z", "hostId": "", "user_id": "demo", "name": "test5", "links": [{"href": "http://localhost:8774/v1.1/tushar2/servers/7", "rel": "self"}, {"href": "http://localhost:8774/tushar2/servers/7", "rel": "bookmark"}], "addresses": {}, "tenant_id": "tushar2", "image": {"id": "2", "links": [{"href": "http://localhost:8774/tushar2/images/2", "rel": "bookmark"}]}, "created": "2011-12-14T03:06:04Z", "uuid": "5b63c965-8966-4ebc-ae1b-1c1c551a044c", "accessIPv4": "", "accessIPv6": "", "key_name": null, "...

Hi All

>ttx
I'm ok to not disclosing this in the next two
weeks. Me, Rohit and Ravi found this bug.

>Vish
Thank you for your patch.
I have one question. Why we still set project_id from URL?

Cheers
Nati

2011/12/16 Vish Ishaya <email address hidden>:
> Comments below
>
> On Dec 16, 2011, at 1:30 PM, anotherjesse wrote:
>
>> ttx
>>
>> 1) you still need to be authenticated.  The keystone middleware doesn't
>> let the request hit the router if you fail to auth.
>>
>> 2) you can issue any openstack api request against any tenant.
>>
>> Because most logic in the API uses the context (not the URL) to specify
>> the resources you are interacting with (for example list servers uses
>> the tenant from the context)
>>
>>        instance_list = self.compute_api.get_all(context,
>>                                                 search_opts=search_opts)
>>
>> https://github.com/openstack/nova/blob/master/nova/api/openstack/v2/servers.py#L145
>>
>> So you can create a server in another project but delete, list, show
>> (and most other api calls) will not be able to modify it by via the api.
>> This is because the way that the database layer is written is defensive
>> - it only lets you access data that your context says you have access
>> to.
>>
>> This is still bad in that it allows creation of resources outside of
>> your project (for quota/billing avoidance) and there could be more
>> tunnels.
>
> I believe this is incorrect.  The broken code actually sets context.project_id = to whatever was in the url, so I think you can list and terminate instances in other projects as well.
>>
>> 3) only affects if you are the openstack api using keystone (not
>> deprecated auth)
>>
>> It shouldn't affect people who use the default (which is nova's internal
>> legacy auth) or who use the ec2 api
>
> legacy auth is also broken afaik, because it will successfully authenticate and then the authentication will be overwritten by the url.  EC2 auth shouldn't be affected.
>>
>> --
>> You received this bug notification because you are a member of Nova
>> Core, which is subscribed to the bug report.
>> https://bugs.launchpad.net/bugs/904072
>>
>> Title:
>>  project_id could be overwritten to any value by URI value
>>
>> Status in OpenStack Compute (Nova):
>>  Confirmed
>>
>> Bug description:
>>  project_id could be overwritten to any value by URI value.
>>  User can create server on any project (even if it is not exist)
>>  Quota function will not be working because of this bug. (So This bug is a security vulnerability)
>>
>>  --Test condition
>>  user : demo (not admin)
>>  project_id: 2
>>
>>  -- Requeest
>>  stack@freecloud116:~/devstack$ curl -H "Content-type: application/json" -H "x-auth-token: 8bbdb554-a671-446e-a6c2-07326eeb9ad5" -d '{"server": {"min_count": 1, "flavorRef": "1", "name": "test5", "imageRef": "2", "max_count": 1}}' http://localhost:8774/v1.1/tushar2/servers  <-- invalid tenant_id
>>
>>  -- Response
>>  {"server": {"status": "BUILD", "updated": "2011-12-14T03:06:04Z", "hostId": "", "user_id": "demo", "name": "test5", "links": [{"href": "http://localhost:8774/v1.1/tushar2/servers/7", "rel": "self"}, {"href": "http://localhost:8774/tushar2/servers/7",...

On Dec 16, 2011, at 5:11 PM, Nachi Ueno wrote:

> Hi All
>
>> ttx
> I'm ok to not disclosing this in the next two
> weeks. Me, Rohit and Ravi found this bug.
>
>> Vish
> Thank you for your patch.
> I have one question. Why we still set project_id from URL?

We don't.

It raises an exception if project_id in the url doesn't match project_id in the context. I think this is the right behavior.

>
> Cheers
> Nati
>
> 2011/12/16 Vish Ishaya <email address hidden>:
>> Comments below
>>
>> On Dec 16, 2011, at 1:30 PM, anotherjesse wrote:
>>
>>> ttx
>>>
>>> 1) you still need to be authenticated. The keystone middleware doesn't
>>> let the request hit the router if you fail to auth.
>>>
>>> 2) you can issue any openstack api request against any tenant.
>>>
>>> Because most logic in the API uses the context (not the URL) to specify
>>> the resources you are interacting with (for example list servers uses
>>> the tenant from the context)
>>>
>>> instance_list = self.compute_api.get_all(context,
>>> search_opts=search_opts)
>>>
>>> https://github.com/openstack/nova/blob/master/nova/api/openstack/v2/servers.py#L145
>>>
>>> So you can create a server in another project but delete, list, show
>>> (and most other api calls) will not be able to modify it by via the api.
>>> This is because the way that the database layer is written is defensive
>>> - it only lets you access data that your context says you have access
>>> to.
>>>
>>> This is still bad in that it allows creation of resources outside of
>>> your project (for quota/billing avoidance) and there could be more
>>> tunnels.
>>
>> I believe this is incorrect. The broken code actually sets context.project_id = to whatever was in the url, so I think you can list and terminate instances in other projects as well.
>>>
>>> 3) only affects if you are the openstack api using keystone (not
>>> deprecated auth)
>>>
>>> It shouldn't affect people who use the default (which is nova's internal
>>> legacy auth) or who use the ec2 api
>>
>> legacy auth is also broken afaik, because it will successfully authenticate and then the authentication will be overwritten by the url. EC2 auth shouldn't be affected.
>>>
>>> --
>>> You received this bug notification because you are a member of Nova
>>> Core, which is subscribed to the bug report.
>>> https://bugs.launchpad.net/bugs/904072
>>>
>>> Title:
>>> project_id could be overwritten to any value by URI value
>>>
>>> Status in OpenStack Compute (Nova):
>>> Confirmed
>>>
>>> Bug description:
>>> project_id could be overwritten to any value by URI value.
>>> User can create server on any project (even if it is not exist)
>>> Quota function will not be working because of this bug. (So This bug is a security vulnerability)
>>>
>>> --Test condition
>>> user : demo (not admin)
>>> project_id: 2
>>>
>>> -- Requeest
>>> stack@freecloud116:~/devstack$ curl -H "Content-type: application/json" -H "x-auth-token: 8bbdb554-a671-446e-a6c2-07326eeb9ad5" -d '{"server": {"min_count": 1, "flavorRef": "1", "name": "test5", "imageRef": "2", "max_count": 1}}' http://localhost:8774/v1.1/tushar2/servers...

Hi Vish

Sorry, I misread your patch. It is right behavior.

2011/12/17 Vish Ishaya <email address hidden>:
> On Dec 16, 2011, at 5:11 PM, Nachi Ueno wrote:
>
>> Hi All
>>
>>> ttx
>> I'm ok to not disclHosing this in the next two
>> weeks. Me, Rohit and Ravi found this bug.
>>
>>> Vish
>> Thank you for your patch.
>> I have one question. Why we still set project_id from URL?
>
> We don't.
>
> It raises an exception if project_id in the url doesn't match project_id
> in the context.  I think this is the right behavior.
>
>>
>> Cheers
>> Nati
>>
>> 2011/12/16 Vish Ishaya <email address hidden>:
>>> Comments below
>>>
>>> On Dec 16, 2011, at 1:30 PM, anotherjesse wrote:
>>>
>>>> ttx
>>>>
>>>> 1) you still need to be authenticated.  The keystone middleware doesn't
>>>> let the request hit the router if you fail to auth.
>>>>
>>>> 2) you can issue any openstack api request against any tenant.
>>>>
>>>> Because most logic in the API uses the context (not the URL) to specify
>>>> the resources you are interacting with (for example list servers uses
>>>> the tenant from the context)
>>>>
>>>>        instance_list = self.compute_api.get_all(context,
>>>>                                                 search_opts=search_opts)
>>>>
>>>> https://github.com/openstack/nova/blob/master/nova/api/openstack/v2/servers.py#L145
>>>>
>>>> So you can create a server in another project but delete, list, show
>>>> (and most other api calls) will not be able to modify it by via the api.
>>>> This is because the way that the database layer is written is defensive
>>>> - it only lets you access data that your context says you have access
>>>> to.
>>>>
>>>> This is still bad in that it allows creation of resources outside of
>>>> your project (for quota/billing avoidance) and there could be more
>>>> tunnels.
>>>
>>> I believe this is incorrect.  The broken code actually sets context.project_id = to whatever was in the url, so I think you can list and terminate instances in other projects as well.
>>>>
>>>> 3) only affects if you are the openstack api using keystone (not
>>>> deprecated auth)
>>>>
>>>> It shouldn't affect people who use the default (which is nova's internal
>>>> legacy auth) or who use the ec2 api
>>>
>>> legacy auth is also broken afaik, because it will successfully authenticate and then the authentication will be overwritten by the url.  EC2 auth shouldn't be affected.
>>>>
>>>> --
>>>> You received this bug notification because you are a member of Nova
>>>> Core, which is subscribed to the bug report.
>>>> https://bugs.launchpad.net/bugs/904072
>>>>
>>>> Title:
>>>>  project_id could be overwritten to any value by URI value
>>>>
>>>> Status in OpenStack Compute (Nova):
>>>>  Confirmed
>>>>
>>>> Bug description:
>>>>  project_id could be overwritten to any value by URI value.
>>>>  User can create server on any project (even if it is not exist)
>>>>  Quota function will not be working because of this bug. (So This bug is a security vulnerability)
>>>>
>>>>  --Test condition
>>>>  user : demo (not admin)
>>>>  project_id: 2
>>>>
>>>>  -- Requeest
>>>>  stack@freecloud116:~/devstack$ curl -H "Content-type: application/json" -H "x-auth-token: 8bbdb554...

OK, we'll start coordinating fix and disclosure starting January 3rd.

@Nachi: do you want credit to go to "Nachi Ueno, Rohit Karajgi and Ravi (add last name here)" or to some specific lab or entity ("Researchers at NTT DATA") or both ("Nachi Ueno, Rohit Karajgi and Ravi (add last name here) from NTT DATA") ?

@Jesse/Vish/Everyone, please confirm proposed impact statement:

"$CREDIT discovered a vulnerability in Nova API nodes handling of incoming requests. An authenticated user may craft malicious commands to affect resources on tenants he is not a member of, potentially leading to incorrect billing, quota escaping or compromise of computing resources created by a third-party. Only setups allowing the OpenStack API are affected."

impact statement looks good. I will have to fix up the test fixes as well when this is ready to go in. The patch itself is small, so maybe I will put together a check to turn it off for the tests, then I can remove the check and fix all the tessts in a subsequent patch.

Nachi says credit should go to "Nachi Ueno from NTT PF lab, Rohit Karajgi from Vertex and Venkatesan Ravikumar from HP"

I'm split, I fear we'll have to communicate the testfix.patch downstream as well, even if it makes a bit of a giant patch. Separating them should make it quite clear that the fix is simple.

@Vish: could you attach updated patches (fix.patch + testfix.patch) that I can use in the downstream announcement ?

Here is the vuln patch

This patch removes the testing workaround and fixes all of the tests.

Message sent to downstream, embargo set to Wednesday, January 11, 2012, 1500UTC

Adding Dave Walker for diablo/stable coordination. Markmc should have access through nova-core already.

Adding ubuntu-security to help them coordinate disclosure.

Are the patches in comment #18 and #19 considered official and blessed by upstream?

@Jamie: Yes, those are the ones we included in the "downstream stakeholders" post to <email address hidden> you should have received.

FYI, the testfix patch is not likely to apply cleanly to stable/diablo or any other branch. The first patch should apply cleanly though. We need to either:
a) be ok with the workaround to keep tests working in the first patch
or
b) do some backporting to fix the tests

I took a crack at doing the backport, but need help. I sent this to <email address hidden>:

"nova 2011.3 needs a slightly different patch for the vulnerability.
Specifically, nova/api/openstack/wsgi.py needs to use 'return
faults.Fault(...)' instead of 'return Fault(...)'. I've attached this.

The testsuite proved more problematic for me. I was able to get it down
to 7 failures (one in test_quotas.py and 6 in test_servers.py). Attached
is a preliminary patch for 2011.3 that I created. It would be great if
someone upstream could verify what I've done (especially the changes to
test_quotas.py and test_servers.py) and fix the remaining testsuite
failures."

I also privately corresponded with Mark McLoughlin, so he may or may not be looking into. Attaching the preliminary patches here for completeness.

This has been assigned CVE-2012-0030.

Vish's patch against master needed some rebasing after Vek's "Serialization, deserialization, and response code decorators" commit. Nothing major.

I've taken the liberty of combining the vuln/testfix patches, rebasing and adding a commit message. I've retained Vish as the author of the commit.

Here's the patch backported to the stable branch.

Caveat: I'm having some late night weirdness with tests failing that have nothing to do with this patch, and continuing to fail after the patch is reverted. If someone could double-check that the tests pass, that'd be great.

Jamie - your patch was pretty close, actually. I started from scratch, just to be doubly sure, and ended up with an identical patch apart from fixes for the test failures you mentioned. The attached diff compares yours to mine.

Thanks Mark! This patch works great against Ubuntu's 2011.3 in 11.10 (adjusting for some fuzz and removing two test functions that didn't exist in our ealier version) and the testsuite passes here just fine. Thanks again for your help. :)

This looks fine to me, fwiw.

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

Fix proposed to branch: stable/diablo
Review: https://review.openstack.org/2961

Reviewed: https://review.openstack.org/2960
Committed: http://github.com/openstack/nova/commit/c9c09bd60e7a0e0258d218a31d7878755bea1395
Submitter: Jenkins
Branch: master

commit c9c09bd60e7a0e0258d218a31d7878755bea1395
Author: Thierry Carrez <email address hidden>
Date: Wed Jan 11 13:48:29 2012 +0100

    Do not overwrite project_id from request params

    Prevent project_id overwriting from OSAPI request parameters.
    The patch is actually very simple (nova/api/openstack/wsgi.py) but
    needs significant test adjustments (nova/tests/*) to pass.

    Fixes bug 904072. Patch from Vish Ishaya and Mark McLoughlin.

    Change-Id: I66ea0f178ce6271ec1020e9f1a73bd4e8c83ddab

Fyi, per Thierry's go ahead, I pushed updates to Ubuntu 11.10 and 12.04.

Reviewed: https://review.openstack.org/2961
Committed: http://github.com/openstack/nova/commit/3d4ffb64f1e18117240c26809788528979e3bd15
Submitter: Jenkins
Branch: stable/diablo

commit 3d4ffb64f1e18117240c26809788528979e3bd15
Author: Thierry Carrez <email address hidden>
Date: Wed Jan 11 13:48:29 2012 +0100

    Do not overwrite project_id from request params

    Prevent project_id overwriting from OSAPI request parameters.
    The patch is actually very simple (nova/api/openstack/wsgi.py) but
    needs significant test adjustments (nova/tests/*) to pass.

    Fixes bug 904072. Patch from Vish Ishaya and Mark McLoughlin.

    (cherry picked from commit c9c09bd60e7a0e0258d218a31d7878755bea1395)

    Change-Id: I66ea0f178ce6271ec1020e9f1a73bd4e8c83ddab

Published as OSSA 2012-001
https://lists.launchpad.net/openstack/msg06648.html

Rejecting from oneiric-proposed. It's a security update and should go via -security.