Fix password leak from ProcessExecution errorshavana-eolstable/havana

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

This backport 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

4 files changed, 405 insertions, 11 deletions

diff --git a/openstack/common/processutils.py b/openstack/common/processutils.py
index 9364559f..a936176a 100644
--- a/openstack/common/processutils.py
+++ b/openstack/common/processutils.py
@@ -19,7 +19,7 @@
 System-level utilities and helper functions.
 """
 
-import logging as stdlib_logging
+import logging
 import os
 import random
 import shlex
@@ -29,7 +29,7 @@ from eventlet.green import subprocess
 from eventlet import greenthread
 
 from openstack.common.gettextutils import _  # noqa
-from openstack.common import log as logging
+from openstack.common import strutils
 
 
 LOG = logging.getLogger(__name__)
@@ -104,8 +104,7 @@ def execute(*cmd, **kwargs):
                             execute this command. Defaults to false.
     :type shell:            boolean
     :param loglevel:        log level for execute commands.
-    :type loglevel:         int.  (Should be stdlib_logging.DEBUG or
-                            stdlib_logging.INFO)
+    :type loglevel:         int.  (Should be logging.DEBUG or logging.INFO)
     :returns:               (stdout, stderr) from process execution
     :raises:                :class:`UnknownArgumentError` on
                             receiving unknown arguments
@@ -120,7 +119,7 @@ def execute(*cmd, **kwargs):
     run_as_root = kwargs.pop('run_as_root', False)
     root_helper = kwargs.pop('root_helper', '')
     shell = kwargs.pop('shell', False)
-    loglevel = kwargs.pop('loglevel', stdlib_logging.DEBUG)
+    loglevel = kwargs.pop('loglevel', logging.DEBUG)
 
     if isinstance(check_exit_code, bool):
         ignore_exit_code = not check_exit_code
@@ -140,11 +139,12 @@ def execute(*cmd, **kwargs):
         cmd = shlex.split(root_helper) + list(cmd)
 
     cmd = map(str, cmd)
+    sanitized_cmd = strutils.mask_password(' '.join(cmd))
 
     while attempts > 0:
         attempts -= 1
         try:
-            LOG.log(loglevel, _('Running cmd (subprocess): %s'), ' '.join(cmd))
+            LOG.log(loglevel, _('Running cmd (subprocess): %s'), sanitized_cmd)
             _PIPE = subprocess.PIPE  # pylint: disable=E1101
 
             if os.name == 'nt':
@@ -172,16 +172,18 @@ def execute(*cmd, **kwargs):
                 LOG.log(loglevel, _('Result was %s') % _returncode)
                 if not ignore_exit_code and _returncode not in check_exit_code:
                     (stdout, stderr) = result
+                    sanitized_stdout = strutils.mask_password(stdout)
+                    sanitized_stderr = strutils.mask_password(stderr)
                     raise ProcessExecutionError(exit_code=_returncode,
-                                                stdout=stdout,
-                                                stderr=stderr,
-                                                cmd=' '.join(cmd))
+                                                stdout=sanitized_stdout,
+                                                stderr=sanitized_stderr,
+                                                cmd=sanitized_cmd)
             return result
         except ProcessExecutionError:
             if not attempts:
                 raise
             else:
-                LOG.log(loglevel, _('%r failed. Retrying.'), cmd)
+                LOG.log(loglevel, _('%r failed. Retrying.'), sanitized_cmd)
                 if delay_on_retry:
                     greenthread.sleep(random.randint(20, 200) / 100.0)
         finally:
@@ -220,7 +222,7 @@ def trycmd(*args, **kwargs):
 
 def ssh_execute(ssh, cmd, process_input=None,
                 addl_env=None, check_exit_code=True):
-    LOG.debug(_('Running cmd (SSH): %s'), cmd)
+    LOG.debug('Running cmd (SSH): %s', cmd)
     if addl_env:
         raise InvalidArgumentError(_('Environment not supported over SSH'))
 
diff --git a/openstack/common/strutils.py b/openstack/common/strutils.py
index 971ffe3c..38b0f615 100644
--- a/openstack/common/strutils.py
+++ b/openstack/common/strutils.py
@@ -46,6 +46,39 @@ SLUGIFY_STRIP_RE = re.compile(r"[^\w\s-]")
 SLUGIFY_HYPHENATE_RE = re.compile(r"[-\s]+")
 
 
+# NOTE(flaper87): The following globals are used by `mask_password`
+_SANITIZE_KEYS = ['adminPass', 'admin_pass', 'password', 'admin_password']
+
+# NOTE(ldbragst): Let's build a list of regex objects using the list of
+# _SANITIZE_KEYS we already have. This way, we only have to add the new key
+# to the list of _SANITIZE_KEYS and we can generate regular expressions
+# for XML and JSON automatically.
+_SANITIZE_PATTERNS_2 = []
+_SANITIZE_PATTERNS_1 = []
+
+# NOTE(amrith): Some regular expressions have only one parameter, some
+# have two parameters. Use different lists of patterns here.
+_FORMAT_PATTERNS_1 = [r'(%(key)s\s*[=]\s*)[^\s^\'^\"]+']
+_FORMAT_PATTERNS_2 = [r'(%(key)s\s*[=]\s*[\"\']).*?([\"\'])',
+                      r'(%(key)s\s+[\"\']).*?([\"\'])',
+                      r'([-]{2}%(key)s\s+)[^\'^\"^=^\s]+([\s]*)',
+                      r'(<%(key)s>).*?(</%(key)s>)',
+                      r'([\"\']%(key)s[\"\']\s*:\s*[\"\']).*?([\"\'])',
+                      r'([\'"].*?%(key)s[\'"]\s*:\s*u?[\'"]).*?([\'"])',
+                      r'([\'"].*?%(key)s[\'"]\s*,\s*\'--?[A-z]+\'\s*,\s*u?'
+                      '[\'"]).*?([\'"])',
+                      r'(%(key)s\s*--?[A-z]+\s*)\S+(\s*)']
+
+for key in _SANITIZE_KEYS:
+    for pattern in _FORMAT_PATTERNS_2:
+        reg_ex = re.compile(pattern % {'key': key}, re.DOTALL)
+        _SANITIZE_PATTERNS_2.append(reg_ex)
+
+    for pattern in _FORMAT_PATTERNS_1:
+        reg_ex = re.compile(pattern % {'key': key}, re.DOTALL)
+        _SANITIZE_PATTERNS_1.append(reg_ex)
+
+
 def int_from_bool_as_string(subject):
     """Interpret a string as a boolean and return either 1 or 0.
 
