Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
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 (I173dfb865e84eb7dee54a22c76db1e4f125a0a8a) failed
to address ssh_execute(). This change set addresses ssh_execute.

Submitted to oslo.concurrency in
I0db9e98cbeb2a5e6f9ae0074f24717aa91cfc238

OSSA is aware of this change request.

Change-Id: Ie0caf32469126dd9feb44867adf27acb6e383958
Closes-Bug: #1377981
(cherry-picked from 6a60f84)
  • Loading branch information
Amrith Kumar authored and Tristan Cacqueray committed Oct 6, 2014
1 parent 9f019e0 commit 3cf7961
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 5 deletions.
14 changes: 9 additions & 5 deletions openstack/common/processutils.py
Expand Up @@ -237,7 +237,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'))

Expand All @@ -251,7 +252,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()
Expand All @@ -261,8 +265,8 @@ 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)
56 changes: 56 additions & 0 deletions tests/unit/test_processutils.py
Expand Up @@ -16,6 +16,8 @@
from __future__ import print_function

import errno
import logging
import mock
import os
import stat
import tempfile
Expand Down Expand Up @@ -313,3 +315,57 @@ def test_works(self):
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)

0 comments on commit 3cf7961

Please sign in to comment.