Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Nov 30, 2023

Conversation

s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Nov 7, 2023

SUMMARY

Fixes #81965

I'd like to backport this as far as acceptable since the fix for the symlink CVE was backported to 2.13, so I separated this from the other fixes in #82052 related to installing broken roles.

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. has_issue labels Nov 7, 2023
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Nov 7, 2023
@Akasurde
Copy link
Member

Akasurde commented Nov 8, 2023

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Nov 8, 2023
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Nov 9, 2023
lib/ansible/galaxy/role.py Outdated Show resolved Hide resolved
@s-hertel s-hertel marked this pull request as draft November 15, 2023 16:24
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
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 16, 2023
@s-hertel
Copy link
Contributor Author

s-hertel commented Nov 17, 2023

I thought I might be able to simplify this quite a bit by using tarfile.extractfile to verify the member is in the tarfile before extracting:

try:
    # throw KeyError if the member missing or external
    file_data = role_tar_file.extractfile(member)
except KeyError as e:
    raise AnsibleError(f"Invalid tarfile member is not a subpath of the role: {e}")

if file_data is None:
    # happens when not (member.isreg() or member.issym()) - we don't extract these
    continue

# then extract as usual

But tarfile.extractfile does not normalize the path/link before using it to determine if it exists in the tarfile, so this won't help.

@s-hertel s-hertel marked this pull request as ready for review November 17, 2023 16:26
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 29, 2023
@s-hertel
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 30, 2023
@s-hertel s-hertel requested a review from bcoca November 30, 2023 16:48
@bcoca bcoca merged commit 3a42a00 into ansible:devel Nov 30, 2023
68 checks passed
@s-hertel
Copy link
Contributor Author

@bcoca thanks for the reviews

s-hertel added a commit to s-hertel/ansible that referenced this pull request Nov 30, 2023
…ble#82165)

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 3a42a00)
s-hertel added a commit to s-hertel/ansible that referenced this pull request Nov 30, 2023
…ble#82165)

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 3a42a00)
s-hertel added a commit to s-hertel/ansible that referenced this pull request Nov 30, 2023
…ble#82165)

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 3a42a00)
s-hertel added a commit to s-hertel/ansible that referenced this pull request Dec 20, 2023
…ble#82165)

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 3a42a00)
s-hertel added a commit to s-hertel/ansible that referenced this pull request Dec 20, 2023
…ble#82165)

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 3a42a00)
@ansible ansible locked and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. has_issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: join() missing 1 required positional argument: 'a' in ansible-galaxy
4 participants