Command execution cases need to be strengthened

Bug #1192971 reported by Thierry Carrez
16
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
Wishlist
Unassigned
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned

Bug Description

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)

Tags: security
Revision history for this message
Thierry Carrez (ttx) wrote :
Changed in ossa:
status: New → Incomplete
Revision history for this message
Thierry Carrez (ttx) wrote :

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).

Revision history for this message
Thierry Carrez (ttx) wrote :

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

Revision history for this message
Thierry Carrez (ttx) wrote :

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.

Revision history for this message
Thierry Carrez (ttx) wrote :

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!

Revision history for this message
John Garbutt (johngarbutt) wrote :

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.

Revision history for this message
Lance Bragstad (lbragstad) wrote :

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?

Revision history for this message
Thierry Carrez (ttx) wrote :

@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.

Revision history for this message
Lance Bragstad (lbragstad) wrote :

@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!

Revision history for this message
Thierry Carrez (ttx) wrote :

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

summary: - Potentially unsafe command execution cases
+ Command execution cases need to be strengthened
Changed in cinder:
importance: Undecided → Wishlist
Changed in nova:
importance: Undecided → Wishlist
no longer affects: ossa
Changed in nova:
status: New → Confirmed
Changed in ossa:
status: New → Incomplete
Changed in cinder:
importance: Wishlist → Undecided
Revision history for this message
John Griffith (john-griffith) wrote :

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

Changed in cinder:
status: New → Confirmed
importance: Undecided → High
importance: High → Wishlist
Revision history for this message
Thierry Carrez (ttx) wrote :

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

Revision history for this message
John Griffith (john-griffith) wrote :

@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.

Revision history for this message
Thierry Carrez (ttx) wrote :

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

information type: Private Security → Public
Changed in ossa:
status: Incomplete → Won't Fix
tags: added: security
Changed in nova:
assignee: nobody → Lance Bragstad (ldbragst)
status: Confirmed → In Progress
Haomai Wang (haomai)
Changed in cinder:
assignee: nobody → Haomai Wang (haomai)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

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

Changed in cinder:
status: In Progress → Fix Committed
Changed in cinder:
status: Fix Committed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

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

Revision history for this message
Kurt Seifried (kseifried) wrote :

Does this need a CVE? sounds like it does.

Revision history for this message
Thierry Carrez (ttx) wrote :

@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).

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Joe Gordon (jogo)
Changed in nova:
status: In Progress → Confirmed
assignee: Lance Bragstad (lbragstad) → nobody
Revision history for this message
Sean Dague (sdague) wrote :

It looks like all the nova cases are actually handled here

no longer affects: nova
Revision history for this message
Sean McGinnis (sean-mcginnis) wrote :

Automatically unassigning due to inactivity.

Changed in cinder:
assignee: Haomai Wang (haomai) → nobody
status: In Progress → Triaged
Changed in cinder:
status: Triaged → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.