OpenStack Compute (Nova)

[OSSA 2013-008] DOS by allocating all fixed ips

Reported by Vish Ishaya on 2013-02-14
268
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
High
Michael Still
Essex
High
Michael Still
Folsom
High
Michael Still
OpenStack Security Advisory
Undecided
Thierry Carrez

Bug Description

When using FlatDHCP networking it is possible for a user to use up all of the fixed ips by repeatedly calling addFixedIp to an instance.

Repro case allocating 100 fixed ips:

nova boot --flavor=1 --image=<image> foo
for i in {1..100}; do
  nova add-fixed-ip foo
done

When all ips are exhausted it is impossible for anyone to boot a vm

Note this is possible in vlan mode, but it would only DOS a single tenant in this case.

Vish Ishaya (vishvananda) wrote :

IMO the easy fix for folsom/essex is probably just to disable/remove the add-fixed-ip code. For grizzly we should be enforcing this with a quota.

Vish Ishaya (vishvananda) wrote :

The offending command is in the multinic extension.

Vish Ishaya (vishvananda) wrote :

another potential easy fix for essex/folsom would be to disable multinic via policy.

Dan Prince (dan-prince) wrote :

Vish:

For Folsom/Essex: Are we sure people aren't using that code? I like the idea of updating the default policy to disallow it slightly better (although this could be a breaking change too).

For Grizzly using a quota sounds reasonable. Something like 'quota_fixed_ips'?

Vish Ishaya (vishvananda) wrote :

Dan: I'm not sure that no-one is using it. RAX uses it but enforces quotas in melange. Turning it off by default in policy is fine with me.

Didn't you do quotas for something else recently? I haven't taken a look at the quota code recently, so if you want to take a swing at it that would be awesome.

Thierry Carrez (ttx) on 2013-02-18
no longer affects: nova/grizzly
Thierry Carrez (ttx) wrote :

Hmm, that one is a bit tricky. Disabling the whole feature in a stable update sounds like a bad idea, this can hardly be considered a fix for the vulnerability, at best a workaround.

One alternative would be to issue a "security note" instead, advising people to disable it...
Adding markmc for the stable team for their opinion.

Changed in nova:
status: New → Incomplete
Mark McLoughlin (markmc) wrote :

The rationale for disabling the feature is that only RAX is using it and they enforce quotas separately?

I don't like assuming RAX is the only one using it. We have enough users now that it's impossible to predict what people may be doing. If we publish this issue, recommend disabling the extension and there are people who are using it ... then we've nicely screwed them.

How about we cook up the quotas patch and see how invasive it is? It might be backportable with manageable regression risk

Michael Still (mikalstill) wrote :

I took a quick look at folsom, and one small wart I've found is that I'd like to make the quota per project and the fixips model object doesn't store project id. Do people think this quota should be per project or per instance? If its per instance isn't it still pretty easy to DoS people? You just have to start a bunch of instances as well.

Michael Still (mikalstill) wrote :

Ahhh, I can do a joined load. Here's a patch for folsom. Its untested (not even unit tests) because I have to leave for the airport in a minute. I'll try to take another look at this on the flight.

The folsom changes don't see too invasive to me, noting that I've never done a quota change before and might have that bit horribly wrong.

Michael Still (mikalstill) wrote :

And now that I have stopped travelling, here's a version which passes unit tests. Do people think this is too invasive for folsom?

Vish Ishaya (vishvananda) wrote :

some errors in the above patch:

in db/api.py the method is called floating_ip_count_by_project, when it should be fixed_ip...

the exception should be a new exception FixedIp... not FloatingIp.

I don't think the patch is too invasive. We don't need a db migration right?

Changed in nova:
status: Incomplete → Triaged
Russell Bryant (russellb) wrote :

Agreed that this does not look too invasive for folsom.

Michael Still (mikalstill) wrote :

I can't see why we'd need a migration. Sorry for the typos. I blame jet lag.

Michael Still (mikalstill) wrote :

This version fixes Vishy's comments.

Mark McLoughlin (markmc) wrote :

Agree it's not too invasive for Folsom

