Targeted fix for installing roles with symlinks containing '..' (#82165) (#82323)

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 3a42a00368)

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).

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 not (member.isreg() or member.issym()):
+                                continue
 
-                                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]
+                            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()

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__':

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

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
@@ -66,3 +69,4 @@
   command: ansible-galaxy role remove invalid-testrole
 
 - import_tasks: dir-traversal.yml
+- import_tasks: valid-role-symlinks.yml

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