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

ansible-vault edit race condition #67798

Closed
samdoran opened this issue Feb 26, 2020 · 2 comments · Fixed by #68644 or sthagen/ansible-ansible#238
Closed

ansible-vault edit race condition #67798

samdoran opened this issue Feb 26, 2020 · 2 comments · Fixed by #68644 or sthagen/ansible-ansible#238
Assignees
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. has_pr This issue has an associated PR. 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-1740

A race condition exists in ansible-vault edit which could allow another user on the same computer can read the old and new secret.

When executing ansible-vault edit, the method VaultEditor._edit_file_helper() creates the temporary file with tempfile.mkstemp(). However, the returned file descriptor is closed and VaultEditor.write_data() is called to write to the file. VaultEditor.write_data() will delete the file and recreate it. A malicious user can create the file with permissions allowing them access to the file after the unlink.

The proposed solution is to write directly to the file descriptor in VaultEditor._edit_file_helper() rather than deleting and creating a new file.

Relevant code:

# Create a tempfile
root, ext = os.path.splitext(os.path.realpath(filename))
fd, tmp_path = tempfile.mkstemp(suffix=ext)
os.close(fd)
cmd = self._editor_shell_command(tmp_path)
try:
if existing_data:
self.write_data(existing_data, tmp_path, shred=False)

ISSUE TYPE
  • Bug Report
COMPONENT NAME

lib/ansible/parsing/vault/__init__.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 self-assigned this Feb 26, 2020
@ansibot
Copy link
Contributor

ansibot commented Feb 26, 2020

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 26, 2020
@samdoran samdoran added the has_pr This issue has an associated PR. label Feb 26, 2020
@bcoca
Copy link
Member

bcoca commented Apr 1, 2020

edit uses mkstemp, which creates a directory already only readable by the user, avoiding the permissions issue. But we have a problem because instead of keeping the filehandle and using that, we close it and open ourselves up to the race condition.

We should just pass the filehandle and ensure secure access by using that instead of the generated path string. Creating the file with stricter permissions still does not matter as controlling the directory will trump that.

We get some mitigation from 6452a82 , as this forces usage of LOCAL_TEMP which is normally safer than system temp, but it is not a guarantee as the user can configure it in insecure ways.

bcoca added a commit to bcoca/ansible that referenced this issue Apr 2, 2020
  * when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
bcoca added a commit that referenced this issue Apr 3, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes #67798

Co-authored-by: samdoran
bcoca added a commit to bcoca/ansible that referenced this issue Apr 3, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 3, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 3, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 15, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 15, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
bcoca added a commit to bcoca/ansible that referenced this issue Apr 15, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes ansible#67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
mattclay pushed a commit that referenced this issue Apr 15, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes #67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
mattclay pushed a commit that referenced this issue Apr 15, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes #67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
mattclay pushed a commit that referenced this issue Apr 15, 2020
* when possible, use filedescriptors from mkstemp to avoid race
  * when using path strings, ensure we are always creating the file

CVE-2020-1740
Fixes #67798

Co-authored-by: samdoran
(cherry picked from commit 28f9fbd)
@ansible ansible locked and limited conversation to collaborators May 1, 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. has_pr This issue has an associated PR. 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