From a9d2ceafe429171c0e2ad007058b88bae57c74ce Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Tue, 24 Mar 2020 15:46:56 -0400 Subject: [PATCH] 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 --- changelogs/fragments/af_clean.yml | 2 ++ lib/ansible/constants.py | 2 +- lib/ansible/vars/clean.py | 8 +++----- .../targets/gathering_facts/library/bogus_facts | 12 ++++++++++++ test/integration/targets/gathering_facts/runme.sh | 3 +++ .../gathering_facts/test_prevent_injection.yml | 14 ++++++++++++++ test/sanity/ignore.txt | 1 + 7 files changed, 36 insertions(+), 6 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 3711e60f286..754145d115b 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -112,7 +112,7 @@ MODULE_REQUIRE_ARGS = ('command', 'win_command', 'ansible.windows.win_command', 'ansible.windows.win_shell', 'raw', 'script') MODULE_NO_JSON = ('command', 'win_command', 'ansible.windows.win_command', 'shell', 'win_shell', 'ansible.windows.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 ba494467b7c..4b89b7b429b 100644 --- a/lib/ansible/vars/clean.py +++ b/lib/ansible/vars/clean.py @@ -16,7 +16,6 @@ from ansible.module_utils.common._collections_compat import MutableMapping, Muta from ansible.plugins.loader import connection_loader from ansible.utils.display import Display - display = Display() @@ -169,10 +168,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/ignore.txt b/test/sanity/ignore.txt index abe067b72d8..89ff84dbaac 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -269,6 +269,7 @@ test/integration/targets/collections_relative_imports/collection_root/ansible_co test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py pylint:relative-beyond-top-level test/integration/targets/expect/files/test_command.py future-import-boilerplate test/integration/targets/expect/files/test_command.py metaclass-boilerplate +test/integration/targets/gathering_facts/library/bogus_facts shebang test/integration/targets/get_url/files/testserver.py future-import-boilerplate test/integration/targets/get_url/files/testserver.py metaclass-boilerplate test/integration/targets/group/files/gidget.py future-import-boilerplate