From 6b09287b0decca424898d522055eac1ff46eac44 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Tue, 7 Nov 2023 16:01:48 -0500 Subject: [PATCH 1/3] Targeted fix for installing roles with symlinks that contain '..' --- lib/ansible/galaxy/role.py | 35 +++++++- .../files/create-role-archive.py | 21 ++++- .../tasks/dir-traversal.yml | 87 +++++++++++++++++++ .../ansible-galaxy-role/tasks/main.yml | 1 + .../tasks/valid-role-symlinks.yml | 78 +++++++++++++++++ 5 files changed, 218 insertions(+), 4 deletions(-) create mode 100644 test/integration/targets/ansible-galaxy-role/tasks/valid-role-symlinks.yml diff --git a/lib/ansible/galaxy/role.py b/lib/ansible/galaxy/role.py index 50873c4ca82d01..7d699a8ea9d12b 100644 --- a/lib/ansible/galaxy/role.py +++ b/lib/ansible/galaxy/role.py @@ -45,6 +45,20 @@ display = Display() +def is_rel_path_outside_archive(archive_parent_dir : str, path : str, path_must_exist : bool = False) -> bool: + """ + Check if the path (relative to the archive) resolves to a path in the archive, and optionally validate the path exists. + """ + try: + # Get the absolute paths + archive_dir = os.path.abspath(archive_parent_dir) + path = os.path.abspath(os.path.join(archive_parent_dir, path)) + + return not (path.startswith(archive_dir) and (not path_must_exist or os.path.exists(path))) + except OSError: + return True + + @functools.cache def _check_working_data_filter() -> bool: """ @@ -400,6 +414,25 @@ def install(self): 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) + + if attr == 'linkname' and (member.islnk() or member.issym()): + # https://docs.python.org/3/library/tarfile.html#tarfile.TarInfo.linkname: + # For symbolic links (SYMTYPE), the linkname is relative to the directory that contains the link. + # For hard links (LNKTYPE), the linkname is relative to the root of the archive. + if member.islnk(): + relative_path = os.path.join(*n_parts) + else: + # name is relative to n_archive_parent_dir + relative_to = os.path.dirname(member.name) + relative_path = os.path.join(relative_to, *n_parts) + + # Because we split on os.sep, any links that were not relative now are. + # To avoid installing incomplete roles successfully, we validate the sanitized paths exist. + bad_original_link = n_attr_value.startswith(os.sep) and not n_attr_value.startswith(n_archive_parent_dir) + + if is_rel_path_outside_archive(n_archive_parent_dir, relative_path, path_must_exist=bad_original_link): + raise AnsibleError(f"symlink '{member.name}' could not be found in the role: {attr_value}") + n_final_parts = [] for n_part in n_parts: # TODO if the condition triggers it produces a broken installation. @@ -412,7 +445,7 @@ def install(self): # to debug a broken installation. if not n_part: continue - if n_part == '..': + if n_part == '..' and attr == 'name': display.warning(f"Illegal filename '{n_part}': '..' is not allowed") continue if n_part.startswith('~'): 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 859ddebcd12be7..3e8424b006ef5c 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 @@ -3,6 +3,7 @@ from __future__ import annotations import argparse +import os import pathlib import tarfile import tempfile @@ -19,6 +20,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, @@ -36,10 +46,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 c70e899879f87a..9d7936779986b7 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,87 @@ - dangerous_overwrite_content.content|default('')|b64decode == '' - not dangerous_overwrite_stat.stat.exists - galaxy_install_dangerous is failed + - >- + "symlink 'symlink' could not be found in 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 + - >- + "symlink 'symlink' could not be found in 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 + - >- + "symlink 'symlink' could not be found in 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 e94176d450de8a..30a5489dc90483 100644 --- a/test/integration/targets/ansible-galaxy-role/tasks/main.yml +++ b/test/integration/targets/ansible-galaxy-role/tasks/main.yml @@ -66,3 +66,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 00000000000000..8a60b2efcc803d --- /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 From 9ec388eeefee611beecc24021d8ea55aefe2414f Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Tue, 7 Nov 2023 16:11:55 -0500 Subject: [PATCH 2/3] changelog --- changelogs/fragments/ansible-galaxy-role-install-symlink.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/ansible-galaxy-role-install-symlink.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 00000000000000..16f7c3889823d5 --- /dev/null +++ b/changelogs/fragments/ansible-galaxy-role-install-symlink.yml @@ -0,0 +1,2 @@ +bugfixes: + - ansible-galaxy role install - allow symlinks with '..' if they resolve to a path in the tarfile's role directory (https://github.com/ansible/ansible/issues/81965). From 68088b34560ee96337a6ad2eaf601c6f887a21ce Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Thu, 16 Nov 2023 13:16:11 -0500 Subject: [PATCH 3/3] Refactor to use ansible.utils.path functions 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 --- .../ansible-galaxy-role-install-symlink.yml | 2 +- lib/ansible/galaxy/role.py | 104 ++++++------------ .../tasks/dir-traversal.yml | 11 +- .../ansible-galaxy-role/tasks/main.yml | 15 ++- 4 files changed, 50 insertions(+), 82 deletions(-) diff --git a/changelogs/fragments/ansible-galaxy-role-install-symlink.yml b/changelogs/fragments/ansible-galaxy-role-install-symlink.yml index 16f7c3889823d5..856c501455c07a 100644 --- a/changelogs/fragments/ansible-galaxy-role-install-symlink.yml +++ b/changelogs/fragments/ansible-galaxy-role-install-symlink.yml @@ -1,2 +1,2 @@ bugfixes: - - ansible-galaxy role install - allow symlinks with '..' if they resolve to a path in the tarfile's role directory (https://github.com/ansible/ansible/issues/81965). + - 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 7d699a8ea9d12b..77334f8630e486 100644 --- a/lib/ansible/galaxy/role.py +++ b/lib/ansible/galaxy/role.py @@ -41,24 +41,11 @@ 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() -def is_rel_path_outside_archive(archive_parent_dir : str, path : str, path_must_exist : bool = False) -> bool: - """ - Check if the path (relative to the archive) resolves to a path in the archive, and optionally validate the path exists. - """ - try: - # Get the absolute paths - archive_dir = os.path.abspath(archive_parent_dir) - path = os.path.abspath(os.path.join(archive_parent_dir, path)) - - return not (path.startswith(archive_dir) and (not path_must_exist or os.path.exists(path))) - except OSError: - return True - - @functools.cache def _check_working_data_filter() -> bool: """ @@ -406,62 +393,41 @@ def install(self): # 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) - - if attr == 'linkname' and (member.islnk() or member.issym()): - # https://docs.python.org/3/library/tarfile.html#tarfile.TarInfo.linkname: - # For symbolic links (SYMTYPE), the linkname is relative to the directory that contains the link. - # For hard links (LNKTYPE), the linkname is relative to the root of the archive. - if member.islnk(): - relative_path = os.path.join(*n_parts) - else: - # name is relative to n_archive_parent_dir - relative_to = os.path.dirname(member.name) - relative_path = os.path.join(relative_to, *n_parts) - - # Because we split on os.sep, any links that were not relative now are. - # To avoid installing incomplete roles successfully, we validate the sanitized paths exist. - bad_original_link = n_attr_value.startswith(os.sep) and not n_attr_value.startswith(n_archive_parent_dir) - - if is_rel_path_outside_archive(n_archive_parent_dir, relative_path, path_must_exist=bad_original_link): - raise AnsibleError(f"symlink '{member.name}' could not be found in the role: {attr_value}") - - 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 == '..' and attr == 'name': - 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/tasks/dir-traversal.yml b/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml index 9d7936779986b7..1c17daf7dd4eaf 100644 --- a/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml +++ b/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml @@ -45,8 +45,7 @@ - dangerous_overwrite_content.content|default('')|b64decode == '' - not dangerous_overwrite_stat.stat.exists - galaxy_install_dangerous is failed - - >- - "symlink 'symlink' could not be found in the role" in (galaxy_install_dangerous.stderr | regex_replace('\n', ' ')) + - "'is not a subpath of the role' in (galaxy_install_dangerous.stderr | regex_replace('\n', ' '))" - name: remove tarfile for next test file: @@ -88,8 +87,8 @@ - dangerous_overwrite_content.content|default('')|b64decode == '' - not dangerous_overwrite_stat.stat.exists - galaxy_install_dangerous is failed - - >- - "symlink 'symlink' could not be found in the role" in (galaxy_install_dangerous.stderr | regex_replace('\n', ' ')) + - "'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' @@ -119,8 +118,8 @@ that: - not symlink_outside_role.stat.exists - galaxy_install_dangerous is failed - - >- - "symlink 'symlink' could not be found in the role" in (galaxy_install_dangerous.stderr | regex_replace('\n', ' ')) + - "'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 }}' diff --git a/test/integration/targets/ansible-galaxy-role/tasks/main.yml b/test/integration/targets/ansible-galaxy-role/tasks/main.yml index 30a5489dc90483..5f88a557652e6c 100644 --- a/test/integration/targets/ansible-galaxy-role/tasks/main.yml +++ b/test/integration/targets/ansible-galaxy-role/tasks/main.yml @@ -25,14 +25,17 @@ - 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: Invalid file - copy: - content: "" +- name: Add another invalid symlink + file: + state: link + src: "/" dest: "{{ remote_tmp_dir }}/role.d/tasks/invalid$name.yml" - name: Valid requirements file