[OSSA 2014-032] Nova VMware driver still leaks rescued images (CVE-2014-3608)

[Grant Murphy]

Garth Mollet of Red Hat reported the following when examining the fix for OSSA 2014-017:

.. there may still be a regression in the upstream patches.

With the new patch applied it appears unrescue can still fail if the live vm is in the suspended state. With the new patch unrescue will attempt to poweroff the vm, however poweroff will fail if state == suspended:

        # Only PoweredOn VMs can be powered off.
        # Raise Exception if VM is suspended
        elif pwr_state == "suspended":
             reason = _("instance is suspended and cannot be powered off.")
             raise exception.InstancePowerOffFailure(reason=reason)

And this exception will be uncaught in the case of a manual unrescue, leading to the same end scenario in Jaroslavs test above, where destroying the vm in error state will leave the -rescue instance.

Red Hat bugzilla reference - https://bugzilla.redhat.com/show_bug.cgi?id=1108406

Can we confirm if this is a regression / incomplete fix of bug #1269418 ?

[Garth Mollett (gmollett)]

The bug in the RH bugzilla is set to private so most folks here will be unable to read it. However my comments in the post above refering to "Jaroslavs test above" is refering to testing by Jaroslav Henner that noted that a manual
unrescue would fail if the VM was powered on (due to RH missing the additional patch to power off a powered on
vm when doing unrescue).

[Grant Murphy (gmurphy)]

Original findings recorded in duplicate bug #1338929

[Grant Murphy (gmurphy)]

Ignore comment #2. Bug #1338928 was not related.

[Thierry Carrez (ttx)]

@Nova coresec, please confirm findings here

[Andrew Laski (alaski)]

There does appear to still be a gap in the previous fix. That fix assumed that cleanup only needed to happen if vm_state == RESCUED. But a rescued instance can be suspended in which case that check would fail and not try to unrescue the instance or clean up the rescued image.

It does appear that if the unrescue were attempted and then failed for any reason, perhaps due to the power state check above, Nova should still clean up the rescue instance.

[Garth Mollett (gmollett)]

As this came from Red Hat I can assign a CVE directly in this bug report without the need for the request to secalert@redhat if/when it's confirmed as needed.

[Grant Murphy (gmurphy)]

Steps to reproduce:

1. nova boot --image cirros-0.3.2-i386-disk foo --flavor 1
2. nova rescue foo
3. nova suspend foo
* foo is now in ERROR state
4. nova delete foo
* rescue instance still present and running

[Grant Murphy (gmurphy)]

Draft impact description -

Title: Nova VMWare driver leaks rescued images - incomplete fix
Reporter: Garth Mollett (Red Hat)
Products: Nova
Versions: from 2013.2 to 2013.2.3, and 2014.1 versions up to 2014.1.1

Description:
Garth Mollett from Red Hat reported an incomplete fix to CVE-2014-2573, a vulnerability affecting Nova. If an authenticated user places an instance into rescue, and then issues a suspend command it will cause the instance to enter an ERROR state. Nova does not clean up an instance in this state correctly upon deletion. An attacker can use this to launch a denial of service attack. Only setups using the Nova VMWare driver are affected by this flaw.

[Tristan Cacqueray (tristan-cacqueray)]

The impact description looks good to me, but shouldn't we do an ERRATA for 2014-017 like we did for 2012-017 ?

[Garth Mollett (gmollett)]

Obviously it's entirely up to VMT how advisories for incomplete fixes are handled.

But if it helps you at all in deciding what the process should be, MITRE and Red Hat (as a CNA for opensource), as a standard practice, do assign a new CVE to an issue when a fix is found to be incomplete after the original fix/advisory is released.

We (Red Hat product security) will also release a new erratum / security advisory if such a case occurs and we have already released one for the original issue.

That is our process though, by no means am I saying it should be yours as well. Just trying to be helpful :)

[Thierry Carrez (ttx)]

The two options are open. ERRATAs are usually when the information provided in an advisory is incorrect, and should therefore be redacted. New advisories are for when a subsequent issue is found.