Looks good except you need a QUOTAS.rollback() if there's an exception between the reserve() and commit()

Michael Still (mikalstill) wrote :

Sorry I didn't get a chance to progress this today... I will work on this more tomorrow.

Michael Still (mikalstill) wrote :

So, this took a long time to test and get right, but this new version of the folsom patch has been tested with devstack and appears to work. It includes Mark's request for a rollback of quota allocations on exceptions, and required re-writing the database code to count fixed_ips. I believe this patch is good to go for Folsom, but would appreciate comments from nova-cores.

Changed in nova:
assignee: nobody → Michael Still (mikalstill)
Mark McLoughlin (markmc) wrote :

Looks pretty good to me. I guess we're not so worried about rollback if a failure happens in deallocate_fixed_ip().

Michael Still (mikalstill) wrote :

Yeah, I did think about that, but we don't care about rollback for floating ips, so I figure we similarly don't care for fixed ips. I've just found a PEP8 failure in this code, so I'll send yet another patch which fixes that in a minute.

Michael Still (mikalstill) wrote :

Ok, here are patches for essex and folsom, both of which have been tested with devstack and pass PEP8. The Folsom one also passes unit tests, but I can't get the essex unit tests to run for me, so that one hasn't been done.

@ttx -- are you sure we don't need a patch for grizzly? That's how I am reading your "no longer affects nova/grizzly" above. Am I confused?

Michael Still (mikalstill) wrote :
Thierry Carrez (ttx) wrote :

