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 <jjelen@redhat.com>

* tests: Add more negative test cases

Signed-off-by: Jakub Jelen <jjelen@redhat.com>

* tests: Skip the test with unsafe TMPDIR as it does not work on CentOS8

Signed-off-by: Jakub Jelen <jjelen@redhat.com>

* 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 <jjelen@redhat.com>

---------

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Co-authored-by: Markus Linnala <Markus.Linnala@knowit.fi>

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 }}

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
 

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

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

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'

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([]) }}"

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([]) }}"

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

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