Mask passwords in exceptions and error messages

When a ProcessExecutionError is thrown by processutils.ssh_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.

An earlier commit (e8199714) failed to address ssh_execute(). This
change set addresses ssh_execute.

OSSA is aware of this change request.

Change-Id: I0db9e98cbeb2a5e6f9ae0074f24717aa91cfc238
Closes-Bug: #1343604

oslo/concurrency/processutils.py

@@ -242,7 +242,8 @@ 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)
+    sanitized_cmd = strutils.mask_password(cmd)
+    LOG.debug('Running cmd (SSH): %s', sanitized_cmd)
     if addl_env:
         raise InvalidArgumentError(_('Environment not supported over SSH'))
 
@@ -256,7 +257,10 @@ def ssh_execute(ssh, cmd, process_input=None,
     # NOTE(justinsb): This seems suspicious...
     # ...other SSH clients have buffering issues with this approach
     stdout = stdout_stream.read()
+    sanitized_stdout = strutils.mask_password(stdout)
     stderr = stderr_stream.read()
+    sanitized_stderr = strutils.mask_password(stderr)
+
     stdin_stream.close()
 
     exit_status = channel.recv_exit_status()
@@ -266,11 +270,11 @@ def ssh_execute(ssh, cmd, process_input=None,
         LOG.debug('Result was %s' % exit_status)
         if check_exit_code and exit_status != 0:
             raise ProcessExecutionError(exit_code=exit_status,
-                                        stdout=stdout,
-                                        stderr=stderr,
-                                        cmd=cmd)
+                                        stdout=sanitized_stdout,
+                                        stderr=sanitized_stderr,
+                                        cmd=sanitized_cmd)
 
-    return (stdout, stderr)
+    return (sanitized_stdout, sanitized_stderr)
 
 
 def get_worker_count():

tests/unit/test_processutils.py

@@ -16,6 +16,7 @@
 from __future__ import print_function
 
 import errno
+import logging
 import multiprocessing
 import os
 import tempfile
@@ -332,3 +333,57 @@ class SshExecuteTestCase(test_base.BaseTestCase):
     def test_fails(self):
         self.assertRaises(processutils.ProcessExecutionError,
                           processutils.ssh_execute, FakeSshConnection(1), 'ls')
+
+    def _test_compromising_ssh(self, rc, check):
+        fixture = self.useFixture(fixtures.FakeLogger(level=logging.DEBUG))
+        fake_stdin = six.StringIO()
+
+        fake_stdout = mock.Mock()
+        fake_stdout.channel.recv_exit_status.return_value = rc
+        fake_stdout.read.return_value = 'password="secret"'
+
+        fake_stderr = six.StringIO('password="foobar"')
+
+        command = 'ls --password="bar"'
+
+        connection = mock.Mock()
+        connection.exec_command.return_value = (fake_stdin, fake_stdout,
+                                                fake_stderr)
+
+        if check and rc != -1 and rc != 0:
+            err = self.assertRaises(processutils.ProcessExecutionError,
+                                    processutils.ssh_execute,
+                                    connection, command,
+                                    check_exit_code=check)
+
+            self.assertEqual(rc, err.exit_code)
+            self.assertEqual(err.stdout, 'password="***"')
+            self.assertEqual(err.stderr, 'password="***"')
+            self.assertEqual(err.cmd, 'ls --password="***"')
+            self.assertNotIn('secret', str(err))
+            self.assertNotIn('foobar', str(err))
+        else:
+            o, e = processutils.ssh_execute(connection, command,
+                                            check_exit_code=check)
+            self.assertEqual('password="***"', o)
+            self.assertEqual('password="***"', e)
+            self.assertIn('password="***"', fixture.output)
+            self.assertNotIn('bar', fixture.output)
+
+    def test_compromising_ssh1(self):
+        self._test_compromising_ssh(rc=-1, check=True)
+
+    def test_compromising_ssh2(self):
+        self._test_compromising_ssh(rc=0, check=True)
+
+    def test_compromising_ssh3(self):
+        self._test_compromising_ssh(rc=1, check=True)
+
+    def test_compromising_ssh4(self):
+        self._test_compromising_ssh(rc=1, check=False)
+
+    def test_compromising_ssh5(self):
+        self._test_compromising_ssh(rc=0, check=False)
+
+    def test_compromising_ssh6(self):
+        self._test_compromising_ssh(rc=-1, check=False)