Exceptions thrown, and messages logged by execute() may include passwords (CVE-2014-7230)

Thanks for your bug report. It does look like this could lead to information leakage in the exception handler cases where attempts > 0 and possibly when the exception is propagated up (when attempts == 0).

I'm marking the OSSA bug task as incomplete until discussed with other VMT members as to whether we will issue an advisory for this problem.

Grant, with respect, I disagree with your assessment. The default values are attempts = 1 and ignore_exit_code is False and check_exit_code is None. Therefore, by default, an invocation of execute that results in an error would result in an exception being thrown.

Whether you need to issue a security advisory or not may depend more on the incompleteness of the conditions in mask_password (strutils.mask_password) than just whether this function results in a thrown exception or not.

In testing my changes, I found that the masks in mask_password did not, for example, catch the usage

/usr/sbin/mysqld --password=top-secret

2014-07-19 18:35:01.415 20588 ERROR openstack.common.processutils [-] Running cmd (subprocess): /usr/sbin/mysqld --password=secret

They did catch

/usr/sbin/mysqld --password="top-secret"

2014-07-19 18:35:48.686 20605 ERROR openstack.common.processutils [-] Running cmd (subprocess): /usr/sbin/mysqld --password="***"

I do intend to enter a bug to make the strings in strutils.mask_password more robust.

Thanks for the additional information Amrith.

I guess all I was trying to point out was that the command is definitely being logged here without using mask_password here:

https://github.com/openstack/oslo-incubator/blob/master/openstack/common/processutils.py#L204

I'm going to mark the OSSA task as confirmed. Especially if the mask password implementation does not catch all use cases.

Grant, thanks for the clarification. I've also entered https://bugs.launchpad.net/ossa/+bug/1345233 and will work on the mask_passwords() thing as well.

Issue is public now - https://review.openstack.org/#/c/108215

Using this command: "find */* -iname "*.py" | xargs grep -r 'self.cmd *= *cmd'" reveals that in:

[stable/havana]:
cinder, glance, heat, keystone, neutron and nova are impacted

[stable/icehouse]:
cinder, glance, heat, keystone, neutron, nova and trove are impacted

[master]:
cinder, glance, heat, neutron, nova and trove are impacted

While "find */* -iname "*.py" | xargs grep -r 'mask_password'" reveals that only keystone is using it across all versions

We should be looking more precisely at the affected list and check which projects are using execute() with command that have password passed on the command line.
For each identified affected project we'll need a target series and a patch for each affected branch (we are looking at 12 (stable) + 6 (master) + 3 (oslo) = 21 potential reviews!)

In the meantime, this is impact description draft #1:

Title: Potential password leak when shell command fail
Reporter: Amrith Kumar (Tesora)
Products: Cinder (2013.2 versions up to 2013.2.3,
                    2014.1 versions up to 2014.1.1)
          Glance (2013.2 versions up to 2013.2.3,
                    2014.1 versions up to 2014.1.1)
          Heat (2013.2 versions up to 2013.2.3,
                    2014.1 versions up to 2014.1.1)
          Keystone (2013.2 versions up to 2013.2.3)
          Neutron (2013.2 versions up to 2013.2.3,
                    2014.1 versions up to 2014.1.1)
          Nova (2013.2 versions up to 2013.2.3,
                    2014.1 versions up to 2014.1.1)
          Trove (2014.1 versions up to 2014.1.1)

Description:
Amrith Kumar from Tesora reported a vulnerability in the processutils execute function available from oslo-incubator and which is copied into each project's code. An attacker with read access to the services' logs may obtain passwords used as a parameter of a command that fail. All services are impacted.

