From ddf0311c63287e2d5334770377350c1e0cbfff28 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Tue, 26 Sep 2023 11:32:19 -0500 Subject: [PATCH] Prevent roles from using symlinks to overwrite files outside of the installation directory (#81780) * Sanitize linkname during role installs * Add tests * add clog frag --- changelogs/fragments/cve-2023-5115.yml | 3 ++ lib/ansible/galaxy/role.py | 52 +++++++++++-------- .../files/create-role-archive.py | 45 ++++++++++++++++ .../tasks/dir-traversal.yml | 44 ++++++++++++++++ .../ansible-galaxy-role/tasks/main.yml | 2 + 5 files changed, 123 insertions(+), 23 deletions(-) create mode 100644 changelogs/fragments/cve-2023-5115.yml create mode 100755 test/integration/targets/ansible-galaxy-role/files/create-role-archive.py create mode 100644 test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml diff --git a/changelogs/fragments/cve-2023-5115.yml b/changelogs/fragments/cve-2023-5115.yml new file mode 100644 index 00000000000..69e0ddb7659 --- /dev/null +++ b/changelogs/fragments/cve-2023-5115.yml @@ -0,0 +1,3 @@ +security_fixes: +- ansible-galaxy - Prevent roles from using symlinks to overwrite + files outside of the installation directory (CVE-2023-5115) diff --git a/lib/ansible/galaxy/role.py b/lib/ansible/galaxy/role.py index a71749fc131..2354ef7c3a1 100644 --- a/lib/ansible/galaxy/role.py +++ b/lib/ansible/galaxy/role.py @@ -394,30 +394,36 @@ class GalaxyRole(object): # bits that might be in the file for security purposes # and drop any containing directory, as mentioned above if member.isreg() or member.issym(): - n_member_name = to_native(member.name) - n_archive_parent_dir = to_native(archive_parent_dir) - n_parts = n_member_name.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 n_part == '..': - display.warning(f"Illegal filename '{n_part}': '..' is not allowed") + for attr in ('name', 'linkname'): + attr_value = getattr(member, attr, None) + if not attr_value: 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) - member.name = os.path.join(*n_final_parts) + 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' 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 new file mode 100755 index 00000000000..cfd908c17b2 --- /dev/null +++ b/test/integration/targets/ansible-galaxy-role/files/create-role-archive.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python +"""Create a role archive which overwrites an arbitrary file.""" + +import argparse +import pathlib +import tarfile +import tempfile + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('archive', type=pathlib.Path, help='archive to create') + parser.add_argument('content', type=pathlib.Path, help='content to write') + parser.add_argument('target', type=pathlib.Path, help='file to overwrite') + + args = parser.parse_args() + + create_archive(args.archive, args.content, args.target) + + +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, + tempfile.TemporaryDirectory() as temp_dir_name, + ): + temp_dir_path = pathlib.Path(temp_dir_name) + + meta_main_path = temp_dir_path / 'meta' / 'main.yml' + meta_main_path.parent.mkdir() + meta_main_path.write_text('') + + symlink_path = temp_dir_path / 'symlink' + symlink_path.symlink_to(target_path) + + role_archive.add(meta_main_path) + role_archive.add(symlink_path) + + content_tarinfo = role_archive.gettarinfo(content_path, str(symlink_path)) + + with content_path.open('rb') as content_file: + role_archive.addfile(content_tarinfo, content_file) + + +if __name__ == '__main__': + 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 new file mode 100644 index 00000000000..c70e899879f --- /dev/null +++ b/test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml @@ -0,0 +1,44 @@ +- name: create test directories + file: + path: '{{ remote_tmp_dir }}/dir-traversal/{{ item }}' + state: directory + loop: + - source + - target + - roles + +- name: create test content + copy: + dest: '{{ remote_tmp_dir }}/dir-traversal/source/content.txt' + content: | + some content to write + +- name: build dangerous dir traversal role + script: + chdir: '{{ remote_tmp_dir }}/dir-traversal/source' + cmd: create-role-archive.py dangerous.tar content.txt {{ remote_tmp_dir }}/dir-traversal/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' + 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 diff --git a/test/integration/targets/ansible-galaxy-role/tasks/main.yml b/test/integration/targets/ansible-galaxy-role/tasks/main.yml index 3e1e6e7e71b..e94176d450d 100644 --- a/test/integration/targets/ansible-galaxy-role/tasks/main.yml +++ b/test/integration/targets/ansible-galaxy-role/tasks/main.yml @@ -64,3 +64,5 @@ - name: Uninstall invalid role command: ansible-galaxy role remove invalid-testrole + +- import_tasks: dir-traversal.yml