From b34d141eede8731e63cbb0312efce31559c44022 Mon Sep 17 00:00:00 2001
From: Matt Martz <matt@sivel.net>
Date: Wed, 13 Feb 2019 10:38:28 -0600
Subject: [PATCH] Disallow use of remote home directories containing .. in
 their path (CVE-2019-3828) (#52133)

* Disallow use of remote home directories containing .. in their path

* Add CVE to changelog
---
 .../fragments/disallow-relative-homedir.yaml  |  3 +
 lib/ansible/plugins/action/__init__.py        |  3 +
 test/units/plugins/action/test_action.py      | 61 ++++++++++++-------
 3 files changed, 44 insertions(+), 23 deletions(-)
 create mode 100644 changelogs/fragments/disallow-relative-homedir.yaml

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 = "<<INCLUDE_ANSIBLE_MODULE_JSON_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"]}