@@ -216,3 +249,42 @@ def to_slug(value, incoming=None, errors="strict"):
         "ascii", "ignore").decode("ascii")
     value = SLUGIFY_STRIP_RE.sub("", value).strip().lower()
     return SLUGIFY_HYPHENATE_RE.sub("-", value)
+
+
+def mask_password(message, secret="***"):
+    """Replace password with 'secret' in message.
+
+    :param message: The string which includes security information.
+    :param secret: value with which to replace passwords.
+    :returns: The unicode value of message with the password fields masked.
+
+    For example:
+
+    >>> mask_password("'adminPass' : 'aaaaa'")
+    "'adminPass' : '***'"
+    >>> mask_password("'admin_pass' : 'aaaaa'")
+    "'admin_pass' : '***'"
+    >>> mask_password('"password" : "aaaaa"')
+    '"password" : "***"'
+    >>> mask_password("'original_password' : 'aaaaa'")
+    "'original_password' : '***'"
+    >>> mask_password("u'original_password' :   u'aaaaa'")
+    "u'original_password' :   u'***'"
+    """
+    message = six.text_type(message)
+
+    # NOTE(ldbragst): Check to see if anything in message contains any key
+    # specified in _SANITIZE_KEYS, if not then just return the message since
+    # we don't have to mask any passwords.
+    if not any(key in message for key in _SANITIZE_KEYS):
+        return message
+
+    substitute = r'\g<1>' + secret + r'\g<2>'
+    for pattern in _SANITIZE_PATTERNS_2:
+        message = re.sub(pattern, substitute, message)
+
+    substitute = r'\g<1>' + secret
+    for pattern in _SANITIZE_PATTERNS_1:
+        message = re.sub(pattern, substitute, message)
+
+    return message
diff --git a/tests/unit/test_processutils.py b/tests/unit/test_processutils.py
index 0f9289c5..0eeecf4d 100644
--- a/tests/unit/test_processutils.py
+++ b/tests/unit/test_processutils.py
@@ -19,6 +19,7 @@ from __future__ import print_function
 
 import fixtures
 import os
