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

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

* Add CVE to changelog
(cherry picked from commit b34d141)

Co-authored-by: Matt Martz <matt@sivel.net>
(cherry picked from commit 396a2f74717477d80600450e2b7e45349d7b5110)
---
 .../fragments/disallow-relative-homedir.yaml  |  3 +
 lib/ansible/plugins/action/__init__.py        | 13 ++--
 test/units/plugins/action/test_action.py      | 61 ++++++++++++-------
 3 files changed, 50 insertions(+), 27 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 0000000000..0ae36ef94d
--- /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 79dbf5a25f..9d412d7f64 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -554,12 +554,17 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         if not initial_fragment:
             # Something went wrong trying to expand the path remotely.  Return
             # the original string
-            return path
+            expanded = path

-        if len(split_path) > 1:
-            return self._connection._shell.join_path(initial_fragment, *split_path[1:])
+        elif len(split_path) > 1:
+            expanded = self._connection._shell.join_path(initial_fragment, *split_path[1:])
         else:
-            return initial_fragment
+            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 2d542d1e2d..57dbef1815 100644
--- a/test/units/plugins/action/test_action.py
+++ b/test/units/plugins/action/test_action.py
@@ -54,6 +54,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

@@ -508,6 +530,18 @@ class TestActionBase(unittest.TestCase):
         finally:
             C.BECOME_ALLOW_SAME_USER = become_allow_same_user

+    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):
@@ -559,27 +593,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'
@@ -593,7 +608,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 = ''
@@ -607,7 +622,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 = ''
@@ -623,7 +638,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"]}
--
2.24.3 (Apple Git-128)
