230 lines
9.8 KiB
Diff
230 lines
9.8 KiB
Diff
From 28f9fbdb5e281976e33f443193047068afb97a9b Mon Sep 17 00:00:00 2001
|
|
From: Brian Coca <bcoca@users.noreply.github.com>
|
|
Date: Fri, 3 Apr 2020 10:19:01 -0400
|
|
Subject: [PATCH] safely use vault to edit secrets (#68644)
|
|
|
|
* 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
|
|
---
|
|
changelogs/fragments/vault_tmp_race_fix.yml | 2 +
|
|
lib/ansible/parsing/vault/__init__.py | 119 +++++++++++++-------
|
|
2 files changed, 82 insertions(+), 39 deletions(-)
|
|
create mode 100644 changelogs/fragments/vault_tmp_race_fix.yml
|
|
|
|
diff --git a/changelogs/fragments/vault_tmp_race_fix.yml b/changelogs/fragments/vault_tmp_race_fix.yml
|
|
new file mode 100644
|
|
index 0000000..5807e17
|
|
--- /dev/null
|
|
+++ b/changelogs/fragments/vault_tmp_race_fix.yml
|
|
@@ -0,0 +1,2 @@
|
|
+bugfixes:
|
|
+ - "**security_issue** - create temporary vault file with strict permissions when editing and prevent race condition (CVE-2020-1740)"
|
|
diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py
|
|
index 7a68166..d52cf55 100644
|
|
--- a/lib/ansible/parsing/vault/__init__.py
|
|
+++ b/lib/ansible/parsing/vault/__init__.py
|
|
@@ -19,6 +19,8 @@
|
|
from __future__ import (absolute_import, division, print_function)
|
|
__metaclass__ = type
|
|
|
|
+import errno
|
|
+import fcntl
|
|
import os
|
|
import random
|
|
import shlex
|
|
@@ -27,6 +29,7 @@ import subprocess
|
|
import sys
|
|
import tempfile
|
|
import warnings
|
|
+
|
|
from binascii import hexlify
|
|
from binascii import unhexlify
|
|
from binascii import Error as BinasciiError
|
|
@@ -847,41 +850,47 @@ class VaultEditor:
|
|
|
|
os.remove(tmp_path)
|
|
|
|
- def _edit_file_helper(self, filename, secret,
|
|
- existing_data=None, force_save=False, vault_id=None):
|
|
+ def _edit_file_helper(self, filename, secret, existing_data=None, force_save=False, vault_id=None):
|
|
|
|
# Create a tempfile
|
|
root, ext = os.path.splitext(os.path.realpath(filename))
|
|
fd, tmp_path = tempfile.mkstemp(suffix=ext)
|
|
- os.close(fd)
|
|
|
|
try:
|
|
if existing_data:
|
|
- self.write_data(existing_data, tmp_path, shred=False)
|
|
+ self.write_data(existing_data, fd, shred=False)
|
|
+ except Exception:
|
|
+ # if an error happens, destroy the decrypted file
|
|
+ self._shred_file(tmp_path)
|
|
+ raise
|
|
+ finally:
|
|
+ os.close(fd)
|
|
|
|
+ try:
|
|
# drop the user into an editor on the tmp file
|
|
subprocess.call(self._editor_shell_command(tmp_path))
|
|
except:
|
|
- # whatever happens, destroy the decrypted file
|
|
+ # if an error happens, destroy the decrypted file
|
|
self._shred_file(tmp_path)
|
|
raise
|
|
|
|
b_tmpdata = self.read_data(tmp_path)
|
|
|
|
# Do nothing if the content has not changed
|
|
- if existing_data == b_tmpdata and not force_save:
|
|
- self._shred_file(tmp_path)
|
|
- return
|
|
+ if force_save or existing_data != b_tmpdata:
|
|
|
|
- # encrypt new data and write out to tmp
|
|
- # An existing vaultfile will always be UTF-8,
|
|
- # so decode to unicode here
|
|
- b_ciphertext = self.vault.encrypt(b_tmpdata, secret, vault_id=vault_id)
|
|
- self.write_data(b_ciphertext, tmp_path)
|
|
+ # encrypt new data and write out to tmp
|
|
+ # An existing vaultfile will always be UTF-8,
|
|
+ # so decode to unicode here
|
|
+ b_ciphertext = self.vault.encrypt(b_tmpdata, secret, vault_id=vault_id)
|
|
+ self.write_data(b_ciphertext, tmp_path)
|
|
|
|
- # shuffle tmp file into place
|
|
- self.shuffle_files(tmp_path, filename)
|
|
- display.vvvvv('Saved edited file "%s" encrypted using %s and vault id "%s"' % (filename, secret, vault_id))
|
|
+ # shuffle tmp file into place
|
|
+ self.shuffle_files(tmp_path, filename)
|
|
+ display.vvvvv(u'Saved edited file "%s" encrypted using %s and vault id "%s"' % (to_text(filename), to_text(secret), to_text(vault_id)))
|
|
+
|
|
+ # always shred temp, jic
|
|
+ self._shred_file(tmp_path)
|
|
|
|
def _real_path(self, filename):
|
|
# '-' is special to VaultEditor, dont expand it.
|
|
@@ -952,21 +961,17 @@ class VaultEditor:
|
|
|
|
# Figure out the vault id from the file, to select the right secret to re-encrypt it
|
|
# (duplicates parts of decrypt, but alas...)
|
|
- dummy, dummy, cipher_name, vault_id = parse_vaulttext_envelope(b_vaulttext,
|
|
- filename=filename)
|
|
+ dummy, dummy, cipher_name, vault_id = parse_vaulttext_envelope(b_vaulttext, filename=filename)
|
|
|
|
# vault id here may not be the vault id actually used for decrypting
|
|
# as when the edited file has no vault-id but is decrypted by non-default id in secrets
|
|
# (vault_id=default, while a different vault-id decrypted)
|
|
|
|
+ # we want to get rid of files encrypted with the AES cipher
|
|
+ force_save = (cipher_name not in CIPHER_WRITE_WHITELIST)
|
|
+
|
|
# Keep the same vault-id (and version) as in the header
|
|
- if cipher_name not in CIPHER_WRITE_WHITELIST:
|
|
- # we want to get rid of files encrypted with the AES cipher
|
|
- self._edit_file_helper(filename, vault_secret_used, existing_data=plaintext,
|
|
- force_save=True, vault_id=vault_id)
|
|
- else:
|
|
- self._edit_file_helper(filename, vault_secret_used, existing_data=plaintext,
|
|
- force_save=False, vault_id=vault_id)
|
|
+ self._edit_file_helper(filename, vault_secret_used, existing_data=plaintext, force_save=force_save, vault_id=vault_id)
|
|
|
|
def plaintext(self, filename):
|
|
|
|
@@ -1033,8 +1038,8 @@ class VaultEditor:
|
|
|
|
return data
|
|
|
|
- # TODO: add docstrings for arg types since this code is picky about that
|
|
- def write_data(self, data, filename, shred=True):
|
|
+ def write_data(self, data, thefile, shred=True, mode=0o600):
|
|
+ # TODO: add docstrings for arg types since this code is picky about that
|
|
"""Write the data bytes to given path
|
|
|
|
This is used to write a byte string to a file or stdout. It is used for
|
|
@@ -1051,28 +1056,64 @@ class VaultEditor:
|
|
should be a byte string and not a text type.
|
|
|
|
:arg data: the byte string (bytes) data
|
|
- :arg filename: filename to save 'data' to.
|
|
+ :arg thefile: file descriptor or filename to save 'data' to.
|
|
:arg shred: if shred==True, make sure that the original data is first shredded so that is cannot be recovered.
|
|
:returns: None
|
|
"""
|
|
# FIXME: do we need this now? data_bytes should always be a utf-8 byte string
|
|
b_file_data = to_bytes(data, errors='strict')
|
|
|
|
- # get a ref to either sys.stdout.buffer for py3 or plain old sys.stdout for py2
|
|
- # We need sys.stdout.buffer on py3 so we can write bytes to it since the plaintext
|
|
- # of the vaulted object could be anything/binary/etc
|
|
- output = getattr(sys.stdout, 'buffer', sys.stdout)
|
|
-
|
|
- if filename == '-':
|
|
+ # check if we have a file descriptor instead of a path
|
|
+ is_fd = False
|
|
+ try:
|
|
+ is_fd = (isinstance(thefile, int) and fcntl.fcntl(thefile, fcntl.F_GETFD) != -1)
|
|
+ except Exception:
|
|
+ pass
|
|
+
|
|
+ if is_fd:
|
|
+ # if passed descriptor, use that to ensure secure access, otherwise it is a string.
|
|
+ # assumes the fd is securely opened by caller (mkstemp)
|
|
+ os.ftruncate(thefile, 0)
|
|
+ os.write(thefile, b_file_data)
|
|
+ elif thefile == '-':
|
|
+ # get a ref to either sys.stdout.buffer for py3 or plain old sys.stdout for py2
|
|
+ # We need sys.stdout.buffer on py3 so we can write bytes to it since the plaintext
|
|
+ # of the vaulted object could be anything/binary/etc
|
|
+ output = getattr(sys.stdout, 'buffer', sys.stdout)
|
|
output.write(b_file_data)
|
|
else:
|
|
- if os.path.isfile(filename):
|
|
+ # file names are insecure and prone to race conditions, so remove and create securely
|
|
+ if os.path.isfile(thefile):
|
|
if shred:
|
|
- self._shred_file(filename)
|
|
+ self._shred_file(thefile)
|
|
else:
|
|
- os.remove(filename)
|
|
- with open(filename, "wb") as fh:
|
|
- fh.write(b_file_data)
|
|
+ os.remove(thefile)
|
|
+
|
|
+ # when setting new umask, we get previous as return
|
|
+ current_umask = os.umask(0o077)
|
|
+ try:
|
|
+ try:
|
|
+ # create file with secure permissions
|
|
+ fd = os.open(thefile, os.O_CREAT | os.O_EXCL | os.O_RDWR | os.O_TRUNC, mode)
|
|
+ except OSError as ose:
|
|
+ # Want to catch FileExistsError, which doesn't exist in Python 2, so catch OSError
|
|
+ # and compare the error number to get equivalent behavior in Python 2/3
|
|
+ if ose.errno == errno.EEXIST:
|
|
+ raise AnsibleError('Vault file got recreated while we were operating on it: %s' % to_native(ose))
|
|
+
|
|
+ raise AnsibleError('Problem creating temporary vault file: %s' % to_native(ose))
|
|
+
|
|
+ try:
|
|
+ # now write to the file and ensure ours is only data in it
|
|
+ os.ftruncate(fd, 0)
|
|
+ os.write(fd, b_file_data)
|
|
+ except OSError as e:
|
|
+ raise AnsibleError('Unable to write to temporary vault file: %s' % to_native(e))
|
|
+ finally:
|
|
+ # Make sure the file descriptor is always closed and reset umask
|
|
+ os.close(fd)
|
|
+ finally:
|
|
+ os.umask(current_umask)
|
|
|
|
def shuffle_files(self, src, dest):
|
|
prev = None
|
|
--
|
|
2.27.0
|
|
|