From 23f8639a4b01f6437f241d835efb68b8b7150575 Mon Sep 17 00:00:00 2001 From: Matt Davis <6775756+nitzmahone@users.noreply.github.com> Date: Tue, 29 Oct 2024 09:38:41 -0700 Subject: [PATCH] [stable-2.18] Preserve `_ansible_no_log` from action result; fix `include_vars` to set properly (#84143) (#84179) * fixes for CVE-2024-8775 * propagate truthy `_ansible_no_log` in action result (previously superseded by task-calculated value) * always mask entire `include_vars` action result if any file loaded had a false `show_content` flag (previously used only the flag value from the last file loaded) * update no_log tests for CVE-2024-8775 * include validation of _ansible_no_log preservation when set by actions * replace static values with dynamic for increased robustness to logging/display/callback changes (but still using grep counts :( ) * changelog * use ternary, coerce to bool explicitly (cherry picked from commit c9ac477e53a99e95781f333eec3329a935c1bf95) --- changelogs/fragments/cve-2024-8775.yml | 5 +++ lib/ansible/executor/task_executor.py | 4 +-- lib/ansible/plugins/action/include_vars.py | 3 +- .../include_vars-ad-hoc/dir/encrypted.yml | 6 ++++ .../targets/include_vars-ad-hoc/runme.sh | 22 +++++++++++-- .../targets/include_vars-ad-hoc/vaultpass | 3 ++ .../action_plugins/action_sets_no_log.py | 8 +++++ .../no_log/ansible_no_log_in_result.yml | 13 ++++++++ test/integration/targets/no_log/dynamic.yml | 29 +++++++++++++---- .../targets/no_log/no_log_config.yml | 2 +- .../targets/no_log/no_log_local.yml | 15 +++++---- .../targets/no_log/no_log_suboptions.yml | 14 ++++---- .../no_log/no_log_suboptions_invalid.yml | 29 +++++++++-------- test/integration/targets/no_log/runme.sh | 18 +++++++---- .../integration/targets/no_log/secretvars.yml | 32 +++++++++++++++++++ 15 files changed, 157 insertions(+), 46 deletions(-) create mode 100644 changelogs/fragments/cve-2024-8775.yml create mode 100644 test/integration/targets/include_vars-ad-hoc/dir/encrypted.yml create mode 100755 test/integration/targets/include_vars-ad-hoc/vaultpass create mode 100644 test/integration/targets/no_log/action_plugins/action_sets_no_log.py create mode 100644 test/integration/targets/no_log/ansible_no_log_in_result.yml create mode 100644 test/integration/targets/no_log/secretvars.yml diff --git a/changelogs/fragments/cve-2024-8775.yml b/changelogs/fragments/cve-2024-8775.yml new file mode 100644 index 00000000000..a292c997044 --- /dev/null +++ b/changelogs/fragments/cve-2024-8775.yml @@ -0,0 +1,5 @@ +security_fixes: + - task result processing - Ensure that action-sourced result masking (``_ansible_no_log=True``) + is preserved. (CVE-2024-8775) + - include_vars action - Ensure that result masking is correctly requested when vault-encrypted + files are read. (CVE-2024-8775) diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 932a33cfec0..0c8c8bb3221 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -684,8 +684,8 @@ class TaskExecutor: self._handler.cleanup() display.debug("handler run complete") - # preserve no log - result["_ansible_no_log"] = no_log + # propagate no log to result- the action can set this, so only overwrite it with the task's value if missing or falsey + result["_ansible_no_log"] = bool(no_log or result.get('_ansible_no_log', False)) if self._task.action not in C._ACTION_WITH_CLEAN_FACTS: result = wrap_var(result) diff --git a/lib/ansible/plugins/action/include_vars.py b/lib/ansible/plugins/action/include_vars.py index c32e6227dbf..693ef0ac4c4 100644 --- a/lib/ansible/plugins/action/include_vars.py +++ b/lib/ansible/plugins/action/include_vars.py @@ -237,7 +237,8 @@ class ActionModule(ActionBase): b_data, show_content = self._loader._get_file_contents(filename) data = to_text(b_data, errors='surrogate_or_strict') - self.show_content = show_content + self.show_content &= show_content # mask all results if any file was encrypted + data = self._loader.load(data, file_name=filename, show_content=show_content) if not data: data = dict() diff --git a/test/integration/targets/include_vars-ad-hoc/dir/encrypted.yml b/test/integration/targets/include_vars-ad-hoc/dir/encrypted.yml new file mode 100644 index 00000000000..328f18082a9 --- /dev/null +++ b/test/integration/targets/include_vars-ad-hoc/dir/encrypted.yml @@ -0,0 +1,6 @@ +$ANSIBLE_VAULT;1.1;AES256 +31613539636636336264396235633933633839646337323533316638633336653461393036336664 +3939386435313638366366626566346135623932653238360a366261303663343034633865626132 +31646231623630333636383636383833656331643164656366623332396439306132663264663131 +6439633766376261320a616265306430366530363866356433366430633265353739373732646536 +37623661333064306162373463616231636365373231313939373230643936313362 diff --git a/test/integration/targets/include_vars-ad-hoc/runme.sh b/test/integration/targets/include_vars-ad-hoc/runme.sh index 51b68d21341..5956e1f42b5 100755 --- a/test/integration/targets/include_vars-ad-hoc/runme.sh +++ b/test/integration/targets/include_vars-ad-hoc/runme.sh @@ -1,6 +1,22 @@ #!/usr/bin/env bash -set -eux +set -eux -o pipefail -ansible testhost -i ../../inventory -m include_vars -a 'dir/inc.yml' "$@" -ansible testhost -i ../../inventory -m include_vars -a 'dir=dir' "$@" +echo "single file include" +ansible testhost -i ../../inventory -m include_vars -a 'dir/inc.yml' -vvv 2>&1 | grep -q 'porter.*cable' + +echo "single file encrypted include" +ansible testhost -i ../../inventory -m include_vars -a 'dir/encrypted.yml' -vvv --vault-password-file vaultpass > output.txt 2>&1 + +echo "directory include with encrypted" +ansible testhost -i ../../inventory -m include_vars -a 'dir=dir' -vvv --vault-password-file vaultpass >> output.txt 2>&1 + +grep -q 'output has been hidden' output.txt + +# all content should be masked if any file is encrypted +if grep -e 'i am a secret' -e 'porter.*cable' output.txt; then + echo "FAIL: vault masking failed" + exit 1 +fi + +echo PASS diff --git a/test/integration/targets/include_vars-ad-hoc/vaultpass b/test/integration/targets/include_vars-ad-hoc/vaultpass new file mode 100755 index 00000000000..1f78d41e66d --- /dev/null +++ b/test/integration/targets/include_vars-ad-hoc/vaultpass @@ -0,0 +1,3 @@ +#!/bin/sh + +echo supersecurepassword diff --git a/test/integration/targets/no_log/action_plugins/action_sets_no_log.py b/test/integration/targets/no_log/action_plugins/action_sets_no_log.py new file mode 100644 index 00000000000..cb426168753 --- /dev/null +++ b/test/integration/targets/no_log/action_plugins/action_sets_no_log.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +from ansible.plugins.action import ActionBase + + +class ActionModule(ActionBase): + def run(self, tmp=None, task_vars=None): + return dict(changed=False, failed=False, msg="action result should be masked", _ansible_no_log="yeppers") # ensure that a truthy non-bool works here diff --git a/test/integration/targets/no_log/ansible_no_log_in_result.yml b/test/integration/targets/no_log/ansible_no_log_in_result.yml new file mode 100644 index 00000000000..a80a4a782e9 --- /dev/null +++ b/test/integration/targets/no_log/ansible_no_log_in_result.yml @@ -0,0 +1,13 @@ +- hosts: localhost + gather_facts: no + tasks: + - action_sets_no_log: + register: res_action + + - assert: + that: + - res_action.msg == "action result should be masked" + + - action_sets_no_log: + loop: [1, 2, 3] + register: res_action diff --git a/test/integration/targets/no_log/dynamic.yml b/test/integration/targets/no_log/dynamic.yml index 4a1123d5749..95236779b08 100644 --- a/test/integration/targets/no_log/dynamic.yml +++ b/test/integration/targets/no_log/dynamic.yml @@ -1,27 +1,42 @@ - name: test dynamic no log hosts: testhost gather_facts: no - ignore_errors: yes tasks: - name: no loop, task fails, dynamic no_log - debug: - msg: "SHOW {{ var_does_not_exist }}" + raw: echo {{ var_does_not_exist }} no_log: "{{ not (unsafe_show_logs|bool) }}" + ignore_errors: yes + register: result + + - assert: + that: + - result is failed + - result.results is not defined - name: loop, task succeeds, dynamic does no_log - debug: - msg: "SHOW {{ item }}" + raw: echo {{ item }} loop: - a - b - c no_log: "{{ not (unsafe_show_logs|bool) }}" + register: result + + - assert: + that: + - result.results | length == 3 - name: loop, task fails, dynamic no_log - debug: - msg: "SHOW {{ var_does_not_exist }}" + raw: echo {{ var_does_not_exist }} loop: - a - b - c no_log: "{{ not (unsafe_show_logs|bool) }}" + ignore_errors: yes + register: result + + - assert: + that: + - result is failed + - result.results is not defined # DT needs result.results | length == 3 diff --git a/test/integration/targets/no_log/no_log_config.yml b/test/integration/targets/no_log/no_log_config.yml index 8a5088059db..165f4e07c54 100644 --- a/test/integration/targets/no_log/no_log_config.yml +++ b/test/integration/targets/no_log/no_log_config.yml @@ -10,4 +10,4 @@ - debug: - debug: - loop: '{{ range(3) }}' + loop: '{{ range(3) | list }}' diff --git a/test/integration/targets/no_log/no_log_local.yml b/test/integration/targets/no_log/no_log_local.yml index aacf7de2769..d2bc5ae482f 100644 --- a/test/integration/targets/no_log/no_log_local.yml +++ b/test/integration/targets/no_log/no_log_local.yml @@ -4,19 +4,22 @@ hosts: testhost gather_facts: no tasks: + - include_vars: secretvars.yml + no_log: true + - name: args should be logged in the absence of no_log - shell: echo "LOG_ME_TASK_SUCCEEDED" + shell: echo "{{log_me_prefix}}TASK_SUCCEEDED" - name: failed args should be logged in the absence of no_log - shell: echo "LOG_ME_TASK_FAILED" + shell: echo "{{log_me_prefix}}TASK_FAILED" failed_when: true ignore_errors: true - name: item args should be logged in the absence of no_log shell: echo {{ item }} - with_items: [ "LOG_ME_ITEM", "LOG_ME_SKIPPED", "LOG_ME_ITEM_FAILED" ] - when: item != "LOG_ME_SKIPPED" - failed_when: item == "LOG_ME_ITEM_FAILED" + with_items: [ "{{log_me_prefix}}ITEM", "{{log_me_prefix}}SKIPPED", "{{log_me_prefix}}ITEM_FAILED" ] + when: item != log_me_prefix ~ "SKIPPED" + failed_when: item == log_me_prefix ~ "ITEM_FAILED" ignore_errors: true - name: args should not be logged when task-level no_log set @@ -61,7 +64,7 @@ no_log: true - name: args should be logged when task-level no_log overrides play-level - shell: echo "LOG_ME_OVERRIDE" + shell: echo "{{log_me_prefix}}OVERRIDE" no_log: false - name: Add a fake host for next play diff --git a/test/integration/targets/no_log/no_log_suboptions.yml b/test/integration/targets/no_log/no_log_suboptions.yml index e67ecfe21b5..338a871eedb 100644 --- a/test/integration/targets/no_log/no_log_suboptions.yml +++ b/test/integration/targets/no_log/no_log_suboptions.yml @@ -5,20 +5,20 @@ tasks: - name: Task with suboptions module: - secret: GLAMOROUS + secret: "{{ s106 }}" subopt_dict: - str_sub_opt1: AFTERMATH + str_sub_opt1: "{{ s107 }}" str_sub_opt2: otherstring nested_subopt: - n_subopt1: MANPOWER + n_subopt1: "{{ s101 }}" subopt_list: - - subopt1: UNTAPPED + - subopt1: "{{ s102 }}" subopt2: thridstring - - subopt1: CONCERNED + - subopt1: "{{ s103 }}" - name: Task with suboptions as string module: - secret: MARLIN - subopt_dict: str_sub_opt1=FLICK + secret: "{{ s104 }}" + subopt_dict: str_sub_opt1={{ s105 }} diff --git a/test/integration/targets/no_log/no_log_suboptions_invalid.yml b/test/integration/targets/no_log/no_log_suboptions_invalid.yml index 933a8a9bb27..1092cf5fbef 100644 --- a/test/integration/targets/no_log/no_log_suboptions_invalid.yml +++ b/test/integration/targets/no_log/no_log_suboptions_invalid.yml @@ -4,42 +4,45 @@ ignore_errors: yes tasks: + - include_vars: secretvars.yml + no_log: true + - name: Task with suboptions and invalid parameter module: - secret: SUPREME + secret: "{{ s201 }}" invalid: param subopt_dict: - str_sub_opt1: IDIOM + str_sub_opt1: "{{ s202 }}" str_sub_opt2: otherstring nested_subopt: - n_subopt1: MOCKUP + n_subopt1: "{{ s203 }}" subopt_list: - - subopt1: EDUCATED + - subopt1: "{{ s204 }}" subopt2: thridstring - - subopt1: FOOTREST + - subopt1: "{{ s205 }}" - name: Task with suboptions as string with invalid parameter module: - secret: FOOTREST + secret: "{{ s213 }}" invalid: param - subopt_dict: str_sub_opt1=CRAFTY + subopt_dict: str_sub_opt1={{ s206 }} - name: Task with suboptions with dict instead of list module: - secret: FELINE + secret: "{{ s207 }}" subopt_dict: - str_sub_opt1: CRYSTAL + str_sub_opt1: "{{ s208 }}" str_sub_opt2: otherstring nested_subopt: - n_subopt1: EXPECTANT + n_subopt1: "{{ s209 }}" subopt_list: foo: bar - name: Task with suboptions with incorrect data type module: - secret: AGROUND + secret: "{{ s210 }}" subopt_dict: 9068.21361 subopt_list: - - subopt1: GOLIATH - - subopt1: FREEFALL + - subopt1: "{{ s211 }}" + - subopt1: "{{ s212 }}" diff --git a/test/integration/targets/no_log/runme.sh b/test/integration/targets/no_log/runme.sh index bf764bf9abc..d6476ac69ca 100755 --- a/test/integration/targets/no_log/runme.sh +++ b/test/integration/targets/no_log/runme.sh @@ -1,26 +1,32 @@ #!/usr/bin/env bash -set -eux +set -eux -o pipefail + +# ensure _ansible_no_log returned by actions is actually respected +ansible-playbook ansible_no_log_in_result.yml -vvvvv > "${OUTPUT_DIR}/output.log" 2> /dev/null + +[ "$(grep -c "action result should be masked" "${OUTPUT_DIR}/output.log")" = "0" ] +[ "$(grep -c "the output has been hidden" "${OUTPUT_DIR}/output.log")" = "4" ] # This test expects 7 loggable vars and 0 non-loggable ones. # If either mismatches it fails, run the ansible-playbook command to debug. [ "$(ansible-playbook no_log_local.yml -i ../../inventory -vvvvv "$@" | awk \ -'BEGIN { logme = 0; nolog = 0; } /LOG_ME/ { logme += 1;} /DO_NOT_LOG/ { nolog += 1;} END { printf "%d/%d", logme, nolog; }')" = "27/0" ] +'BEGIN { logme = 0; nolog = 0; } /LOG_ME/ { logme += 1;} /DO_NOT_LOG/ { nolog += 1;} END { printf "%d/%d", logme, nolog; }')" = "26/0" ] # deal with corner cases with no log and loops # no log enabled, should produce 6 censored messages -[ "$(ansible-playbook dynamic.yml -i ../../inventory -vvvvv "$@" -e unsafe_show_logs=no|grep -c 'output has been hidden')" = "6" ] +[ "$(ansible-playbook dynamic.yml -i ../../inventory -vvvvv "$@" -e unsafe_show_logs=no|grep -c 'output has been hidden')" = "6" ] # DT needs 7 # no log disabled, should produce 0 censored [ "$(ansible-playbook dynamic.yml -i ../../inventory -vvvvv "$@" -e unsafe_show_logs=yes|grep -c 'output has been hidden')" = "0" ] # test no log for sub options -[ "$(ansible-playbook no_log_suboptions.yml -i ../../inventory -vvvvv "$@" | grep -Ec '(MANPOWER|UNTAPPED|CONCERNED|MARLIN|FLICK)')" = "0" ] +[ "$(ansible-playbook no_log_suboptions.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'SECRET')" = "0" ] # test invalid data passed to a suboption -[ "$(ansible-playbook no_log_suboptions_invalid.yml -i ../../inventory -vvvvv "$@" | grep -Ec '(SUPREME|IDIOM|MOCKUP|EDUCATED|FOOTREST|CRAFTY|FELINE|CRYSTAL|EXPECTANT|AGROUND|GOLIATH|FREEFALL)')" = "0" ] +[ "$(ansible-playbook no_log_suboptions_invalid.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'SECRET')" = "0" ] # test variations on ANSIBLE_NO_LOG [ "$(ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "1" ] [ "$(ANSIBLE_NO_LOG=0 ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "1" ] -[ "$(ANSIBLE_NO_LOG=1 ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "6" ] +[ "$(ANSIBLE_NO_LOG=1 ansible-playbook no_log_config.yml -i ../../inventory -vvvvv "$@" | grep -Ec 'the output has been hidden')" = "6" ] # DT needs 5 diff --git a/test/integration/targets/no_log/secretvars.yml b/test/integration/targets/no_log/secretvars.yml new file mode 100644 index 00000000000..0030d747d74 --- /dev/null +++ b/test/integration/targets/no_log/secretvars.yml @@ -0,0 +1,32 @@ +# These values are in a separate vars file and referenced dynamically to avoid spurious counts from contextual error messages +# that show the playbook contents inline (since unencrypted playbook contents are not considered secret). +log_me_prefix: LOG_ME_ + +# Unique values are used for each secret below to ensure that one secret "learned" does not cause another non-secret +# value to be considered secret simply because they share the same value. A common substring is, however, present in +# each one to simplify searching for secret values in test output. Having a unique value for each also helps in +# debugging when unexpected output is encountered. + +# secrets for no_log_suboptions.yml +s101: SECRET101 +s102: SECRET102 +s103: SECRET103 +s104: SECRET104 +s105: SECRET105 +s106: SECRET106 +s107: SECRET107 + +# secrets for no_log_suboptions_invalid.yml +s201: SECRET201 +s202: SECRET202 +s203: SECRET203 +s204: SECRET204 +s205: SECRET205 +s206: SECRET206 +s207: SECRET207 +s208: SECRET208 +s209: SECRET209 +s210: SECRET210 +s211: SECRET211 +s212: SECRET212 +s213: SECRET213