+import stat
 import tempfile
 
 import six
@@ -26,6 +27,14 @@ import six
 from openstack.common import processutils
 from openstack.common import test
 
+TEST_EXCEPTION_AND_MASKING_SCRIPT = """#!/bin/bash
+# This is to test stdout and stderr
+# and the command returned in an exception
+# when a non-zero exit code is returned
+echo onstdout --password='"secret"'
+echo onstderr --password='"secret"' 1>&2
+exit 38"""
+
 
 class UtilsTest(test.BaseTestCase):
     # NOTE(jkoelker) Moar tests from nova need to be ported. But they
@@ -217,6 +226,30 @@ class FakeSshStream(six.StringIO):
     def setup_channel(self, rc):
         self.channel = FakeSshChannel(rc)
 
+    def test_exception_and_masking(self):
+        tmpfilename = self.create_tempfiles(
+            [["test_exceptions_and_masking",
+              TEST_EXCEPTION_AND_MASKING_SCRIPT]], ext='bash')[0]
+
+        os.chmod(tmpfilename, (stat.S_IRWXU |
+                               stat.S_IRGRP |
+                               stat.S_IXGRP |
+                               stat.S_IROTH |
+                               stat.S_IXOTH))
+
+        err = self.assertRaises(processutils.ProcessExecutionError,
+                                processutils.execute,
+                                tmpfilename, 'password="secret"',
+                                'something')
+
+        self.assertEqual(38, err.exit_code)
+        self.assertEqual(err.stdout, 'onstdout --password="***"\n')
+        self.assertEqual(err.stderr, 'onstderr --password="***"\n')
+        self.assertEqual(err.cmd, ' '.join([tmpfilename,
+                                            'password="***"',
+                                            'something']))
+        self.assertNotIn('secret', str(err))
+
 
 class FakeSshConnection(object):
     def __init__(self, rc):
diff --git a/tests/unit/test_strutils.py b/tests/unit/test_strutils.py
index 22aafd9a..302f55b8 100644
--- a/tests/unit/test_strutils.py
+++ b/tests/unit/test_strutils.py
@@ -213,3 +213,290 @@ class StrUtilsTest(test.BaseTestCase):
         self.assertEqual(six.u("perche"), to_slug("perch\xc3\xa9"))
         self.assertEqual(six.u("strange"),
                          to_slug("\x80strange", errors="ignore"))
