From 946b82bef71d3b2d4ecf07ec937b650634bc84a0 Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Wed, 30 Dec 2015 18:21:34 +0100 Subject: [PATCH 1/5] shred ansible-vault tmp_file. Also when editor is interruped. --- lib/ansible/parsing/vault/__init__.py | 35 ++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index d8cf66feca..b7304d156f 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -219,7 +219,27 @@ class VaultEditor: def __init__(self, password): self.vault = VaultLib(password) - + + def _shred_file(self, tmp_path): + """securely destroy a decrypted file.""" + def generate_data(length): + import string, random + chars = string.ascii_lowercase + string.ascii_uppercase + string.digits + return ''.join(random.SystemRandom().choice(chars) for _ in range(length)) + + if not os.path.isfile(tmp_path): + # file is already gone + return + + ld = os.path.getsize(tmp_path) + passes = 3 + with open(tmp_path, "w") as fh: + for _ in range(int(passes)): + data = generate_data(ld) + fh.write(data) + fh.seek(0, 0) + os.remove(tmp_path) + def _edit_file_helper(self, filename, existing_data=None, force_save=False): # Create a tempfile @@ -229,12 +249,18 @@ class VaultEditor: self.write_data(existing_data, tmp_path) # drop the user into an editor on the tmp file - call(self._editor_shell_command(tmp_path)) + try: + call(self._editor_shell_command(tmp_path)) + except: + # whatever happens, destroy the decrypted file + self._shred_file(tmp_path) + raise + tmpdata = self.read_data(tmp_path) # Do nothing if the content has not changed if existing_data == tmpdata and not force_save: - os.remove(tmp_path) + self._shred_file(tmp_path) return # encrypt new data and write out to tmp @@ -329,7 +355,7 @@ class VaultEditor: sys.stdout.write(bytes) else: if os.path.isfile(filename): - os.remove(filename) + self._shred_file(filename) with open(filename, "wb") as fh: fh.write(bytes) @@ -338,6 +364,7 @@ class VaultEditor: # overwrite dest with src if os.path.isfile(dest): prev = os.stat(dest) + # old file 'dest' was encrypted, no need to _shred_file os.remove(dest) shutil.move(src, dest) From 7193d27acc7719b25b70eb4709964d0c93796162 Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Mon, 4 Jan 2016 17:19:35 +0100 Subject: [PATCH 2/5] add os.fsync() so that the shredding data (hopefully) hits the drive --- lib/ansible/parsing/vault/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index b7304d156f..1eca0cd571 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -235,9 +235,10 @@ class VaultEditor: passes = 3 with open(tmp_path, "w") as fh: for _ in range(int(passes)): + fh.seek(0, 0) data = generate_data(ld) fh.write(data) - fh.seek(0, 0) + os.fsync(fh) os.remove(tmp_path) def _edit_file_helper(self, filename, existing_data=None, force_save=False): From 1e911375e850e79295d053f3e3c45c9d9d247159 Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Mon, 4 Jan 2016 18:13:59 +0100 Subject: [PATCH 3/5] add docs, remove unnecessary int() cast --- lib/ansible/parsing/vault/__init__.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index 1eca0cd571..28e819860a 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -221,7 +221,22 @@ class VaultEditor: self.vault = VaultLib(password) def _shred_file(self, tmp_path): - """securely destroy a decrypted file.""" + """Securely destroy a decrypted file + + Inspired by unix `shred', try to destroy the secrets "so that they can be + recovered only with great difficulty with specialised hardware, if at all". + + See https://github.com/ansible/ansible/pull/13700 . + + Note that: + - For flash: overwriting would have no effect (due to wear leveling). But the + added disk wear is considered insignificant. + - For other storage systems: the filesystem lies to the vfs (kernel), the disk + driver lies to the filesystem and the disk lies to the driver. But it's better + than nothing. + - most tmp dirs are now tmpfs (ramdisks), for which this is a non-issue. + """ + def generate_data(length): import string, random chars = string.ascii_lowercase + string.ascii_uppercase + string.digits @@ -234,7 +249,7 @@ class VaultEditor: ld = os.path.getsize(tmp_path) passes = 3 with open(tmp_path, "w") as fh: - for _ in range(int(passes)): + for _ in range(passes): fh.seek(0, 0) data = generate_data(ld) fh.write(data) From 151e09d129d63ce485d42d3f6cf0915bb8bd8cee Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Tue, 5 Jan 2016 01:34:45 +0100 Subject: [PATCH 4/5] use unix shred if possible, otherwise fast custom impl; do not shred encrypted file --- lib/ansible/parsing/vault/__init__.py | 90 ++++++++++++++++++--------- 1 file changed, 62 insertions(+), 28 deletions(-) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index 28e819860a..bcd038c8b8 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -219,41 +219,67 @@ class VaultEditor: def __init__(self, password): self.vault = VaultLib(password) + + def _shred_file_custom(self, tmp_path): + """"Destroy a file, when shred (core-utils) is not available - def _shred_file(self, tmp_path): - """Securely destroy a decrypted file + Unix `shred' destroys files "so that they can be recovered only with great difficulty with + specialised hardware, if at all". It is based on the method from the paper + "Secure Deletion of Data from Magnetic and Solid-State Memory", + Proceedings of the Sixth USENIX Security Symposium (San Jose, California, July 22-25, 1996). - Inspired by unix `shred', try to destroy the secrets "so that they can be - recovered only with great difficulty with specialised hardware, if at all". + We do not go to that length to re-implement shred in Python; instead, overwriting with a block + of random data should suffice. See https://github.com/ansible/ansible/pull/13700 . - - Note that: - - For flash: overwriting would have no effect (due to wear leveling). But the - added disk wear is considered insignificant. - - For other storage systems: the filesystem lies to the vfs (kernel), the disk - driver lies to the filesystem and the disk lies to the driver. But it's better - than nothing. - - most tmp dirs are now tmpfs (ramdisks), for which this is a non-issue. """ - def generate_data(length): - import string, random - chars = string.ascii_lowercase + string.ascii_uppercase + string.digits - return ''.join(random.SystemRandom().choice(chars) for _ in range(length)) + file_len = os.path.getsize(tmp_path) + + passes = 3 + with open(tmp_path, "wb") as fh: + for _ in range(passes): + fh.seek(0, 0) + # get a random chunk of data + data = os.urandom(min(1024*1024*2, file_len)) + bytes_todo = file_len + while bytes_todo > 0: + chunk = data[:bytes_todo] + fh.write(chunk) + bytes_todo -= len(chunk) + + assert(fh.tell() == file_len) + os.fsync(fh) + + + def _shred_file(self, tmp_path): + """Securely destroy a decrypted file + + Note standard limitations of GNU shred apply (For flash, overwriting would have no effect + due to wear leveling; for other storage systems, the async kernel->filesystem->disk calls never + guarantee data hits the disk; etc). Furthermore, if your tmp dirs is on tmpfs (ramdisks), + it is a non-issue. + + Nevertheless, some form of overwriting the data (instead of just removing the fs index entry) is + a good idea. If shred is not available (e.g. on windows, or no core-utils installed), fall back on + a custom shredding method. + """ if not os.path.isfile(tmp_path): # file is already gone return + + try: + r = call(['shred', tmp_path]) + except OSError as e: + # shred is not available on this system, or some other error occured. + self._shred_file_custom(tmp_path) + r = 0 + + if r != 0: + # we could not successfully execute unix shred; therefore, do custom shred. + self._shred_file_custom(tmp_path) - ld = os.path.getsize(tmp_path) - passes = 3 - with open(tmp_path, "w") as fh: - for _ in range(passes): - fh.seek(0, 0) - data = generate_data(ld) - fh.write(data) - os.fsync(fh) os.remove(tmp_path) def _edit_file_helper(self, filename, existing_data=None, force_save=False): @@ -262,7 +288,7 @@ class VaultEditor: _, tmp_path = tempfile.mkstemp() if existing_data: - self.write_data(existing_data, tmp_path) + self.write_data(existing_data, tmp_path, shred=False) # drop the user into an editor on the tmp file try: @@ -300,7 +326,7 @@ class VaultEditor: ciphertext = self.read_data(filename) plaintext = self.vault.decrypt(ciphertext) - self.write_data(plaintext, output_file or filename) + self.write_data(plaintext, output_file or filename, shred=False) def create_file(self, filename): """ create a new encrypted file """ @@ -365,13 +391,21 @@ class VaultEditor: return data - def write_data(self, data, filename): + def write_data(self, data, filename, shred=True): + """write data to given path + + if shred==True, make sure that the original data is first shredded so + that is cannot be recovered + """ bytes = to_bytes(data, errors='strict') if filename == '-': sys.stdout.write(bytes) else: if os.path.isfile(filename): - self._shred_file(filename) + if shred: + self._shred_file(filename) + else: + os.remove(filename) with open(filename, "wb") as fh: fh.write(bytes) From 11ce08b9dde32c7e4b51a6fffc22f301c81181be Mon Sep 17 00:00:00 2001 From: Eric Feliksik Date: Tue, 5 Jan 2016 18:04:38 +0100 Subject: [PATCH 5/5] cleaner implementation and random chunk length. --- lib/ansible/parsing/vault/__init__.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index bcd038c8b8..1d4eeef465 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -22,6 +22,7 @@ import shlex import shutil import sys import tempfile +import random from io import BytesIO from subprocess import call from ansible.errors import AnsibleError @@ -235,20 +236,21 @@ class VaultEditor: """ file_len = os.path.getsize(tmp_path) + max_chunk_len = min(1024*1024*2, file_len) passes = 3 with open(tmp_path, "wb") as fh: for _ in range(passes): fh.seek(0, 0) - # get a random chunk of data - data = os.urandom(min(1024*1024*2, file_len)) - bytes_todo = file_len - while bytes_todo > 0: - chunk = data[:bytes_todo] - fh.write(chunk) - bytes_todo -= len(chunk) - - assert(fh.tell() == file_len) + # get a random chunk of data, each pass with other length + chunk_len = random.randint(max_chunk_len/2, max_chunk_len) + data = os.urandom(chunk_len) + + for _ in range(0, file_len // chunk_len): + fh.write(data) + fh.write(data[:file_len % chunk_len]) + + assert(fh.tell() == file_len) # FIXME remove this assert once we have unittests to check its accuracy os.fsync(fh) @@ -273,13 +275,12 @@ class VaultEditor: r = call(['shred', tmp_path]) except OSError as e: # shred is not available on this system, or some other error occured. - self._shred_file_custom(tmp_path) - r = 0 + r = 1 if r != 0: # we could not successfully execute unix shred; therefore, do custom shred. self._shred_file_custom(tmp_path) - + os.remove(tmp_path) def _edit_file_helper(self, filename, existing_data=None, force_save=False):