prevent ansible_facts injection (#68431) (#68446)

* prevent ansible_facts injection (#68431)

- also only replace when needed
 - switched from replace to index
 - added test to verify bogus_facts are not accepted

CVE-2020-10684

(cherry picked from commit a9d2ceafe429171c0e2ad007058b88bae57c74ce)

* add to ignore

7 files changed, 36 insertions, 5 deletions

diff --git a/changelogs/fragments/af_clean.yml b/changelogs/fragments/af_clean.yml
new file mode 100644
index 0000000000..9d14ca5869
--- /dev/null
+++ b/changelogs/fragments/af_clean.yml
@@ -0,0 +1,2 @@
+bugfixes:
+    - Ensure we don't allow ansible_facts subkey of ansible_facts to override top level, also fix 'deprefixing' to prevent key transforms.
diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py
index 225361ddd7..fcce72d4f6 100644
--- a/lib/ansible/constants.py
+++ b/lib/ansible/constants.py
@@ -112,7 +112,7 @@ INTERNAL_RESULT_KEYS = ('add_host', 'add_group')
 LOCALHOST = ('127.0.0.1', 'localhost', '::1')
 MODULE_REQUIRE_ARGS = ('command', 'win_command', 'shell', 'win_shell', 'raw', 'script')
 MODULE_NO_JSON = ('command', 'win_command', 'shell', 'win_shell', 'raw')
-RESTRICTED_RESULT_KEYS = ('ansible_rsync_path', 'ansible_playbook_python')
+RESTRICTED_RESULT_KEYS = ('ansible_rsync_path', 'ansible_playbook_python', 'ansible_facts')
 TREE_DIR = None
 VAULT_VERSION_MIN = 1.0
 VAULT_VERSION_MAX = 1.0
diff --git a/lib/ansible/vars/clean.py b/lib/ansible/vars/clean.py
index 7b29383c2e..1987e3c57e 100644
--- a/lib/ansible/vars/clean.py
+++ b/lib/ansible/vars/clean.py
@@ -156,10 +156,9 @@ def namespace_facts(facts):
     ''' return all facts inside 'ansible_facts' w/o an ansible_ prefix '''
     deprefixed = {}
     for k in facts:
-        if k in ('ansible_local',):
-            # exceptions to 'deprefixing'
-            deprefixed[k] = module_response_deepcopy(facts[k])
+        if k.startswith('ansible_') and k not in ('ansible_local',):
+            deprefixed[k[8:]] = module_response_deepcopy(facts[k])
         else:
-            deprefixed[k.replace('ansible_', '', 1)] = module_response_deepcopy(facts[k])
+            deprefixed[k] = module_response_deepcopy(facts[k])
 
     return {'ansible_facts': deprefixed}
diff --git a/test/integration/targets/gathering_facts/library/bogus_facts b/test/integration/targets/gathering_facts/library/bogus_facts
new file mode 100644
index 0000000000..a6aeede546
--- /dev/null
+++ b/test/integration/targets/gathering_facts/library/bogus_facts
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+echo '{
+    "changed": false,
+    "ansible_facts": {
+        "ansible_facts": {
+            "discovered_interpreter_python": "(touch /tmp/pwned-$(date -Iseconds)-$(whoami) ) 2>/dev/null >/dev/null && /usr/bin/python",
+			"bogus_overwrite": "yes"
+         },
+         "dansible_iscovered_interpreter_python": "(touch /tmp/pwned-$(date -Iseconds)-$(whoami) ) 2>/dev/null >/dev/null && /usr/bin/python"
+    }
+}'
diff --git a/test/integration/targets/gathering_facts/runme.sh b/test/integration/targets/gathering_facts/runme.sh
index db23764157..ccea766281 100755
--- a/test/integration/targets/gathering_facts/runme.sh
+++ b/test/integration/targets/gathering_facts/runme.sh
@@ -7,3 +7,6 @@ ansible-playbook test_gathering_facts.yml -i inventory -v "$@"
 # ANSIBLE_CACHE_PLUGIN=base ansible-playbook test_gathering_facts.yml -i inventory -v "$@"
 
 ANSIBLE_GATHERING=smart ansible-playbook test_run_once.yml -i inventory -v "$@"
+
+# ensure clean_facts is working properly
+ansible-playbook test_prevent_injection.yml -i inventory -v "$@"
diff --git a/test/integration/targets/gathering_facts/test_prevent_injection.yml b/test/integration/targets/gathering_facts/test_prevent_injection.yml
new file mode 100644
index 0000000000..f304fe88ec
--- /dev/null
+++ b/test/integration/targets/gathering_facts/test_prevent_injection.yml
@@ -0,0 +1,14 @@
+- name: Ensure clean_facts is working properly
+  hosts: facthost1
+  gather_facts: false
+  tasks:
+    - name: gather 'bad' facts
+      action: bogus_facts
+
+    - name: ensure that the 'bad' facts didn't polute what they are not supposed to
+      assert:
+        that:
+            - "'touch' not in discovered_interpreter_python|default('')"
+            - "'touch' not in ansible_facts.get('discovered_interpreter_python', '')"
+            - "'touch' not in ansible_facts.get('ansible_facts', {}).get('discovered_interpreter_python', '')"
+            - bogus_overwrite is undefined
diff --git a/test/sanity/code-smell/shebang.py b/test/sanity/code-smell/shebang.py
index 7dd2a2718a..61d883c846 100755
--- a/test/sanity/code-smell/shebang.py
+++ b/test/sanity/code-smell/shebang.py
@@ -28,6 +28,7 @@ def main():
         'test/integration/targets/win_module_utils/library/legacy_only_new_way_win_line_ending.ps1',
         'test/integration/targets/win_module_utils/library/legacy_only_old_way_win_line_ending.ps1',
         'test/utils/shippable/timing.py',
+        'test/integration/targets/gathering_facts/library/bogus_facts',
     ])
 
     for path in sys.argv[1:] or sys.stdin.read().splitlines():