Command execution cases need to be strengthened

[Thierry Carrez]

Grant Murphy from Red Hat Product Security Team reports the following potential vulnerability:

For the most part OpenStack seems to do command execution safely using subprocess.Popen. There are two instances where things become a little dubious. The first is when shell=True is used with subprocess. This doesn't prevent arguments being supplied that allow for multiple commands to be executed. e.g. '; cat /etc/passwd'. The second case is where commands are made to an external ssh host.

See attached file for a lit of potential injections: we should double-check them (even if I expect most of them to turn false positive)

[Thierry Carrez (ttx)]

Looking into the Nova reported cases, there are 3 cases which may be exploitable:

nova/virt/powervm/blockdev.py
run_vios_command() and run_vios_command_as_root() pass numerous unchecked commands to processutils.ssh_execute which in turn calls paramiko's exec_command which just interprets shell commands.

nova/virt/powervm/common.py
ssh_command_as_root() also pushes various unchecked commands to a paramiko ssh channel.

plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost:
uses a _run_command() utility that calls subprocess.Popen with shell=True.

We need to further look into those commands to see if any of those parameters end up being attacker-controllable. Other cases mentioned in the report in Nova look invalid (shell=False and no SSH connection).

[Thierry Carrez (ttx)]

On Cinder side, the fixhy part is:

cinder/cinder/volume/drivers/storwize_svc.py
makes various calls to cinder.utils.ssh_execute() (via SanISCSIDriver._run_ssh()) which runs commands through a paramiko ssh channel.

Not in the report, but worth checking:

cinder/volume/drivers/san/hp/hp_3par_common.py
Calls to _cli_run() which pushes commands for SSH execution

[Thierry Carrez (ttx)]

With more investigation, nothing strikes me as exploitable, although I'll add a few more area devs to make sure. If we can't find an exploitable instance, this shall be opened as a strengthening bug and improved in the open.

[Thierry Carrez (ttx)]

Adding Lance Bragstad for nova/virt/powervm/blockdev.py and nova/virt/powervm/common.py
Adding John Garbutt for plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost
Adding John Griffith for cinder/volume/drivers/storwize_svc.py and cinder/volume/drivers/san/hp/hp_3par_common.py

Guys: this is an embargoed security bug, please do not post anything outside of this bug.

See the rest of the bug for context. Please doublecheck that the variables that are inserted in those commands sent through SSH (or called with shell=True) are not user-controlled in any way. If none of them are exploitable, we'll open this bug and improve those calls in the open. If one of them is exploitable we'll assign a CVE and fix it on this private bug first. Thanks for your help!

[John Garbutt (johngarbutt)]

The issue in (plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost) is being fixed here:
https://bugs.launchpad.net/nova/+bug/1074087
https://review.openstack.org/#/c/34580/

I agree with the original assessment, the xenapi bit is not exploitable.

[Lance Bragstad (lbragstad)]

I have checked out the the PowerVM driver and I think just to be safe, I can purpose a patch to sanitize the input we get before running the remote command.

We take several values from nova.conf (such as remote destination path for storing instances in the remote VIOS system; in which someone could name a file ';rm -rf /;'). Knowing this it doesn't hurt to sanitize all input but introducing a method that does it for everything. The following is a case where this happens:

https://github.com/openstack/nova/blob/master/nova/virt/powervm/blockdev.py#L261

I purpose we make a private method either existing in blockdev.py or common.py (common *might* be a better place for it as it makes it usable by more modules in the future, and kind of seems like a common utility) to check and escape the command parameter we are passed in.
https://github.com/openstack/nova/blob/master/nova/virt/powervm/blockdev.py#L543
https://github.com/openstack/nova/blob/master/nova/virt/powervm/blockdev.py#L560

Such that by the time we get to:

https://github.com/openstack/nova/blob/master/nova/virt/powervm/blockdev.py#L566

We can ensure the 'cmd' variable contains an escaped/safe string.

Thoughts?

[Thierry Carrez (ttx)]

