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 <[email protected]> * tests: Add more negative test cases Signed-off-by: Jakub Jelen <[email protected]> * tests: Skip the test with unsafe TMPDIR as it does not work on CentOS8 Signed-off-by: Jakub Jelen <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Jakub Jelen <[email protected]> Co-authored-by: Markus Linnala <[email protected]>
willshersystems · Sep 12, 2024 · d10f2ad · d10f2ad
1 parent de66f60
commit d10f2ad
Show file tree

Hide file tree

Showing 9 changed files with 206 additions and 16 deletions.
diff --git a/tasks/check_vars.yml 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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