From 1d0d2645eed36ac4e17052ab4eacf240132d96fb Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Tue, 14 Apr 2020 22:14:18 -0400 Subject: [PATCH] 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 --- changelogs/fragments/af_clean.yml | 2 ++ lib/ansible/constants.py | 2 +- lib/ansible/vars/clean.py | 7 +++---- .../targets/gathering_facts/library/bogus_facts | 12 ++++++++++++ test/integration/targets/gathering_facts/runme.sh | 3 +++ .../gathering_facts/test_prevent_injection.yml | 14 ++++++++++++++ test/sanity/code-smell/shebang.py | 1 + 7 files changed, 36 insertions(+), 5 deletions(-) create mode 100644 changelogs/fragments/af_clean.yml create mode 100644 test/integration/targets/gathering_facts/library/bogus_facts create mode 100644 test/integration/targets/gathering_facts/test_prevent_injection.yml diff --git a/changelogs/fragments/af_clean.yml b/changelogs/fragments/af_clean.yml new file mode 100644 index 00000000000..9d14ca58698 --- /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 225361ddd73..fcce72d4f69 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 7b29383c2e5..1987e3c57eb 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 00000000000..a6aeede546b --- /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 db237641579..ccea7662811 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 00000000000..f304fe88ec2 --- /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 7dd2a2718a8..61d883c8469 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():