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

fetch module path traversal #67793

Closed
samdoran opened this issue Feb 26, 2020 · 2 comments · Fixed by #68720
Closed

fetch module path traversal #67793

samdoran opened this issue Feb 26, 2020 · 2 comments · Fixed by #68720
Assignees
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. files Files category has_pr This issue has an associated PR. module This issue/PR relates to a module. security Related to a vulnerability or CVE support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@samdoran
Copy link
Contributor

SUMMARY

CVE-2020-1735

Possibly related to CVE-2019-3828 (#52133)

The fetch module takes the source result from the slurp module, which came from the remote host. We don't really validate this path and it could have been manipulated by the remote host in a malicious way such that we end up a path similar to /tmp/result_fetch/ansible1/../../../../../../../../../../../../../../../../home/<user>/.profile for the source. This allows an attacker to place a file the contents of which they control.

Relevant Code:

slurpres = self._execute_module(module_name='slurp', module_args=dict(src=source), task_vars=task_vars)
if slurpres.get('failed'):
if not fail_on_missing and (slurpres.get('msg').startswith('file not found') or remote_checksum == '1'):
result['msg'] = "the remote file does not exist, not transferring, ignored"
result['file'] = source
result['changed'] = False
else:
result.update(slurpres)
return result
else:
if slurpres['encoding'] == 'base64':
remote_data = base64.b64decode(slurpres['content'])
if remote_data is not None:
remote_checksum = checksum_s(remote_data)
# the source path may have been expanded on the
# target system, so we compare it here and use the
# expanded version if it's different
remote_source = slurpres.get('source')
if remote_source and remote_source != source:
source = remote_source

Suggested correction from the reporter:

  • Don't use %s/%s/%s to compute the destination file or clean the last argument
  • add the following check:
target_dir = os.path.normpath(os.path.join(self._loader.path_dwim(dest), target_name))
dest = os.path.normpath(os.path.join(target_dir, source_local)
assert os.path.commonpath([target_dir, dest]) == target_dir
ISSUE TYPE
  • Bug Report
COMPONENT NAME

lib/ansible/plugins/action/fetch.py

ANSIBLE VERSION
2.10
CONFIGURATION
default
OS / ENVIRONMENT
STEPS TO REPRODUCE
EXPECTED RESULTS
ACTUAL RESULTS

@samdoran samdoran added the security Related to a vulnerability or CVE label Feb 26, 2020
@samdoran samdoran changed the title Fetch module path traversal fetch module path traversal Feb 26, 2020
@samdoran samdoran self-assigned this Feb 26, 2020
@samdoran
Copy link
Contributor Author

os.path.commonpath() was added in Python 3.4 but is not available in Python 2.7, so we cannot use that. os.path.commonprefix() is available in Python 2.7 and should work, though.

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. files Files category module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 26, 2020
@bcoca
Copy link
Member

bcoca commented Apr 1, 2020

we can just remove those lines since we already do remote-expand previously, there is no need to use the return from slurp.

@ansibot ansibot added the has_pr This issue has an associated PR. label Apr 1, 2020
@bcoca bcoca assigned bcoca and unassigned samdoran Apr 3, 2020
bcoca added a commit to bcoca/ansible that referenced this issue Apr 6, 2020
  * ignore slurp result
  * also fixed naming when source is relative
  * also fixed bug in local connection plugin
  * add tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action exceps
  * moved dest transform down to when needed

fixes ansible#67793

CVE-2019-3828
bcoca added a commit that referenced this issue Apr 8, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes #67793

CVE-2019-3828
bcoca added a commit to bcoca/ansible that referenced this issue Apr 8, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes ansible#67793

CVE-2019-3828

(cherry picked from commit ba87c22)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 8, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes ansible#67793

CVE-2019-3828

(cherry picked from commit ba87c22)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 8, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes ansible#67793

CVE-2019-3828

(cherry picked from commit ba87c22)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 9, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes ansible#67793

CVE-2019-3828

(cherry picked from commit ba87c22)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 15, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check

fixes ansible#67793

CVE-2019-3828

(cherry picked from commit ba87c22)
mattclay pushed a commit that referenced this issue Apr 15, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes #67793

CVE-2019-3828

(cherry picked from commit ba87c22)
mattclay pushed a commit that referenced this issue Apr 15, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check

fixes #67793

CVE-2019-3828

(cherry picked from commit ba87c22)
mattclay pushed a commit that referenced this issue Apr 15, 2020
* fixed fetch traversal from slurp

  * ignore slurp result for dest
  * fixed naming when source is relative
  * fixed bug in local connection plugin
  * added tests with fake slurp
  * moved existing role tests into runme.sh
  * normalized on action excepts
  * moved dest transform down to when needed
  * added is_subpath check
  * fixed bug in local connection

fixes #67793

CVE-2019-3828

(cherry picked from commit ba87c22)
relrod added a commit to relrod/ansible that referenced this issue Apr 18, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs ansible#67793, ansible#68720

Signed-off-by: Rick Elrod <[email protected]>
relrod added a commit to relrod/ansible that referenced this issue Apr 18, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs ansible#67793, ansible#68720

Signed-off-by: Rick Elrod <[email protected]>
relrod added a commit to relrod/ansible that referenced this issue Apr 18, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs ansible#67793, ansible#68720

Signed-off-by: Rick Elrod <[email protected]>
relrod added a commit to relrod/ansible that referenced this issue Apr 18, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs ansible#67793, ansible#68720

Signed-off-by: Rick Elrod <[email protected]>
mattclay pushed a commit that referenced this issue Apr 21, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs #67793, #68720

Signed-off-by: Rick Elrod <[email protected]>
mattclay pushed a commit that referenced this issue Apr 21, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs #67793, #68720

Signed-off-by: Rick Elrod <[email protected]>
mattclay pushed a commit that referenced this issue Apr 21, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs #67793, #68720

Signed-off-by: Rick Elrod <[email protected]>
mattclay pushed a commit that referenced this issue Apr 21, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs #67793, #68720

Signed-off-by: Rick Elrod <[email protected]>
bcoca pushed a commit to bcoca/ansible that referenced this issue Apr 22, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs ansible#67793, ansible#68720

Signed-off-by: Rick Elrod <[email protected]>
bcoca pushed a commit to bcoca/ansible that referenced this issue Apr 28, 2020
Change:
This corrects an incorrect CVE identifier in the changelog entry for
CVE-2020-1735.

Test Plan:
N/A

Tickets:
Refs ansible#67793, ansible#68720

Signed-off-by: Rick Elrod <[email protected]>
@ansible ansible locked and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. files Files category has_pr This issue has an associated PR. module This issue/PR relates to a module. security Related to a vulnerability or CVE support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
3 participants