Incomplete fixes fall in the middle. In this specific case, it feels like the original OSSA is still correct, but a regression / corner case was discovered in the fix. I think it's simpler to just issue a new one in that specific case (referencing the old one).

[Thierry Carrez (ttx)]

Reviewing the impact description:

I would use the title: "Nova VMWare driver still leaks rescued images"

"incomplete fix to CVE-2014-2573" -> "incomplete fix to OSSA-2014-017 (CVE-2014-2573)"

Otherwise looks good.

[Andrew Laski (alaski)]

I'm not able to test this in a VMWare environment, but the included patch looks like it should address the issue.

[Thierry Carrez (ttx)]

@nova-coresec, please review proposed patch (or add a domain specialist to the bug so that he can)

[Grant Murphy (gmurphy)]

Updated impact description including ttx recommended changes -

Title: Nova VMware driver still leaks rescued images
Reporter: Garth Mollett (Red Hat)
Products: Nova
Versions: from 2013.2 to 2013.2.3, and 2014.1 versions up to 2014.1.2

Description:
Garth Mollett from Red Hat reported an incomplete fix to OSSA-2014-017 (CVE-2014-2573), a vulnerability affecting Nova. If an authenticated user places an instance into rescue, and then issues a suspend command it will cause the instance to enter an ERROR state. Nova does not clean up an instance in this state correctly upon deletion. An attacker can use this to launch a denial of service attack. Only setups using the Nova VMware driver are affected by this flaw.

[Thierry Carrez (ttx)]

+1 on Impact desc

[Jeremy Stanley (fungi)]

Grant's updated impact description in comment #15 looks fine to me.

[Garth Mollett (gmollett)]

We has assigned an updated CVE for this.
Please use:

CVE-2014-3608 OpenStack Nova incomplete fix for CVE-2014-2573

To refer to this issue.

[Grant Murphy (gmurphy)]

@nova-coresec Does anybody with a vmware setup have the free cycles to test this patch?

[Matthew Booth (mbooth-9)]

Couple of things. Firstly, I wrote a discussion patch to change the way the vmware driver does rescue images here:

https://review.openstack.org/#/c/106078/

The basic idea is that it doesn't create a second VM, so it can't leak. However, it's dependent on a bunch of outstanding work, and needs more testing. It also needs to handle upgrades. I think it's the ultimate solution to this problem, though.

I reviewed Andrew's patch above. I think it's robust and it would be correct (although I haven't actually run it, yet). However, I'm concerned about the performance impact. It adds a call to get_vm_ref_from_name on every destroy(). This is an ugly call, and called here on a rescue image, it would almost always result in an additional call to vim_utils.get_objects(). This is an unfortunate function which traverses every object on the vsphere server to find what it's looking for.

I'll have a look at this in a bit more detail tomorrow. Hopefully I can come up with a slightly cheaper fix until we can do this properly.

[Matthew Booth (mbooth-9)]

I have previously reproduced this error, however it is no longer reproducible on master. It is not possible to suspend a rescued instance, or to rescue a suspended instance.

I will try to reproduce again on Icehouse.

[Grant Murphy (gmurphy)]

Can we please get confirmation that this is actually still an issue?

[Matthew Booth (mbooth-9)]

Sorry, forgot to update here. It is definitely reproducible on Icehouse. Patch in progress.

[Matthew Booth (mbooth-9)]

The reason this is no longer reproducible on master is commit 8ff170dc95bf3101fe38a2624e941bfa3b7c1138, which restricts the state transitions of a rescued instance. Given that the fix for this in the VMware driver in icehouse would be custom for the stable branch, I favour backporting the non-deadend patch.

It's already proposed for backport here: https://review.openstack.org/#/c/109624/

[Thierry Carrez (ttx)]

@Matthew: do you know if Havana is affected ?

[Thierry Carrez (ttx)]

We don't care about Havana anymore, so ignore me. Patch got stuck in Zuul, just rechecked it. This should be ready soon.

[Tristan Cacqueray (tristan-cacqueray)]

I've mark the OSSA task as "Fix committed" as the advance notification have been sent, the disclosure date is set to:
2014-10-02, 1500UTC