From d10f2ada11cb1430c29636b69ca457ba89f3ede2 Mon Sep 17 00:00:00 2001 From: Jakub Jelen Date: Thu, 12 Sep 2024 08:24:22 +0200 Subject: [PATCH] fix: use quote with command, shell and validate with variable (#298) * robustness: quote test backup/restore file names This avoids issues if file names are not safepaths. * security: use quote with command, shell and validate with variable Skip quotation only if variable is checked. Add test suit to excercise some quote use cases. * robustness: fail if systemd.unit could have something in need of quote Ensure systemd.unit contents is robust. This disables possibility to have something that needs to be quoted there. But as ansible lacks proper way to quote systemd unit files (see man systemd.syntax, rules are not shell rules), it is better to fail such configs. If you are trying to do that, you are doing it wrong anyway or have malicious intent. Also ensure similar issue with sysctl.conf. Issue can be seen with `tests_hostkeys_unsafe_path.yml`, when adding following to role params: sshd_install_service: true sshd_config_file: "{{ ansible_facts.env.TMPDIR }}/sshd.d/foo.conf" sshd_binary: "{{ ansible_facts.env.TMPDIR }}/sshd" __sshd_runtime_directory: "{{ ansible_facts.env.TMPDIR }}/run" * tests: Quote also the source filename Signed-off-by: Jakub Jelen * tests: Add more negative test cases Signed-off-by: Jakub Jelen * tests: Skip the test with unsafe TMPDIR as it does not work on CentOS8 Signed-off-by: Jakub Jelen * Move the variable checks to separate file ... ... and explain better why this is problematic Drops also the check for internal variables as the user should not bother with these. Signed-off-by: Jakub Jelen --------- Signed-off-by: Jakub Jelen Co-authored-by: Markus Linnala --- tasks/check_vars.yml | 24 +++++++ tasks/install.yml | 7 ++- tasks/install_config.yml | 8 +-- tasks/install_namespace.yml | 4 +- tasks/install_service.yml | 2 +- tests/tasks/backup.yml | 6 +- tests/tasks/restore.yml | 8 +-- tests/tests_hostkeys_unsafe_path.yml | 70 +++++++++++++++++++++ tests/tests_unsafe_options.yml | 93 ++++++++++++++++++++++++++++ 9 files changed, 206 insertions(+), 16 deletions(-) create mode 100644 tasks/check_vars.yml create mode 100644 tests/tests_hostkeys_unsafe_path.yml create mode 100644 tests/tests_unsafe_options.yml diff --git a/tasks/check_vars.yml b/tasks/check_vars.yml new file mode 100644 index 00000000..0fe9a3e9 --- /dev/null +++ b/tasks/check_vars.yml @@ -0,0 +1,24 @@ +--- +- name: Ensure sshd_sysconfig_use_strong_rng is safe to use in shell/command + ansible.builtin.assert: + that: + - (sshd_sysconfig_use_strong_rng | string) == (sshd_sysconfig_use_strong_rng | quote) + msg: | + The variable `sshd_sysconfig_use_strong_rng` is not safe for shell/command/template expansions: + sshd_sysconfig_use_strong_rng: {{ sshd_sysconfig_use_strong_rng }} == {{ sshd_sysconfig_use_strong_rng | quote }} + +- name: Ensure sshd_binary is safe to use in shell/command + ansible.builtin.assert: + that: + - sshd_binary == (sshd_binary | quote) + msg: | + The variable `sshd_binary` is not safe for shell/command/template expansions: + sshd_binary: {{ sshd_binary }} == {{ sshd_binary | quote }} + +- name: Ensure sshd_config_file is safe to use in shell/command + ansible.builtin.assert: + that: + - sshd_config_file == (sshd_config_file | quote) + msg: | + The variable `sshd_sysconfig_use_strong_rng` is not safe for shell/command/template expansions: + sshd_config_file: {{ sshd_config_file }} == {{ sshd_config_file | quote }} diff --git a/tasks/install.yml b/tasks/install.yml index 9e6c409c..cbc4b7dd 100644 --- a/tasks/install.yml +++ b/tasks/install.yml @@ -4,6 +4,9 @@ when: - not __sshd_os_supported | bool +- name: Check variables are safe for use for shell expansions and word splitting + ansible.builtin.include_tasks: check_vars.yml + - name: Install ssh packages ansible.builtin.package: name: "{{ sshd_packages }}" @@ -80,7 +83,7 @@ {% if sshd_sysconfig %} source /etc/sysconfig/sshd {% endif %} - ssh-keygen -q -t {{ item | regex_search('(rsa|dsa|ecdsa|ed25519)') }} -f {{ item }} -C '' -N '' + ssh-keygen -q -t {{ item | regex_search('(rsa|dsa|ecdsa|ed25519)') }} -f {{ item | quote }} -C '' -N '' args: creates: "{{ item }}" loop: "{{ __sshd_verify_hostkeys | from_json | list }}" @@ -109,7 +112,7 @@ - name: Generate temporary hostkey ansible.builtin.command: > - ssh-keygen -q -t rsa -f '{{ sshd_test_hostkey.path }}/rsa_key' -C '' -N '' + ssh-keygen -q -t rsa -f {{ sshd_test_hostkey.path | quote }}/rsa_key -C '' -N '' changed_when: false when: sshd_test_hostkey.path is defined diff --git a/tasks/install_config.yml b/tasks/install_config.yml index 1c4efb12..4aa17a2a 100644 --- a/tasks/install_config.yml +++ b/tasks/install_config.yml @@ -19,9 +19,9 @@ {% if not __sshd_supports_validate %} true %s {% elif sshd_test_hostkey is defined and sshd_test_hostkey.path is defined %} - {{ sshd_binary }} -t -f %s -h {{ sshd_test_hostkey.path }}/rsa_key + {{ sshd_binary | quote }} -t -f %s -h {{ sshd_test_hostkey.path | quote }}/rsa_key {% else %} - {{ sshd_binary }} -t -f %s + {{ sshd_binary | quote }} -t -f %s {% endif %} backup: "{{ sshd_backup }}" notify: Reload_sshd @@ -38,9 +38,9 @@ {% if not __sshd_supports_validate %} true %s {% elif sshd_test_hostkey is defined and sshd_test_hostkey.path is defined %} - {{ sshd_binary }} -t -f %s -h {{ sshd_test_hostkey.path }}/rsa_key + {{ sshd_binary | quote }} -t -f %s -h {{ sshd_test_hostkey.path | quote }}/rsa_key {% else %} - {{ sshd_binary }} -t -f %s + {{ sshd_binary | quote }} -t -f %s {% endif %} backup: "{{ sshd_backup }}" notify: Reload_sshd diff --git a/tasks/install_namespace.yml b/tasks/install_namespace.yml index 1e7e1b64..3a879e04 100644 --- a/tasks/install_namespace.yml +++ b/tasks/install_namespace.yml @@ -16,9 +16,9 @@ {% if not __sshd_supports_validate %} true %s {% elif sshd_test_hostkey is defined and sshd_test_hostkey.path is defined %} - {{ sshd_binary }} -t -f %s -h {{ sshd_test_hostkey.path }}/rsa_key + {{ sshd_binary | quote }} -t -f %s -h {{ sshd_test_hostkey.path | quote }}/rsa_key {% else %} - {{ sshd_binary }} -t -f %s + {{ sshd_binary | quote }} -t -f %s {% endif %} backup: "{{ sshd_backup }}" notify: Reload_sshd diff --git a/tasks/install_service.yml b/tasks/install_service.yml index b843449f..41e164f8 100644 --- a/tasks/install_service.yml +++ b/tasks/install_service.yml @@ -46,7 +46,7 @@ # Due to ansible bug 21026, cannot use service module on RHEL 7 - name: Enable service in chroot - ansible.builtin.command: systemctl enable {{ sshd_service }} # noqa command-instead-of-module + ansible.builtin.command: systemctl enable {{ sshd_service | quote }} # noqa command-instead-of-module when: - ansible_connection == 'chroot' - ansible_facts['os_family'] == 'RedHat' diff --git a/tests/tasks/backup.yml b/tests/tasks/backup.yml index fe646244..f30ab4c7 100644 --- a/tests/tasks/backup.yml +++ b/tests/tasks/backup.yml @@ -16,9 +16,9 @@ set -o pipefail fi set -eu - if test -f {{ item }}; then - mkdir -p {{ __sshd_test_backup.path }}/$(dirname {{ item }}) - cp -a {{ item }} {{ __sshd_test_backup.path }}/$(dirname {{ item }}) + if test -f {{ item | quote }}; then + mkdir -p {{ __sshd_test_backup.path | quote }}/$(dirname {{ item | quote }}) + cp -a {{ item | quote }} {{ __sshd_test_backup.path | quote }}/$(dirname {{ item | quote }}) fi changed_when: false loop: "{{ __sshd_test_backup_files | d([]) }}" diff --git a/tests/tasks/restore.yml b/tests/tasks/restore.yml index 2a1959a7..9999d855 100644 --- a/tests/tasks/restore.yml +++ b/tests/tasks/restore.yml @@ -5,10 +5,10 @@ if set -o | grep pipefail 2>&1 /dev/null ; then set -o pipefail fi - if test -f {{ __sshd_test_backup.path }}/{{ item }}; then - cp -a {{ __sshd_test_backup.path }}/{{ item }} $(dirname {{ item }}) - elif test -f {{ item }}; then - rm {{ item }} + if test -f {{ __sshd_test_backup.path | quote }}/{{ item | quote }}; then + cp -a {{ __sshd_test_backup.path | quote }}/{{ item | quote }} $(dirname {{ item | quote }}) + elif test -f {{ item | quote }}; then + rm {{ item | quote }} fi changed_when: false loop: "{{ __sshd_test_backup_files | d([]) }}" diff --git a/tests/tests_hostkeys_unsafe_path.yml b/tests/tests_hostkeys_unsafe_path.yml new file mode 100644 index 00000000..560894a1 --- /dev/null +++ b/tests/tests_hostkeys_unsafe_path.yml @@ -0,0 +1,70 @@ +--- +- name: Test quote with unsafe input + hosts: all + environment: + TMPDIR: "{{ __tmpdir }}" + vars: + __sshd_test_backup_files: + - /etc/ssh/sshd_config + - /etc/ssh/sshd_config.d/00-ansible_system_role.conf + __badflag_file: /tmp/BADFLAG + # Avoid / in TMPDIR file name + __badflag: >- + $(touch -- "$(echo {{ __badflag_file | b64encode }} | base64 -d)") + # Iterate w/o quote, w/ ' and w/ " + __tmpdir: >- + /tmp/a {{ __badflag }} ' {{ __badflag }} '" {{ __badflag }} "b + + tasks: + - name: Ensure BADFLAG does not exist + ansible.builtin.file: + path: /tmp/BADFLAG + state: absent + + - name: Assert TMPDIR is correctly set + ansible.builtin.assert: + that: + - __tmpdir != '' + - ansible_facts.env.TMPDIR == __tmpdir + + - name: "Backup configuration files" + ansible.builtin.include_tasks: tasks/backup.yml + + - name: Create BAD TMPDIR + ansible.builtin.file: + state: directory + path: "{{ ansible_facts.env.TMPDIR }}" + mode: '0755' + + - name: Configure sshd with BAD config + ansible.builtin.include_role: + name: ansible-sshd + vars: + sshd_skip_defaults: true + sshd_verify_hostkeys: [] + when: + - ansible_facts['os_family'] != 'RedHat' or ansible_facts['distribution_major_version'] | int != 8 + + - name: Verify the options are correctly set + tags: tests::verify + block: + - name: Flush handlers + ansible.builtin.meta: flush_handlers + + - name: Get status BADFLAG + ansible.builtin.stat: + path: "{{ __badflag_file }}" + register: badflag + + - name: Ensure BADFLAG does not exist + ansible.builtin.assert: + that: + - not badflag.stat.exists + + - name: Remove BAD TMPDIR + ansible.builtin.file: + state: absent + path: "{{ ansible_facts.env.TMPDIR }}" + + - name: "Restore configuration files" + ansible.builtin.include_tasks: tasks/restore.yml diff --git a/tests/tests_unsafe_options.yml b/tests/tests_unsafe_options.yml new file mode 100644 index 00000000..a36f799a --- /dev/null +++ b/tests/tests_unsafe_options.yml @@ -0,0 +1,93 @@ +--- +- name: Test quote with unsafe input + hosts: all + vars: + __sshd_test_backup_files: + - /etc/ssh/sshd_config + - /etc/ssh/sshd_config.d/00-ansible_system_role.conf + + tasks: + - name: "Backup configuration files" + ansible.builtin.include_tasks: tasks/backup.yml + + - name: Configure sshd with bad sysconfig configuration + block: + - name: Include the role + ansible.builtin.include_role: + name: ansible-sshd + vars: + sshd_skip_defaults: true + sshd_verify_hostkeys: [] + sshd_sysconfig_use_strong_rng: "maybe yes" + register: role_result + + - name: Unreachable task -- the role should have failed! + ansible.builtin.fail: + msg: UNREACH + + rescue: + - name: Check that we failed in the role + ansible.builtin.assert: + that: + - ansible_failed_result.msg != 'UNREACH' + - not role_result.changed + msg: "Role has not failed when it should have with invalid inputs" + + - name: Configure sshd with bad path to sshd binary + block: + - name: Include the role + ansible.builtin.include_role: + name: ansible-sshd + vars: + sshd_skip_defaults: true + sshd_verify_hostkeys: [] + sshd_binary: "/usr/sbin/sshd binary" + register: role_result + + - name: Unreachable task -- the role should have failed! + ansible.builtin.fail: + msg: UNREACH + + rescue: + - name: Check that we failed in the role + ansible.builtin.assert: + that: + - ansible_failed_result.msg != 'UNREACH' + - not role_result.changed + msg: "Role has not failed when it should have with invalid inputs" + + - name: Configure sshd with bad path sshd config + block: + - name: Include the role + ansible.builtin.include_role: + name: ansible-sshd + vars: + sshd_skip_defaults: true + sshd_verify_hostkeys: [] + sshd_config_file: /etc/ssh/sshd.config.d/my fancy config + register: role_result + + - name: Unreachable task -- the role should have failed! + ansible.builtin.fail: + msg: UNREACH + + rescue: + - name: Check that we failed in the role + ansible.builtin.assert: + that: + - ansible_failed_result.msg != 'UNREACH' + - not role_result.changed + msg: "Role has not failed when it should have with invalid inputs" + + - name: Make sure service is still running + ansible.builtin.service: + name: sshd + state: started + register: result + failed_when: result.changed + tags: tests::verify + when: + - not (ansible_facts['os_family'] == 'RedHat' and ansible_facts['distribution_major_version'] == '6') + + - name: "Restore configuration files" + ansible.builtin.include_tasks: tasks/restore.yml