diff --git a/changelogs/fragments/disallow-relative-homedir.yaml b/changelogs/fragments/disallow-relative-homedir.yaml new file mode 100644 index 00000000000..0ae36ef94d2 --- /dev/null +++ b/changelogs/fragments/disallow-relative-homedir.yaml @@ -0,0 +1,3 @@ +bugfixes: +- remote home directory - Disallow use of remote home directories that include + relative pathing by means of `..` (CVE-2019-3828) (https://github.com/ansible/ansible/pull/52133) diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 0a9b2b78a95..5b28e1125a8 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -635,6 +635,9 @@ class ActionBase(with_metaclass(ABCMeta, object)): else: expanded = initial_fragment + if '..' in os.path.dirname(expanded).split('/'): + raise AnsibleError("'%s' returned an invalid relative home directory path containing '..'" % self._play_context.remote_addr) + return expanded def _strip_success_message(self, data): diff --git a/test/units/plugins/action/test_action.py b/test/units/plugins/action/test_action.py index 89d7377e8d0..79e4456a277 100644 --- a/test/units/plugins/action/test_action.py +++ b/test/units/plugins/action/test_action.py @@ -56,6 +56,28 @@ WINDOWS_ARGS = "<>" """ +def _action_base(): + fake_loader = DictDataLoader({ + }) + mock_module_loader = MagicMock() + mock_shared_loader_obj = MagicMock() + mock_shared_loader_obj.module_loader = mock_module_loader + mock_connection_loader = MagicMock() + + mock_shared_loader_obj.connection_loader = mock_connection_loader + mock_connection = MagicMock() + + play_context = MagicMock() + + action_base = DerivedActionBase(task=None, + connection=mock_connection, + play_context=play_context, + loader=fake_loader, + templar=None, + shared_loader_obj=mock_shared_loader_obj) + return action_base + + class DerivedActionBase(ActionBase): TRANSFERS_FILES = False @@ -526,6 +548,18 @@ class TestActionBase(unittest.TestCase): action_base._low_level_execute_command('ECHO SAME', sudoable=True) become.build_become_command.assert_called_once_with("ECHO SAME", shell) + def test__remote_expand_user_relative_pathing(self): + action_base = _action_base() + action_base._play_context.remote_addr = 'bar' + action_base._low_level_execute_command = MagicMock(return_value={'stdout': b'../home/user'}) + action_base._connection._shell.join_path.return_value = '../home/user/foo' + with self.assertRaises(AnsibleError) as cm: + action_base._remote_expand_user('~/foo') + self.assertEqual( + cm.exception.message, + "'bar' returned an invalid relative home directory path containing '..'" + ) + class TestActionBaseCleanReturnedData(unittest.TestCase): def test(self): @@ -577,27 +611,8 @@ class TestActionBaseCleanReturnedData(unittest.TestCase): class TestActionBaseParseReturnedData(unittest.TestCase): - def _action_base(self): - fake_loader = DictDataLoader({ - }) - mock_module_loader = MagicMock() - mock_shared_loader_obj = MagicMock() - mock_shared_loader_obj.module_loader = mock_module_loader - mock_connection_loader = MagicMock() - - mock_shared_loader_obj.connection_loader = mock_connection_loader - mock_connection = MagicMock() - - action_base = DerivedActionBase(task=None, - connection=mock_connection, - play_context=None, - loader=fake_loader, - templar=None, - shared_loader_obj=mock_shared_loader_obj) - return action_base - def test_fail_no_json(self): - action_base = self._action_base() + action_base = _action_base() rc = 0 stdout = 'foo\nbar\n' err = 'oopsy' @@ -611,7 +626,7 @@ class TestActionBaseParseReturnedData(unittest.TestCase): self.assertEqual(res['module_stderr'], err) def test_json_empty(self): - action_base = self._action_base() + action_base = _action_base() rc = 0 stdout = '{}\n' err = '' @@ -625,7 +640,7 @@ class TestActionBaseParseReturnedData(unittest.TestCase): self.assertFalse(res) def test_json_facts(self): - action_base = self._action_base() + action_base = _action_base() rc = 0 stdout = '{"ansible_facts": {"foo": "bar", "ansible_blip": "blip_value"}}\n' err = '' @@ -641,7 +656,7 @@ class TestActionBaseParseReturnedData(unittest.TestCase): # self.assertIsInstance(res['ansible_facts'], AnsibleUnsafe) def test_json_facts_add_host(self): - action_base = self._action_base() + action_base = _action_base() rc = 0 stdout = '''{"ansible_facts": {"foo": "bar", "ansible_blip": "blip_value"}, "add_host": {"host_vars": {"some_key": ["whatever the add_host object is"]}