Nova libvirt driver live migration should sanitize target host

[Paul McMillan]

In nova, an administrator can specify the target host for a libvirt live migrate action.

This host is formatted into a base string (default="qemu+tcp://%s/system")
https://github.com/openstack/nova/blob/744fa6b7b88b131e0b9f5a1eca88b14a7351b540/nova/virt/libvirt/driver.py#L158
and then passed directly to libvirt as a target URI:
https://github.com/openstack/nova/blob/744fa6b7b88b131e0b9f5a1eca88b14a7351b540/nova/virt/libvirt/driver.py#L4270
dom.migrateToURI(CONF.libvirt.live_migration_uri % dest,

The host does not appear to be validated, stripped, or otherwise checked to make sure that the value is reasonable. This allows an admin to attempt to migrate an instance out of a cloud (which may or may not be a security issue). Much more importantly, libvirt's URI format accepts many parameters in this URI, some of which allow execution of arbitrary commands at the same privilege level as libvirt.
http://libvirt.org/remote.html#Remote_URI_reference

Due to later checks it does not appear to be exploitable, but it should nevertheless be fixed to avoid future issues.

[Grant Murphy (gmurphy)]

Ouch.

Paul - Just to be clear for the remote code execution the % dest is a user controlled parameter?

[Paul McMillan (paul-mcmillan)]

Yep, it's user controlled. Here's where it happens in novaclient.

https://github.com/openstack/python-novaclient/blob/c8ad3157633b26ae324c352a73bb599eb084c115/novaclient/v3/servers.py#L300

https://github.com/openstack/python-novaclient/blob/c8ad3157633b26ae324c352a73bb599eb084c115/novaclient/v3/servers.py#L909

[Paul McMillan (paul-mcmillan)]

It's worth noting that this applies anywhere a user-influenced libvirt URI might be in use. I'm not aware of any other user-controlled places, but they might exist in rendered XML templates, etc.

[Paul McMillan (paul-mcmillan)]

(that is to say, the problem with URIs is not restricted to migration, that's just where it happened to occur)

[Daniel Berrange (berrange)]

And this is why code shouldn't just do string substitution / concatenation to create URIs, or XML documents, etc. APIs exist for manipulating URIs, to ensure correct escaping rules are followed, precisely to avoid such flaws :-)

[Thierry Carrez (ttx)]

I suspect that affects grizzly and havana as well...

[Grant Murphy (gmurphy)]

I might have a quick look to see if there are other places where we are doing unsafe substitution like this. Assuming that I don't find anything does this sound reasonable for the impact description?

--

Title: Command Injection via Live Migration
Reporter: Paul McMillan (?)
Products: Nova
Affects: All supported versions

Description:
Paul McMillan of (?) reported a remote command injection vulnerability in Nova. By supplying specially crafted host destination for the migration it is possible for an authenticated user to execute commands as a privileged user on the Nova backend. Only setups using libvirt for migration are affected by this flaw.

[Daniel Berrange (berrange)]

Sounds ok to me.

[Thierry Carrez (ttx)]

Agreed, we should first check if there are other cases of user-controlled variables being injected in libvirt URLs. Then just plug that specific hole(s) using a basic patch, and publish OSSA quick. Then in a future version it might make sense to strengthen the whole thing by going through APIs instead of manipulating those ourselves.

About impact desc: Paul works for Nebula. I would say "specially-crafted host destination for *live* migration". I would mention "on Nova *compute nodes*" (instead of Nova backend), maybe mention "libvirt user" instead of "privileged user". I wouldn't mention "setups using libvirt for migration", but "setups using the libvirt driver" (not mention "for migration" unless there is a way to disable live migration in configuration).

[Bryan D. Payne (bdpayne)]

+1 to Thierry's suggestions

[Grant Murphy (gmurphy)]

Thanks ttx. I've attempted to incorporate these suggestions. (Also note the 'Affects' tweak). Actually this is exactly the kind of thing the security fuzzing / testing work the OSSG are talking about would be good at detecting.

------

Title: Command Injection via Live Migration
Reporter: Paul McMillan (Nebula)
Products: Nova
Affects: Essex to Icehouse

Description:
Paul McMillan of Nebula reported a remote command injection vulnerability in Nova. By supplying specially crafted host destination for live migration it is possible for an authenticated user to execute commands as the libvirt user on Nova compute nodes. Only setups using the libvirt driver are affected by this flaw.

[Thierry Carrez (ttx)]

Following proposed new rules, we'd replace the Affects line to look like this:
Versions: 2012.1 to 2013.1.4, and 2013.2 versions up to 2013.2.1

2013.2.2 shall be out this week, so depending on when we release this, that would even become:
Versions: 2012.1 to 2013.1.4, and 2013.2 versions up to 2013.2.2

Maybe mention "Nova" in the title ("Nova command injection via live migration"). I know it's redundant but that's how we did it so far. We also generally don't use capitalization.

Otherwise the description looks good.

[Daniel Berrange (berrange)]

Changing bug title to reflect fact that flaw is in nova libvirt driver, not libvirt itself.

[John Garbutt (johngarbutt)]

I thought the scheduler code validates to see if the host is up and not the same as the current node, which might protect you a tiny bit. But maybe we bypass that on a user selected host?

Also just wondering if there are calls to check can live migrate to that fake host the timeout before this fails. It might only by Havana onwards though.

Either way, from a denfense in depth point of view, we need to stop doing this kind of thing, and check were we are at.

[Nikola Đipanov (ndipanov)]

I have to say that after looking at this I am not sure this is an exploitable issue as described in the bug.

Basically the live migration code in the conductor (tasks) service will run the following code for the destination host:

 def _check_host_is_up(self, host):
        try:
            service = db.service_get_by_compute_host(self.context, host)
        except exception.NotFound:
            raise exception.ComputeServiceUnavailable(host=host)

        if not self.servicegroup_api.service_is_up(service):
            raise exception.ComputeServiceUnavailable(host=host)

see nova/conductor/tasks/live_migrate.py

Now this will obviously fail, unless there is some way to tamper with the host value in the corresponding service row in the db as well. As far as I could tell this is not possible through the admin API. The host is set when the service is created to the value of the CONF.netconf.host option (result of socket.gethostname() call by default) which is done on service start, and I do not see an easy way to tamper with that without already having access to the machine which makes this whole exercise kind of pointless.

Fruther to this - I don't think that escaping the url or building it in a different way is the straightforward solution here. There is no easy way to white-list hostnames without actually doing the complete libvirt url parsing on Nova side and then white-listing what commands can be run. Confirm that the 'dest' is an actual hostname in the cluster seems like a more straightforward solution here, which we are already doing.

The code above was introduced in havana, so I will look into Grizzly now and see if it may be vulnerable.

And of course - if I am missing something obvious here - please do point it out!

[Grant Murphy (gmurphy)]

Ok so if it is not actually exploitable then we will have to retract the CVE and then fix this as security hardening (if at all?).
You could do something like this to ensure no bad input (e.g. command=) could make its way to the libvirt call.

def make_validator():
  import re
  whitelist = re.compile('^[\w:.-]+$')
  def check(dest):
    return whitelist.match(dest) != None

  return check

valid_destination = make_validator()
if not valid_destination(dest):
    die_a_horrible_death()

[Thierry Carrez (ttx)]

Let's wait for the original reporter (Paul) to confirm or deny John/Nikola's analysis. If this really is shallow (as an attack vector) then we can implement extra validation as a public bugfix.

[Nikola Đipanov (ndipanov)]

Also we might want to hold off canceling the CVE, as grizzly may be vulnerable. I will check back later when I get a chance to look into it.

[Nikola Đipanov (ndipanov)]

After looking at the grizzly tree - it seems that the situation is more or less the same there except the check happens in the scheduler service in the scheduler driver base class. take a look at nova.shceduler.driver _live_migration_dest_check method.

We might want to harden it just in case - yes, tho I am not sure a regex is the best way to do it, but we can take that onto the public review.

And of course - we definitely need a second opinion... at least!

[Thierry Carrez (ttx)]

@Paul: any chance you could confirm or deny Nikola's analysis ?

[Paul McMillan (paul-mcmillan)]

@Thierry: after looking it over carefully, I think I agree with Nikola's analysis. I don't think the code ends up getting called (at least in the standard workflow) without that check, so I agree we can make this public, and fix it using the standard flow.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/116707
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=e6f59d02ee56479bf13920867f921cd11c66948a
Submitter: Jenkins
Branch: master

commit e6f59d02ee56479bf13920867f921cd11c66948a
Author: Karen Noel <email address hidden>
Date: Sat Aug 23 13:42:42 2014 -0400

    libvirt: add validation of migration hostname

    The migration hostname is substituted into the libvirt migration
    URI string. If the hostname contains special characters it might
    be possible to construct a malicious migration URI causing libvirt
    to execute arbitrary commands as root. Protect against such a risk
    by validating the hostname characters.

    Change-Id: Ia3c53df1a842b5ef88f1f7ed5a3f4121df698161
    Closes-bug: #1276862