Skip to content

Commit

Permalink
Refactor to use ansible.utils.path functions
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@s-hertel
s-hertel committed Nov 16, 2023
1 parent 9ec388e commit e81a29a
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 82 deletions.
2 changes: 1 addition & 1 deletion changelogs/fragments/ansible-galaxy-role-install-symlink.yml
View file
Original file line number Diff line number Diff line change
@@ -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).
105 changes: 36 additions & 69 deletions lib/ansible/galaxy/role.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand Down Expand Up @@ -406,62 +393,42 @@ 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)

# 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

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))
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()
Expand Down
11 changes: 5 additions & 6 deletions test/integration/targets/ansible-galaxy-role/tasks/dir-traversal.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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 }}'
Expand Down
15 changes: 9 additions & 6 deletions test/integration/targets/ansible-galaxy-role/tasks/main.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit e81a29a

Please sign in to comment.