+
+
+class MaskPasswordTestCase(test.BaseTestCase):
+
+    def test_json(self):
+        # Test 'adminPass' w/o spaces
+        payload = """{'adminPass':'mypassword'}"""
+        expected = """{'adminPass':'***'}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'adminPass' with spaces
+        payload = """{ 'adminPass' : 'mypassword' }"""
+        expected = """{ 'adminPass' : '***' }"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' w/o spaces
+        payload = """{'admin_pass':'mypassword'}"""
+        expected = """{'admin_pass':'***'}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' with spaces
+        payload = """{ 'admin_pass' : 'mypassword' }"""
+        expected = """{ 'admin_pass' : '***' }"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' w/o spaces
+        payload = """{'admin_password':'mypassword'}"""
+        expected = """{'admin_password':'***'}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' with spaces
+        payload = """{ 'admin_password' : 'mypassword' }"""
+        expected = """{ 'admin_password' : '***' }"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' w/o spaces
+        payload = """{'password':'mypassword'}"""
+        expected = """{'password':'***'}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' with spaces
+        payload = """{ 'password' : 'mypassword' }"""
+        expected = """{ 'password' : '***' }"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+    def test_xml(self):
+        # Test 'adminPass' w/o spaces
+        payload = """<adminPass>mypassword</adminPass>"""
+        expected = """<adminPass>***</adminPass>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'adminPass' with spaces
+        payload = """<adminPass>
+                        mypassword
+                     </adminPass>"""
+        expected = """<adminPass>***</adminPass>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' w/o spaces
+        payload = """<admin_pass>mypassword</admin_pass>"""
+        expected = """<admin_pass>***</admin_pass>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' with spaces
+        payload = """<admin_pass>
+                        mypassword
+                     </admin_pass>"""
+        expected = """<admin_pass>***</admin_pass>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' w/o spaces
+        payload = """<admin_password>mypassword</admin_password>"""
+        expected = """<admin_password>***</admin_password>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' with spaces
+        payload = """<admin_password>
+                        mypassword
+                     </admin_password>"""
+        expected = """<admin_password>***</admin_password>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' w/o spaces
+        payload = """<password>mypassword</password>"""
+        expected = """<password>***</password>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' with spaces
+        payload = """<password>
+                        mypassword
+                     </password>"""
+        expected = """<password>***</password>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+    def test_xml_attribute(self):
+        # Test 'adminPass' w/o spaces
+        payload = """adminPass='mypassword'"""
+        expected = """adminPass='***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'adminPass' with spaces
+        payload = """adminPass = 'mypassword'"""
+        expected = """adminPass = '***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'adminPass' with double quotes
+        payload = """adminPass = "mypassword\""""
+        expected = """adminPass = "***\""""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' w/o spaces
+        payload = """admin_pass='mypassword'"""
+        expected = """admin_pass='***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' with spaces
+        payload = """admin_pass = 'mypassword'"""
+        expected = """admin_pass = '***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_pass' with double quotes
+        payload = """admin_pass = "mypassword\""""
+        expected = """admin_pass = "***\""""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' w/o spaces
+        payload = """admin_password='mypassword'"""
+        expected = """admin_password='***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' with spaces
+        payload = """admin_password = 'mypassword'"""
+        expected = """admin_password = '***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'admin_password' with double quotes
+        payload = """admin_password = "mypassword\""""
+        expected = """admin_password = "***\""""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' w/o spaces
+        payload = """password='mypassword'"""
+        expected = """password='***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' with spaces
+        payload = """password = 'mypassword'"""
+        expected = """password = '***'"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        # Test 'password' with double quotes
+        payload = """password = "mypassword\""""
+        expected = """password = "***\""""
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+    def test_json_message(self):
+        payload = """body: {"changePassword": {"adminPass": "1234567"}}"""
+        expected = """body: {"changePassword": {"adminPass": "***"}}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        payload = """body: {"rescue": {"admin_pass": "1234567"}}"""
+        expected = """body: {"rescue": {"admin_pass": "***"}}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        payload = """body: {"rescue": {"admin_password": "1234567"}}"""
+        expected = """body: {"rescue": {"admin_password": "***"}}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        payload = """body: {"rescue": {"password": "1234567"}}"""
+        expected = """body: {"rescue": {"password": "***"}}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+    def test_xml_message(self):
+        payload = """<?xml version="1.0" encoding="UTF-8"?>
+<rebuild
+    xmlns="http://docs.openstack.org/compute/api/v1.1"
+    name="foobar"
+    imageRef="http://openstack.example.com/v1.1/32278/images/70a599e0-31e7"
+    accessIPv4="1.2.3.4"
+    accessIPv6="fe80::100"
+    adminPass="seekr3t">
+  <metadata>
+    <meta key="My Server Name">Apache1</meta>
+  </metadata>
+</rebuild>"""
+        expected = """<?xml version="1.0" encoding="UTF-8"?>
+<rebuild
+    xmlns="http://docs.openstack.org/compute/api/v1.1"
+    name="foobar"
+    imageRef="http://openstack.example.com/v1.1/32278/images/70a599e0-31e7"
+    accessIPv4="1.2.3.4"
+    accessIPv6="fe80::100"
+    adminPass="***">
+  <metadata>
+    <meta key="My Server Name">Apache1</meta>
+  </metadata>
+</rebuild>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        payload = """<?xml version="1.0" encoding="UTF-8"?>
+<rescue xmlns="http://docs.openstack.org/compute/api/v1.1"
+    admin_pass="MySecretPass"/>"""
+        expected = """<?xml version="1.0" encoding="UTF-8"?>
+<rescue xmlns="http://docs.openstack.org/compute/api/v1.1"
+    admin_pass="***"/>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        payload = """<?xml version="1.0" encoding="UTF-8"?>
+<rescue xmlns="http://docs.openstack.org/compute/api/v1.1"
+    admin_password="MySecretPass"/>"""
+        expected = """<?xml version="1.0" encoding="UTF-8"?>
+<rescue xmlns="http://docs.openstack.org/compute/api/v1.1"
+    admin_password="***"/>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+        payload = """<?xml version="1.0" encoding="UTF-8"?>
+<rescue xmlns="http://docs.openstack.org/compute/api/v1.1"
+    password="MySecretPass"/>"""
+        expected = """<?xml version="1.0" encoding="UTF-8"?>
+<rescue xmlns="http://docs.openstack.org/compute/api/v1.1"
+    password="***"/>"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+    def test_mask_password(self):
+        payload = "test = 'password'  :   'aaaaaa'"
+        expected = "test = 'password'  :   '111'"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='111'))
+
+        payload = 'mysqld --password "aaaaaa"'
+        expected = 'mysqld --password "****"'
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='****'))
+
+        payload = 'mysqld --password aaaaaa'
+        expected = 'mysqld --password ???'
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='???'))
+
+        payload = 'mysqld --password = "aaaaaa"'
+        expected = 'mysqld --password = "****"'
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='****'))
+
+        payload = "mysqld --password = 'aaaaaa'"
+        expected = "mysqld --password = '****'"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='****'))
+
+        payload = "mysqld --password = aaaaaa"
+        expected = "mysqld --password = ****"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='****'))
+
+        payload = "test = password =   aaaaaa"
+        expected = "test = password =   111"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='111'))
+
+        payload = "test = password=   aaaaaa"
+        expected = "test = password=   111"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='111'))
+
+        payload = "test = password =aaaaaa"
+        expected = "test = password =111"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='111'))
+
+        payload = "test = password=aaaaaa"
+        expected = "test = password=111"
+        self.assertEqual(expected,
+                         strutils.mask_password(payload, secret='111'))
+
+        payload = 'test = "original_password" : "aaaaaaaaa"'
+        expected = 'test = "original_password" : "***"'
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = 'test = "param1" : "value"'
+        expected = 'test = "param1" : "value"'
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = """{'adminPass':'mypassword'}"""
+        payload = six.text_type(payload)
+        expected = """{'adminPass':'***'}"""
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = ("test = 'node.session.auth.password','-v','mypassword',"
+                   "'nomask'")
+        expected = ("test = 'node.session.auth.password','-v','***',"
+                    "'nomask'")
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = ("test = 'node.session.auth.password', '--password', "
+                   "'mypassword', 'nomask'")
+        expected = ("test = 'node.session.auth.password', '--password', "
+                    "'***', 'nomask'")
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = ("test = 'node.session.auth.password', '--password', "
+                   "'mypassword'")
+        expected = ("test = 'node.session.auth.password', '--password', "
+                    "'***'")
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = "test = node.session.auth.password -v mypassword nomask"
+        expected = "test = node.session.auth.password -v *** nomask"
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = ("test = node.session.auth.password --password mypassword "
+                   "nomask")
+        expected = ("test = node.session.auth.password --password *** "
+                    "nomask")
+        self.assertEqual(expected, strutils.mask_password(payload))
+
+        payload = ("test = node.session.auth.password --password mypassword")
+        expected = ("test = node.session.auth.password --password ***")
+        self.assertEqual(expected, strutils.mask_password(payload))