We need a patch for Grizzly, it's just that grizzly does not need a specific series task. it's tracked in the main bugtask. We only use series tasks when we need a stable/* backport. The main bugtask tracks "master" (master happening to be grizzly right now). So it's not "no longer affects Grizzly", it's "removing extraneous bugtask".

Michael Still (mikalstill) wrote :

Ok, here's a patch for grizzly. Again, passes unit tests and devstack twiddling.

Michael Still (mikalstill) wrote :

It would be cool if other nova-core's could check each of these three patches and see what they think.

Thierry Carrez (ttx) wrote :

Adding nova-core for review on this bug.

Sean Dague (sdague) wrote :

(reviewing the grizzly patch): All the quota tests seem to set fixed_ips quota just to the default of 10, which isn't a very good update test. That should be updated. Other than that this approach seems sane.

Sean -- I'm happy to tweak the unit tests. Are there any other
comments from others?

Michael

@Mikal: wanna have a try at the impact description ? See https://wiki.openstack.org/wiki/VulnerabilityManagement for templates.

Michael Still (mikalstill) wrote :

@ttx -- I pinged you with a draft of one the other day, but perhaps you didn't see it. How about something like this?

-----

SUBJECT: Vulnerability in OpenStack Nova

This is an advance warning of a vulnerability discovered in OpenStack,
to give you, as downstream stakeholders, a chance to coordinate the
release of fixes and reduce the vulnerability window. Please treat the
following information as confidential until the proposed public
disclosure date.

Title: DoS by allocating all Fixed IPs
Impact: Medium
Reporter: Vish Ishaya (Nebula)
Products: Nova
Affects: All versions

Description:
Vish Ishaya reported a vulnerability in Nova where there is no quota
for Fixed IPs. Previously the instance quota acted as a proxy for
a Fixed IP quota, but if your configuration allows an instance to
consume more than one Fixed IP via an extension such as multinic
then this is no longer true. Running out of Fixed IPs would result in
not being able to spawn new instances.

Proposed patches:
See attached patches. Unless a flaw is discovered in them, these
patches will be merged to Nova master (Grizzly), stable/folsom,
and stable/essex branches on the public disclosure date.

CVE:
No CVE has been assigned to this issue yet.

Proposed public disclosure date/time:
Tuesday, March 12, 1500UTC
Please do not make the issue public (or release public patches) before
this coordinated embargo date.

Regards,

-----

If we're running with that disclosure date we should probably send this out ASAP. Perhaps you could do it overnight?

Thierry Carrez (ttx) wrote :

+1 on the description. Looks like we're missing some approvals on that patch, though. If we gather them today then we can send this baby out with a disclosure date of Thursday, March 14.

Michael Still (mikalstill) wrote :

Thanks ttx. At this point I think we should handle Sean's request for more unit tests for trunk in a separate patch. I'm happy to do that work, I just don't want to block this fix. Given I am trapped in a training course yet again today, can you please run with getting this disclosure email sent when we're ready?

Russell Bryant (russellb) wrote :

Grizzly patch looks good to me.

Russell Bryant (russellb) wrote :

Notification sent to downstream stakeholders.

Thierry Carrez (ttx) wrote :

Proposed public disclosure date/time:
Thursday, March 14, 1500UTC

Thierry Carrez (ttx) wrote :

Adding Stable and Essex maintainers

Jamie Strandboge (jdstrand) wrote :

FYI, applying the essex patch to 2012.1.3+stable-20120827 results in several unit test errors (see attached).

Michael Still (mikalstill) wrote :

Attempting to fetch that attachment gets me a "processing error" message from LP.

Jamie Strandboge (jdstrand) wrote :

I was able to fetch it by clicking on it, then using 'Save page as' in my browser. If using wget or similar, that won't work because the bug is private.

Michael Still (mikalstill) wrote :

Jamie -- I got it working by waiting a bit and trying again.

Michael Still (mikalstill) wrote :

Jamie, sorry about that. Does this new patch work for you?

Jamie Strandboge (jdstrand) wrote :

The updated patch works with the version of essex in Ubuntu 12.04 LTS, yes. Thanks!

information type: Private Security → Public Security

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

Changed in nova:
status: Triaged → In Progress

Reviewed: https://review.openstack.org/24452
Committed: http://github.com/openstack/nova/commit/9561484166f245d0e4602a36351d6cac72dd9426
Submitter: Jenkins
Branch: stable/folsom

commit 9561484166f245d0e4602a36351d6cac72dd9426
Author: Michael Still <email address hidden>
Date: Wed Mar 13 04:44:14 2013 +1100

    Add quotas for fixed ips.

    DocImpact: there is now a default quota of 10 fixed ips per tenant.
    This will need to be adjusted by deployers if that number does not
    meet their needs.

    Resolves bug 1125468 for folsom.

    Change-Id: I970d540cfa6a61b7e903703f845a6453ff55f225

Reviewed: https://review.openstack.org/24453
Committed: http://github.com/openstack/nova/commit/efaacdaee116388234558e2682b647d41fe5b149
Submitter: Jenkins
Branch: stable/essex

commit efaacdaee116388234558e2682b647d41fe5b149
Author: Michael Still <email address hidden>
Date: Fri Mar 15 03:45:41 2013 +1100

    Add quotas for fixed ips.

    DocImpact: there is now a default quota of 10 fixed ips per tenant.
    This will need to be adjusted by deployers if that number does not
    meet their needs.

    Resolves bug 1125468 for essex.

    Change-Id: I2a5afaa47afb182f4917cd43e1ebd0d6cd1330e3

Reviewed: https://review.openstack.org/24451
Committed: http://github.com/openstack/nova/commit/99429214d4ddb5bdc7de185693b8a53ad50df3c6
Submitter: Jenkins
Branch: master

commit 99429214d4ddb5bdc7de185693b8a53ad50df3c6
Author: Michael Still <email address hidden>
Date: Tue Mar 5 05:53:43 2013 +1100

    Add quotas for fixed ips.

    DocImpact: there is now a default quota of 10 fixed ips per tenant.
    This will need to be adjusted by deployers if that number does not
    meet their needs.

    Resolves bug 1125468.

    Change-Id: Iffa19583340f80cb2a13ba5fce31f7ff724a52d6

Changed in nova:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2013-03-20
Changed in nova:
milestone: none → grizzly-rc1
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-04-04
Changed in nova:
milestone: grizzly-rc1 → 2013.1
Thierry Carrez (ttx) on 2013-05-24
summary: - DOS by allocating all fixed ips
+ [OSSA 2013-008] DOS by allocating all fixed ips
Changed in ossa:
assignee: nobody → Thierry Carrez (ttx)
status: New → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers