From 080c3ce90c1a178d8e53dad432578714eafdbeca Mon Sep 17 00:00:00 2001 From: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Date: Thu, 18 Jan 2024 18:17:13 -0500 Subject: [PATCH] Targeted fix for installing roles with symlinks containing '..' (#82165) (#82325) Set the tarfile attribute to a normalized value from unfrackpath instead of validating path parts and omiting potentially invald parts Allow tarfile paths/links containing '..', '$', '~' as long as the normalized realpath is in the tarfile's role directory (cherry picked from commit 3a42a0036875c8cab6a62ab9ea67a365e1dd4781) --- .../ansible-galaxy-role-install-symlink.yml | 2 + lib/ansible/galaxy/role.py | 71 ++++++++------- .../files/create-role-archive.py | 21 ++++- .../tasks/dir-traversal.yml | 86 +++++++++++++++++++ .../ansible-galaxy-role/tasks/main.yml | 15 +++- .../tasks/valid-role-symlinks.yml | 78 +++++++++++++++++ 6 files changed, 231 insertions(+), 42 deletions(-) create mode 100644 changelogs/fragments/ansible-galaxy-role-install-symlink.yml create mode 100644 test/integration/targets/ansible-galaxy-role/tasks/valid-role-symlinks.yml diff --git a/changelogs/fragments/ansible-galaxy-role-install-symlink.yml b/changelogs/fragments/ansible-galaxy-role-install-symlink.yml new file mode 100644 index 00000000000..856c501455c --- /dev/null +++ b/changelogs/fragments/ansible-galaxy-role-install-symlink.yml @@ -0,0 +1,2 @@ +bugfixes: + - ansible-galaxy role install - normalize tarfile paths and symlinks using ``ansible.utils.path.unfrackpath`` and consider them valid as long as the realpath is in the tarfile's role directory (https://github.com/ansible/ansible/issues/81965). diff --git a/lib/ansible/galaxy/role.py b/lib/ansible/galaxy/role.py index 9eb6e7b4fa1..ab36edb2ca8 100644 --- a/lib/ansible/galaxy/role.py +++ b/lib/ansible/galaxy/role.py @@ -42,6 +42,7 @@ from ansible.module_utils.compat.version import LooseVersion from ansible.module_utils.urls import open_url from ansible.playbook.role.requirement import RoleRequirement from ansible.utils.display import Display +from ansible.utils.path import is_subpath, unfrackpath display = Display() @@ -393,43 +394,41 @@ class GalaxyRole(object): # we only extract files, and remove any relative path # bits that might be in the file for security purposes # and drop any containing directory, as mentioned above - if member.isreg() or member.issym(): - for attr in ('name', 'linkname'): - attr_value = getattr(member, attr, None) - if not attr_value: - continue - n_attr_value = to_native(attr_value) - n_archive_parent_dir = to_native(archive_parent_dir) - n_parts = n_attr_value.replace(n_archive_parent_dir, "", 1).split(os.sep) - n_final_parts = [] - for n_part in n_parts: - # TODO if the condition triggers it produces a broken installation. - # It will create the parent directory as an empty file and will - # explode if the directory contains valid files. - # Leaving this as is since the whole module needs a rewrite. - # - # Check if we have any files with illegal names, - # and display a warning if so. This could help users - # to debug a broken installation. - if not n_part: - continue - if n_part == '..': - display.warning(f"Illegal filename '{n_part}': '..' is not allowed") - continue - if n_part.startswith('~'): - display.warning(f"Illegal filename '{n_part}': names cannot start with '~'") - continue - if '$' in n_part: - display.warning(f"Illegal filename '{n_part}': names cannot contain '$'") - continue - n_final_parts.append(n_part) - setattr(member, attr, os.path.join(*n_final_parts)) - - if _check_working_data_filter(): - # deprecated: description='extract fallback without filter' python_version='3.11' - role_tar_file.extract(member, to_native(self.path), filter='data') # type: ignore[call-arg] + if not (member.isreg() or member.issym()): + continue + + for attr in ('name', 'linkname'): + if not (attr_value := getattr(member, attr, None)): + continue + + if attr_value.startswith(os.sep) and not is_subpath(attr_value, archive_parent_dir): + err = f"Invalid {attr} for tarfile member: path {attr_value} is not a subpath of the role {archive_parent_dir}" + raise AnsibleError(err) + + if attr == 'linkname': + # Symlinks are relative to the link + relative_to_archive_dir = os.path.dirname(getattr(member, 'name', '')) + archive_dir_path = os.path.join(archive_parent_dir, relative_to_archive_dir, attr_value) else: - role_tar_file.extract(member, to_native(self.path)) + # Normalize paths that start with the archive dir + attr_value = attr_value.replace(archive_parent_dir, "", 1) + attr_value = os.path.join(*attr_value.split(os.sep)) # remove leading os.sep + archive_dir_path = os.path.join(archive_parent_dir, attr_value) + + resolved_archive = unfrackpath(archive_parent_dir) + resolved_path = unfrackpath(archive_dir_path) + if not is_subpath(resolved_path, resolved_archive): + err = f"Invalid {attr} for tarfile member: path {resolved_path} is not a subpath of the role {resolved_archive}" + raise AnsibleError(err) + + relative_path = os.path.join(*resolved_path.replace(resolved_archive, "", 1).split(os.sep)) or '.' + setattr(member, attr, relative_path) + + if _check_working_data_filter(): + # deprecated: description='extract fallback without filter' python_version='3.11' + role_tar_file.extract(member, to_native(self.path), filter='data') # type: ignore[call-arg] + else: + role_tar_file.extract(member, to_native(self.path)) # write out the install info file for later use self._write_galaxy_install_info() diff --git a/test/integration/targets/ansible-galaxy-role/files/create-role-archive.py b/test/integration/targets/ansible-galaxy-role/files/create-role-archive.py index cfd908c17b2..487666381fe 100755 --- a/test/integration/targets/ansible-galaxy-role/files/create-role-archive.py +++ b/test/integration/targets/ansible-galaxy-role/files/create-role-archive.py @@ -2,6 +2,7 @@ """Create a role archive which overwrites an arbitrary file.""" import argparse +import os import pathlib import tarfile import tempfile @@ -18,6 +19,15 @@ def main() -> None: create_archive(args.archive, args.content, args.target) +def generate_files_from_path(path): + if os.path.isdir(path): + for subpath in os.listdir(path): + _path = os.path.join(path, subpath) + yield from generate_files_from_path(_path) + elif os.path.isfile(path): + yield pathlib.Path(path) + + def create_archive(archive_path: pathlib.Path, content_path: pathlib.Path, target_path: pathlib.Path) -> None: with ( tarfile.open(name=archive_path, mode='w') as role_archive, @@ -35,10 +45,15 @@ def create_archive(archive_path: pathlib.Path, content_path: pathlib.Path, targe role_archive.add(meta_main_path) role_archive.add(symlink_path) - content_tarinfo = role_archive.gettarinfo(content_path, str(symlink_path)) + for path in generate_files_from_path(content_path): + if path == content_path: + arcname = str(symlink_path) + else: + arcname = os.path.join(temp_dir_path, path) - with content_path.open('rb') as content_file: - role_archive.addfile(content_tarinfo, content_file) + content_tarinfo = role_archive.gettarinfo(path, arcname) + with path.open('rb') as file_content: + role_archive.addfile(content_tarinfo, file_content) if __name__ == '__main__': diff --git a/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml b/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml index c70e899879f..1c17daf7dd4 100644 --- a/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml +++ b/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml @@ -23,6 +23,9 @@ command: cmd: ansible-galaxy role install --roles-path '{{ remote_tmp_dir }}/dir-traversal/roles' dangerous.tar chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + environment: + ANSIBLE_NOCOLOR: True + ANSIBLE_FORCE_COLOR: False ignore_errors: true register: galaxy_install_dangerous @@ -42,3 +45,86 @@ - dangerous_overwrite_content.content|default('')|b64decode == '' - not dangerous_overwrite_stat.stat.exists - galaxy_install_dangerous is failed + - "'is not a subpath of the role' in (galaxy_install_dangerous.stderr | regex_replace('\n', ' '))" + +- name: remove tarfile for next test + file: + path: '{{ item }}' + state: absent + loop: + - '{{ remote_tmp_dir }}/dir-traversal/source/dangerous.tar' + - '{{ remote_tmp_dir }}/dir-traversal/roles/dangerous.tar' + +- name: build dangerous dir traversal role that includes .. in the symlink path + script: + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + cmd: create-role-archive.py dangerous.tar content.txt {{ remote_tmp_dir }}/dir-traversal/source/../target/target-file-to-overwrite.txt + executable: '{{ ansible_playbook_python }}' + +- name: install dangerous role + command: + cmd: 'ansible-galaxy role install --roles-path {{ remote_tmp_dir }}/dir-traversal/roles dangerous.tar' + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + environment: + ANSIBLE_NOCOLOR: True + ANSIBLE_FORCE_COLOR: False + ignore_errors: true + register: galaxy_install_dangerous + +- name: check for overwritten file + stat: + path: '{{ remote_tmp_dir }}/dir-traversal/target/target-file-to-overwrite.txt' + register: dangerous_overwrite_stat + +- name: get overwritten content + slurp: + path: '{{ remote_tmp_dir }}/dir-traversal/target/target-file-to-overwrite.txt' + register: dangerous_overwrite_content + when: dangerous_overwrite_stat.stat.exists + +- assert: + that: + - dangerous_overwrite_content.content|default('')|b64decode == '' + - not dangerous_overwrite_stat.stat.exists + - galaxy_install_dangerous is failed + - "'is not a subpath of the role' in (galaxy_install_dangerous.stderr | regex_replace('\n', ' '))" + +- name: remove tarfile for next test + file: + path: '{{ remote_tmp_dir }}/dir-traversal/source/dangerous.tar' + state: absent + +- name: build dangerous dir traversal role that includes .. in the relative symlink path + script: + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + cmd: create-role-archive.py dangerous_rel.tar content.txt ../context.txt + +- name: install dangerous role with relative symlink + command: + cmd: 'ansible-galaxy role install --roles-path {{ remote_tmp_dir }}/dir-traversal/roles dangerous_rel.tar' + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + environment: + ANSIBLE_NOCOLOR: True + ANSIBLE_FORCE_COLOR: False + ignore_errors: true + register: galaxy_install_dangerous + +- name: check for symlink outside role + stat: + path: "{{ remote_tmp_dir | realpath }}/dir-traversal/roles/symlink" + register: symlink_outside_role + +- assert: + that: + - not symlink_outside_role.stat.exists + - galaxy_install_dangerous is failed + - "'is not a subpath of the role' in (galaxy_install_dangerous.stderr | regex_replace('\n', ' '))" + +- name: remove test directories + file: + path: '{{ remote_tmp_dir }}/dir-traversal/{{ item }}' + state: absent + loop: + - source + - target + - roles diff --git a/test/integration/targets/ansible-galaxy-role/tasks/main.yml b/test/integration/targets/ansible-galaxy-role/tasks/main.yml index b39df11ca8b..5f88a557652 100644 --- a/test/integration/targets/ansible-galaxy-role/tasks/main.yml +++ b/test/integration/targets/ansible-galaxy-role/tasks/main.yml @@ -25,10 +25,18 @@ - name: Valid role archive command: "tar cf {{ remote_tmp_dir }}/valid-role.tar {{ remote_tmp_dir }}/role.d" -- name: Invalid file - copy: - content: "" +- name: Add invalid symlink + file: + state: link + src: "~/invalid" dest: "{{ remote_tmp_dir }}/role.d/tasks/~invalid.yml" + force: yes + +- name: Add another invalid symlink + file: + state: link + src: "/" + dest: "{{ remote_tmp_dir }}/role.d/tasks/invalid$name.yml" - name: Valid requirements file copy: @@ -61,3 +69,4 @@ command: ansible-galaxy role remove invalid-testrole - import_tasks: dir-traversal.yml +- import_tasks: valid-role-symlinks.yml diff --git a/test/integration/targets/ansible-galaxy-role/tasks/valid-role-symlinks.yml b/test/integration/targets/ansible-galaxy-role/tasks/valid-role-symlinks.yml new file mode 100644 index 00000000000..8a60b2efcc8 --- /dev/null +++ b/test/integration/targets/ansible-galaxy-role/tasks/valid-role-symlinks.yml @@ -0,0 +1,78 @@ +- name: create test directories + file: + path: '{{ remote_tmp_dir }}/dir-traversal/{{ item }}' + state: directory + loop: + - source + - target + - roles + +- name: create subdir in the role content to test relative symlinks + file: + dest: '{{ remote_tmp_dir }}/dir-traversal/source/role_subdir' + state: directory + +- copy: + dest: '{{ remote_tmp_dir }}/dir-traversal/source/role_subdir/.keep' + content: '' + +- set_fact: + installed_roles: "{{ remote_tmp_dir | realpath }}/dir-traversal/roles" + +- name: build role with symlink to a directory in the role + script: + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + cmd: create-role-archive.py safe-link-dir.tar ./ role_subdir/.. + executable: '{{ ansible_playbook_python }}' + +- name: install role successfully + command: + cmd: 'ansible-galaxy role install --roles-path {{ remote_tmp_dir }}/dir-traversal/roles safe-link-dir.tar' + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + register: galaxy_install_ok + +- name: check for the directory symlink in the role + stat: + path: "{{ installed_roles }}/safe-link-dir.tar/symlink" + register: symlink_in_role + +- assert: + that: + - symlink_in_role.stat.exists + - symlink_in_role.stat.lnk_source == installed_roles + '/safe-link-dir.tar' + +- name: remove tarfile for next test + file: + path: '{{ remote_tmp_dir }}/dir-traversal/source/safe-link-dir.tar' + state: absent + +- name: build role with safe relative symlink + script: + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + cmd: create-role-archive.py safe.tar ./ role_subdir/../context.txt + executable: '{{ ansible_playbook_python }}' + +- name: install role successfully + command: + cmd: 'ansible-galaxy role install --roles-path {{ remote_tmp_dir }}/dir-traversal/roles safe.tar' + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + register: galaxy_install_ok + +- name: check for symlink in role + stat: + path: "{{ installed_roles }}/safe.tar/symlink" + register: symlink_in_role + +- assert: + that: + - symlink_in_role.stat.exists + - symlink_in_role.stat.lnk_source == installed_roles + '/safe.tar/context.txt' + +- name: remove test directories + file: + path: '{{ remote_tmp_dir }}/dir-traversal/{{ item }}' + state: absent + loop: + - source + - target + - roles