@Lance: that code path should definitely be strengthened... but we first need to assess if it was directly exploitable, to see if we need to embargo the fix, assign a CVE and issue an advisory about it. I assume that if you can write to nova.conf you can probably do bad things to the remote VIOS system anyway (like you could just change the Nova code itself), so this is not a vulnerability.

Unless you tell me otherwise i'll assume none of those values are taken from non-root-user input, and so we can fix all those cases publicly.

[Lance Bragstad (lbragstad)]

@Thierry correct, those values would only be taken from a root user who would be writing them to the nova.conf file.

Regardless, I agree with the fact that we should strengthen the checking from values from nova.conf before firing off commands to the VIOS. I have a patch ready to go that builds and sanitizes those values. I don't think it would be necessary to implement the "command builder" in every place we send something off to the VIOS as this really only pertains to places where we read in values from nova.conf. I'll check back for when a public bug is opened per our discussion earlier, and push up the patch accordingly. Thanks all!

[Thierry Carrez (ttx)]

Err... we still need John Griffith to confirm the Cinder case.

[John Griffith (john-griffith)]

both the storwiz and the hp_3par_common ssh calls could be hardened.

[Thierry Carrez (ttx)]

@JohnGriffith: they could be hardened, but they are not exploitable as is ?

[John Griffith (john-griffith)]

@ThierryCarez: the only one that's questionable IMO is the access to the CHAP credentials on the StorWiz box, while I think it should be improved I don't think it's an exploit that should keep this embargoed.

[Thierry Carrez (ttx)]

Agreed, opening so that we can fix those in the open.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/37469
Committed: http://github.com/openstack/cinder/commit/6be79a8e3b4607adbbe6a26ee565156cd0fb36b0
Submitter: Jenkins
Branch: master

commit 6be79a8e3b4607adbbe6a26ee565156cd0fb36b0
Author: Haomai Wang <email address hidden>
Date: Wed Jul 17 21:36:55 2013 +0800

    Tidy up the SSH call to avoid injection attacks in storwize_svc

    Let the command and arguments form up a list and avoid the extra arguments
    attackers inserted to the command string

    fix bug 1192971

    Change-Id: I72bb7ef137223381c9daa613e61f1fde4c3bc8ae

[OpenStack Infra (hudson-openstack)]

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

[Kurt Seifried (kseifried)]

Does this need a CVE? sounds like it does.

[Thierry Carrez (ttx)]

@Kurt: none of those cases are exploitable, even if it pastes unchecked strings inside commands, the strings used are not under the control of a potential attacker (they are root-owned config values).

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/39485
Committed: http://github.com/openstack/cinder/commit/f752302d181583a95cf44354aea607ce9d9283f4
Submitter: Jenkins
Branch: master

commit f752302d181583a95cf44354aea607ce9d9283f4
Author: Haomai Wang <email address hidden>
Date: Wed Jul 17 21:36:55 2013 +0800

    Tidy up the SSH call to avoid injection attacks in storwize_svc

    Let the command and arguments form up a list and avoid the extra arguments
    attackers inserted to the command string

    fix bug 1192971

    Change-Id: I57b3fe60e64d9d0dc1ea9a18442c877be2ceece3

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/37697
Committed: http://github.com/openstack/cinder/commit/c55589b131828f3a595903f6796cb2d0babb772f
Submitter: Jenkins
Branch: master

commit c55589b131828f3a595903f6796cb2d0babb772f
Author: Haomai Wang <email address hidden>
Date: Thu Jul 18 23:05:43 2013 +0800

    Tidy up the SSH call to avoid injection attacks for HP's driver

    Let the command and arguments form up a list and avoid the extra arguments
    attackers inserted to the command string.

    And modify the interface of _cli_run, there is no need for a extra argument.

    fix bug 1192971
    Change-Id: Iff6a3ecb64feccae1b29164117576cab9943200a

[Sean Dague (sdague)]

It looks like all the nova cases are actually handled here

[Sean McGinnis (sean-mcginnis)]

Automatically unassigning due to inactivity.