Hum, the mask_password is more broad than stated, here what I used to track affected version:
find */* -iname "*.py" | xargs grep -r 'mask_password(' | grep -v ':def' | grep -v '>>>'

Here is the summary

mask_password: [stable/havana]: keystone
execute: [stable/havana]: cinder, glance, heat, keystone, neutron and nova

mask_password: [stable/icehouse]: keystone, nova and trove
execute: [stable/icehouse]: cinder, glance, heat, keystone, neutron, nova and trove

mask_password: [master]: cinder, glance, heat, keystone, nova and trove
execute: [master]: cinder, glance, heat, neutron, nova and trove are impacted

I simplified the affected versions, it may now include unaffected version but it so much easier to read...
Here is the combined impact description draft #2:

Title: Potential password leak when shell command fail or because of incorrect masking
Reporter: Amrith Kumar (Tesora)
Products: Cinder, Glance, Heat, Keystone, Neutron, Nova, Trove
Versions: up to 2013.2.3, 2014.1 versions up to 2014.1.1

Description:
Amrith Kumar from Tesora reported two vulnerabilities in the processutils.execute() and mask_password() functions available from oslo-incubator that are copied into each project's code. An attacker with read access to the services' logs may obtain passwords used as a parameter of a command that fail or when the mask_password does not work properly. All services are impacted.

Change abandoned by amrith (<email address hidden>) on branch: master
Review: https://review.openstack.org/108215
Reason: I am abandoning https://review.openstack.org/#/c/108216/ and https://review.openstack.org/#/c/108215/ and will create a new commit with proper dependencies.

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

After looking more closely at the affected project usage, it's not as broad as it firsts sounded. Either service does not log the exception/mask_password, either vulnerable are used without any password/sensitive data on command line.

Yet here is a more concrete list of vulnerable code:

execute: [stable/havana]:
 * cinder (cinder/brick/iscsi/iscsi.py:414),
 * nova (no clear vulnerable usage, yet the exception is re-implemented in tools/esx/guest_tool.py and nova/virt/powervm/operator.py:213 is logging the exception)

mask_password: [stable/icehouse]:
 * trove (trove/extensions/mysql/service.py:85)
execute: [stable/icehouse]:
 * cinder (cinder/zonemanager/drivers/brocade/brcd_fc_zone_client_cli.py:331),
 * nova (unclear),
 * trove (unclear)

mask_password: [master]:
 * trove (trove/instance/service.py:181)
execute: [master]:
 * cinder (cinder/zonemanager/drivers/brocade/brcd_fc_zone_client_cli.py:336),
 * trove (trove/guestagent/strategies/restore/couchbase_impl.py:193)

@trove-coresec and @cinder-coresec: Can you please confirmed the impacted code ?

@nova-coresec: Can you please double check Nova codebase for those vulnerability ?

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

Change abandoned by amrith (<email address hidden>) on branch: master
Review: https://review.openstack.org/109417
Reason: Change has been submitted to oslo.concurrency as https://review.openstack.org/#/c/114656/

@slicknik Can you confirm trove/icehouse is not impacted? This may leak password if the request does not encapsulate password with '"''
  https://github.com/openstack/trove/blob/stable/icehouse/trove/extensions/mysql/service.py#L85
e.g., >>> mask_password("requests.... password=SECRET")
u'requests.... password=SECRET'

@cinder-coresec: Can you check how an execution error in this call will get handled ?
   https://github.com/openstack/cinder/blob/stable/havana/cinder/brick/iscsi/iscsi.py#L419
e.g., if the exception is logged it might leak the password to logs (either because password is not encapsulated with '"', either because the mask_password is not called)

@Tracy Jones: Can you explain why did you added "compute" tag ? I double check nova source code and couldn't find a clear code path that would leak a password by logging ProcessExecutionError exception.

Reviewed: https://review.openstack.org/114656
Committed: https://git.openstack.org/cgit/openstack/oslo.concurrency/commit/?id=c906dccefccedd8d00d6aa3eacc76194e8199714
Submitter: Jenkins
Branch: master

commit c906dccefccedd8d00d6aa3eacc76194e8199714
Author: Amrith Kumar <email address hidden>
Date: Thu Aug 14 00:52:02 2014 -0400

    Mask passwords in exceptions and error messages

    When a ProcessExecutionError is thrown by processutils.execute(), the
    exception may contain information such as password. Upstream
    applications that just log the message (as several appear to do) could
    inadvertently expose these passwords to a user with read access to the
    log files. It is therefore considered prudent to invoke
    strutils.mask_password() on the command, stdout and stderr in the
    exception. A test case has been added to ensure that all three are
    properly masked.

    OSSA is aware of this change request.

    Originally-Submitted-In: I173dfb865e84eb7dee54a22c76db1e4f125a0a8a

    Change-Id: Ie122db5f19802f519b96ed024ab3f2b5eede3eee
    Closes-Bug: #1343604

Reviewed: https://review.openstack.org/109417
Committed: https://git.openstack.org/cgit/openstack/oslo-incubator/commit/?id=63c99a0fd5fa7f60b33c7fa756020e5562b6afb0
Submitter: Jenkins
Branch: master

commit 63c99a0fd5fa7f60b33c7fa756020e5562b6afb0
Author: Amrith Kumar <email address hidden>
Date: Thu Jul 24 17:04:42 2014 -0400

    Mask passwords in exceptions and error messages

    When a ProcessExecutionError is thrown by processutils.execute(), the
    exception may contain information such as password. Upstream
    applications that just log the message (as several appear to do) could
    inadvertently expose these passwords to a user with read access to the
    log files. It is therefore considered prudent to invoke
    strutils.mask_password() on the command, stdout and stderr in the
    exception. A test case has been added to ensure that all three are
    properly masked.

    OSSA is aware of this change request.

    Submitted to oslo.concurrency in
    Ie122db5f19802f519b96ed024ab3f2b5eede3eee

    Change-Id: I173dfb865e84eb7dee54a22c76db1e4f125a0a8a
    Closes-Bug: #1343604

I'm not sure, isn't the exception info in the logs restricted access anyway here? We don't typically emit exception info to end user, however if there are places in the client this happens (it shouldn't it should be generalized REST faults) that should certainly be fixed.

I do agree that it might be worthwhile to scrub these, in the logs even for those submitting support requests, but I'm not sure i see this as a sec issue currently.

@John Griffith: Well password leak in system logs is considered as a vulnerability because they may end up being shared with third parties.
Though, about the code in iscsi.py#L419, it's not clear if this exception will get logged and if the iscsi admin password will get leaked or not.

Anyway, I think cinder should triage this one for stable/havana and stable/icehouse in order to update the openstack common code of processutils.

@amrith Looking at oslo-incubator, didn't you missed to patch ssh_execute too ?
 https://github.com/openstack/oslo-incubator/blob/master/openstack/common/processutils.py#L271

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

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

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

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

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: master
Review: https://review.openstack.org/116719
Reason: Taken over by https://review.openstack.org/116927

Reviewed: https://review.openstack.org/116927
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=ff0f112bb41fa0f01692f6c4eb8a20d9a4fa8a7c
Submitter: Jenkins
Branch: master

commit ff0f112bb41fa0f01692f6c4eb8a20d9a4fa8a7c
Author: Jay S. Bryant <email address hidden>
Date: Tue Aug 26 10:32:52 2014 -0500

    Sync latest processutils from oslo-incubator

    This sync is primarily being done to pull in commit
    63c99a0f so that passwords are masked in exception and
    error messages.

    ------------------------------------------------

    oslo-incubator head:

    commit 80a08a413fb0f23a056eca2d273b167f0a09bab6
    Merge: 83c4098 d73f3b1
    Author: Jenkins <email address hidden>
    Date: Mon Aug 25 14:32:36 2014 +0000

        Merge "Remove unused/mutable default args"

    -----------------------------------------------

    The sync pulls in the following changes (newest to oldest):

    63c99a0f - Mask passwords in exceptions and error messages
    e184dd36 - Fix exception message in openstack.common.processutils.execute
    d6b55fb2 - Remove `processutils` dependency on `log`

    -----------------------------------------------

    Change-Id: Ia92aab76fa83d01c5fbf6f9d31df2463fc26ba5c
    Partial-bug: 1343604

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

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

commit 853d8f9897f8563851441108a9be26b10908c076
Author: Tristan Cacqueray <email address hidden>
Date: Tue Aug 26 18:16:40 2014 +0000

    Mask passwords in exceptions and error messages

    When a ProcessExecutionError is thrown by processutils.execute(), the
    exception may contain information such as password. Upstream
    applications that just log the message (as several appear to do) could
    inadvertently expose these passwords to a user with read access to the
    log files. It is therefore considered prudent to invoke
    strutils.mask_password() on the command, stdout and stderr in the
    exception.

    Cherry-pick from review.openstack.org/109417
    Partial-Bug: #1343604

    Change-Id: I9741dcdebdb4bed295ddc5ec4c4b117fffcfe88c

Change abandoned by Davanum Srinivas (dims) (<email address hidden>) on branch: master
Review: https://review.openstack.org/119813

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/120207

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/120209

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/120219

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: stable/havana
Review: https://review.openstack.org/120209
Reason: Wrong cherry-pick

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/120223

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: stable/havana
Review: https://review.openstack.org/120207
Reason: Wrong cherry-pick

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/120229

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/120234

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/121095

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: stable/havana
Review: https://review.openstack.org/120234
Reason: Replaced by proper backport from oslo-incubator here: https://review.openstack.org/#/c/121095/

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: stable/havana
Review: https://review.openstack.org/120223
Reason: Replaced by proper backport from oslo-incubator here: https://review.openstack.org/#/c/121095/

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/121096

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/121382

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/121383

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: stable/icehouse
Review: https://review.openstack.org/120219
Reason: Replaced by proper backport from oslo-incubator here: https://review.openstack.org/121382

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: stable/icehouse
Review: https://review.openstack.org/120229
Reason: Replaced by proper backport from oslo-incubator here: https://review.openstack.org/121383

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/121416

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

Proposed fixes can be tracked here:
  https://review.openstack.org/#/q/I3b49b1d667f6ade9ae3f6765d735440a3e838917,n,z

So far, only Cinder, Nova and Trove have been confirmed. Here is the updated impact description:

Title: Potential password leak to log when shell command fail or because of incorrect password masking
Reporter: Amrith Kumar (Tesora)
Products: Cinder, Nova, Trove
Versions: up to 2013.2.4, 2014.1 versions up to 2014.1.1

Description:
Amrith Kumar from Tesora reported two vulnerabilities in the processutils.execute() and strutils.mask_password() functions available from oslo-incubator that are copied into each project's code. An attacker with read access to the services' logs may obtain passwords used as a parameter of a command that have failed or when the mask_password did not mask password properly. All services are impacted.

Reviewed: https://review.openstack.org/121095
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=6772d5ffb2ca3fbfb0accebaed0cea18b80a03e0
Submitter: Jenkins
Branch: stable/havana

commit 6772d5ffb2ca3fbfb0accebaed0cea18b80a03e0
Author: Tristan Cacqueray <email address hidden>
Date: Sat Sep 13 15:51:44 2014 +0000

    Sync latest process and str utils from oslo

    This backport the necessary changes to fix both issues:
    * Make execute method clean password in exception
    * Make sure mask_password works properly

    Backport in oslo-incubator: 4cf1a2a158f7c3c21799bf2e6ba0e7b5fbc25d62

    Closes-Bug: 1343604
    Closes-Bug: 1345233
    SecurityImpact

    Change-Id: I3b49b1d667f6ade9ae3f6765d735440a3e838917

Reviewed: https://review.openstack.org/121096
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=34611b3e369f4b78c6575281858d60907ceca92d
Submitter: Jenkins
Branch: stable/havana

commit 34611b3e369f4b78c6575281858d60907ceca92d
Author: Tristan Cacqueray <email address hidden>
Date: Fri Sep 12 12:58:24 2014 +0000

    Sync latest process and str utils from oslo

    This backport the necessary changes to fix both issues:
    * Make execute method clean password in exception
    * Make sure mask_password works properly

    Backport in oslo-incubator: 4cf1a2a158f7c3c21799bf2e6ba0e7b5fbc25d62

    Closes-Bug: 1343604
    Closes-Bug: 1345233
    SecurityImpact

    Change-Id: I3b49b1d667f6ade9ae3f6765d735440a3e838917

On Impact desc:
title is way too long. maybe "Potential leak of passwords into log files"
"All services are impacted" seem to go against "Cinder, Nova, Trove". Maybe remove the mention altogether ?

@ttx Thanks for the review!
Note about the versions: affected products have been fixed in time for 2013.2.4 (except Trove that is not part of Havana).
Here is the updated impact description.

Title: Potential leak of passwords into log files
Reporter: Amrith Kumar (Tesora)
Products: Cinder, Nova, Trove
Versions: up to 2013.2.3, 2014.1 versions up to 2014.1.2

Description:
Amrith Kumar from Tesora reported two vulnerabilities in the processutils.execute() and strutils.mask_password() functions available from oslo-incubator that are copied into each project's code. An attacker with read access to the services' logs may obtain passwords used as a parameter of a command that have failed or when the mask_password did not mask passwords properly.

Reviewed: https://review.openstack.org/121382
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=ec249ee8c68e8d22a9e52cdc8bf062e7b0f7fd79
Submitter: Jenkins
Branch: stable/icehouse

commit ec249ee8c68e8d22a9e52cdc8bf062e7b0f7fd79
Author: Tristan Cacqueray <email address hidden>
Date: Sat Sep 13 18:12:05 2014 +0000

    Sync latest process and str utils from oslo

    This backport the necessary changes to fix both issues:
    * Make execute method clean password in exception
    * Make sure mask_password works properly

    Backport in oslo-incubator: https://review.openstack.org/121365

    Closes-Bug: 1343604
    Closes-Bug: 1345233
    SecurityImpact

    Change-Id: I3b49b1d667f6ade9ae3f6765d735440a3e838917

Reviewed: https://review.openstack.org/121416
Committed: https://git.openstack.org/cgit/openstack/trove/commit/?id=a36c2a7732cc33623b95989d6c27812014a9a727
Submitter: Jenkins
Branch: stable/icehouse

commit a36c2a7732cc33623b95989d6c27812014a9a727
Author: Tristan Cacqueray <email address hidden>
Date: Sun Sep 14 18:17:40 2014 +0000

    Sync latest process and str utils from oslo

    This sync required changes to fix these issues:
    * Make execute method clean password in exception
    * Make sure mask_password works properly

    This is not trivial as these fixes relies on many other changes,
    only the necessary code have been imported/adapted.

    ------------------------------------------------
    The sync pulls in the following changes (newest to oldest):

    63c99a0f - Mask passwords in exceptions and error messages
    66142c34 - Make strutils.mask_password more secure
    d6b55fb2 - Remove `processutils` dependency on `log`
    cb5a804b - Move `mask_password` to strutils

    -----------------------------------------------

    Closes-Bug: 1343604
    Closes-Bug: 1345233
    SecurityImpact

    Change-Id: I3b49b1d667f6ade9ae3f6765d735440a3e838917

Reviewed: https://review.openstack.org/121383
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=f58d95c964cb9a42f573596d1cc80d2034ddb23e
Submitter: Jenkins
Branch: stable/icehouse

commit f58d95c964cb9a42f573596d1cc80d2034ddb23e
Author: Tristan Cacqueray <email address hidden>
Date: Sat Sep 13 18:54:33 2014 +0000

    Sync process and str utils from oslo

    This patch backports the necessary changes to fix both issues:
    * Make execute method clean password in exception
    * Make sure mask_password works properly

    ------------------------------------------------
    The sync pulls in the following changes (newest to oldest):

    63c99a0f - Mask passwords in exceptions and error messages
    66142c34 - Make strutils.mask_password more secure
    d6b55fb2 - Remove `processutils` dependency on `log`
    cb5a804b - Move `mask_password` to strutils

    -----------------------------------------------

    Backport in oslo-incubator: https://review.openstack.org/121365

    Closes-Bug: 1343604
    Closes-Bug: 1345233
    SecurityImpact

    Change-Id: I3b49b1d667f6ade9ae3f6765d735440a3e838917

Reviewed: https://review.openstack.org/121417
Committed: https://git.openstack.org/cgit/openstack/trove/commit/?id=9672744f090d462cac5eb757ceaacd7122362708
Submitter: Jenkins
Branch: master

commit 9672744f090d462cac5eb757ceaacd7122362708
Author: Tristan Cacqueray <email address hidden>
Date: Sun Sep 14 19:18:06 2014 +0000

    Sync latest process and str utils from oslo

    This sync required changes to fix these issues:
    * Make execute method clean password in exception
    * Make sure mask_password works properly

    ------------------------------------------------
    The sync pulls in the following changes (newest to oldest):

    6a60f842 - Mask passwords in exceptions and error messages (SSH)
    63c99a0f - Mask passwords in exceptions and error messages
    66142c34 - Make strutils.mask_password more secure

    -----------------------------------------------

    Closes-Bug: 1343604
    Closes-Bug: 1345233
    SecurityImpact

    Change-Id: I3b49b1d667f6ade9ae3f6765d735440a3e838917

The ssh_execute is still vulnerable to #1343604 and we need to know if the last havana and icehouse releases are really affected...

@nova-coresec: @cinder-coresec: can someone please have a look at these location:

https://etherpad.openstack.org/p/1343604-nova-ssh_execute
https://etherpad.openstack.org/p/1343604-cinder-ssh_execute

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

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

Follow-up on bug 1377981