From 29bdd0b326af1a52c52a97889af8161bfb9647f9 Mon Sep 17 00:00:00 2001 From: Adrian Likins Date: Fri, 10 Nov 2017 14:31:32 -0500 Subject: [PATCH] Better handling of malformed vault data envelope (#32515) If an embedded vaulted variable ('!vault' in yaml) had an invalid format, it would eventually cause an error for seemingly unrelated reasons. "Invalid" meaning not valid hexlify (extra chars, non-hex chars, etc). For ex, if a host_vars file had invalid vault format variables, on py2, it would cause an error like: 'ansible.vars.hostvars.HostVars object' has no attribute u'broken.example.com' Depending on where the invalid vault is, it could also cause "VARIABLE IS NOT DEFINED!". The behavior can also change if ansible-playbook is py2 or py3. Root cause is errors from binascii.unhexlify() not being handled consistently. Fix is to add a AnsibleVaultFormatError exception and raise it on any unhexlify() errors and to handle it properly elsewhere. Add a _unhexlify() that try/excepts around a binascii.unhexlify() and raises an AnsibleVaultFormatError on invalid vault data. This is so the same exception type is always raised for this case. Previous it was different between py2 and py3. binascii.unhexlify() raises a binascii.Error if the hexlified blobs in a vault data blob are invalid. On py2, binascii.Error is a subclass of Exception. On py3, binascii.Error is a subclass of TypeError When decrypting content of vault encrypted variables, if a binascii.Error is raised it propagates up to playbook.base.Base.post_validate(). post_validate() handles exceptions for TypeErrors but not for base Exception subclasses (like py2 binascii.Error). * Add a display.warning on vault format errors * Unit tests for _unhexlify, parse_vaulttext* * Add intg test cases for invalid vault formats Fixes #28038 (cherry picked from commit 9c588274100905fb0ca60e534fb3a8861173f009) --- CHANGELOG.md | 2 + lib/ansible/parsing/vault/__init__.py | 111 ++++++++++++++---- .../targets/vault/invalid_format/README.md | 1 + .../broken-group-vars-tasks.yml | 23 ++++ .../invalid_format/broken-host-vars-tasks.yml | 7 ++ .../group_vars/broken-group-vars.yml | 8 ++ .../broken-host-vars.example.com/vars | 11 ++ .../targets/vault/invalid_format/inventory | 5 + .../invalid_format/original-broken-host-vars | 6 + .../invalid_format/original-group-vars.yml | 2 + .../targets/vault/invalid_format/some-vars | 6 + .../targets/vault/invalid_format/vault-secret | 1 + test/integration/targets/vault/runme.sh | 7 ++ test/units/parsing/vault/test_vault.py | 56 +++++++++ 14 files changed, 222 insertions(+), 24 deletions(-) create mode 100644 test/integration/targets/vault/invalid_format/README.md create mode 100644 test/integration/targets/vault/invalid_format/broken-group-vars-tasks.yml create mode 100644 test/integration/targets/vault/invalid_format/broken-host-vars-tasks.yml create mode 100644 test/integration/targets/vault/invalid_format/group_vars/broken-group-vars.yml create mode 100644 test/integration/targets/vault/invalid_format/host_vars/broken-host-vars.example.com/vars create mode 100644 test/integration/targets/vault/invalid_format/inventory create mode 100644 test/integration/targets/vault/invalid_format/original-broken-host-vars create mode 100644 test/integration/targets/vault/invalid_format/original-group-vars.yml create mode 100644 test/integration/targets/vault/invalid_format/some-vars create mode 100644 test/integration/targets/vault/invalid_format/vault-secret diff --git a/CHANGELOG.md b/CHANGELOG.md index dd8651cd29..a28b52868b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,8 @@ Ansible Changes By Release (https://github.com/ansible/ansible/pull/32744) * Prevent host_group_vars plugin load errors when using 'path as inventory hostname' https://github.com/ansible/ansible/issues/32764 +* Better errors when loading malformed vault envelopes + (https://github.com/ansible/ansible/issues/28038) diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py index ef08e602fb..4ef5d062bb 100644 --- a/lib/ansible/parsing/vault/__init__.py +++ b/lib/ansible/parsing/vault/__init__.py @@ -29,6 +29,7 @@ import tempfile import warnings from binascii import hexlify from binascii import unhexlify +from binascii import Error as BinasciiError from hashlib import md5 from hashlib import sha256 from io import BytesIO @@ -105,6 +106,10 @@ class AnsibleVaultPasswordError(AnsibleVaultError): pass +class AnsibleVaultFormatError(AnsibleError): + pass + + def is_encrypted(data): """ Test if this is vault encrypted data blob @@ -148,20 +153,7 @@ def is_encrypted_file(file_obj, start_pos=0, count=-1): file_obj.seek(current_position) -def parse_vaulttext_envelope(b_vaulttext_envelope, default_vault_id=None): - """Retrieve information about the Vault and clean the data - - When data is saved, it has a header prepended and is formatted into 80 - character lines. This method extracts the information from the header - and then removes the header and the inserted newlines. The string returned - is suitable for processing by the Cipher classes. - - :arg b_vaulttext: byte str containing the data from a save file - :returns: a byte str suitable for passing to a Cipher class's - decrypt() function. - """ - # used by decrypt - default_vault_id = default_vault_id or C.DEFAULT_VAULT_IDENTITY +def _parse_vaulttext_envelope(b_vaulttext_envelope, default_vault_id=None): b_tmpdata = b_vaulttext_envelope.splitlines() b_tmpheader = b_tmpdata[0].strip().split(b';') @@ -169,7 +161,6 @@ def parse_vaulttext_envelope(b_vaulttext_envelope, default_vault_id=None): b_version = b_tmpheader[1].strip() cipher_name = to_text(b_tmpheader[2].strip()) vault_id = default_vault_id - # vault_id = None # Only attempt to find vault_id if the vault file is version 1.2 or newer # if self.b_version == b'1.2': @@ -181,6 +172,37 @@ def parse_vaulttext_envelope(b_vaulttext_envelope, default_vault_id=None): return b_ciphertext, b_version, cipher_name, vault_id +def parse_vaulttext_envelope(b_vaulttext_envelope, default_vault_id=None, filename=None): + """Parse the vaulttext envelope + + When data is saved, it has a header prepended and is formatted into 80 + character lines. This method extracts the information from the header + and then removes the header and the inserted newlines. The string returned + is suitable for processing by the Cipher classes. + + :arg b_vaulttext: byte str containing the data from a save file + :kwarg default_vault_id: The vault_id name to use if the vaulttext does not provide one. + :kwarg filename: The filename that the data came from. This is only + used to make better error messages in case the data cannot be + decrypted. This is optional. + :returns: A tuple of byte str of the vaulttext suitable to pass to parse_vaultext, + a byte str of the vault format version, + the name of the cipher used, and the vault_id. + :raises: AnsibleVaultFormatError: if the vaulttext_envelope format is invalid + """ + # used by decrypt + default_vault_id = default_vault_id or C.DEFAULT_VAULT_IDENTITY + + try: + return _parse_vaulttext_envelope(b_vaulttext_envelope, default_vault_id) + except Exception as exc: + msg = "Vault envelope format error" + if filename: + msg += ' in %s' % (filename) + msg += ': %s' % exc + raise AnsibleVaultFormatError(msg) + + def format_vaulttext_envelope(b_ciphertext, cipher_name, version=None, vault_id=None): """ Add header and format to 80 columns @@ -222,6 +244,41 @@ def format_vaulttext_envelope(b_ciphertext, cipher_name, version=None, vault_id= return b_vaulttext +def _unhexlify(b_data): + try: + return unhexlify(b_data) + except (BinasciiError, TypeError) as exc: + raise AnsibleVaultFormatError('Vault format unhexlify error: %s' % exc) + + +def _parse_vaulttext(b_vaulttext): + b_vaulttext = _unhexlify(b_vaulttext) + b_salt, b_crypted_hmac, b_ciphertext = b_vaulttext.split(b"\n", 2) + b_salt = _unhexlify(b_salt) + b_ciphertext = _unhexlify(b_ciphertext) + + return b_ciphertext, b_salt, b_crypted_hmac + + +def parse_vaulttext(b_vaulttext): + """Parse the vaulttext + + :arg b_vaulttext: byte str containing the vaulttext (ciphertext, salt, crypted_hmac) + :returns: A tuple of byte str of the ciphertext suitable for passing to a + Cipher class's decrypt() function, a byte str of the salt, + and a byte str of the crypted_hmac + :raises: AnsibleVaultFormatError: if the vaulttext format is invalid + """ + # SPLIT SALT, DIGEST, AND DATA + try: + return _parse_vaulttext(b_vaulttext) + except AnsibleVaultFormatError: + raise + except Exception as exc: + msg = "Vault vaulttext format error: %s" % exc + raise AnsibleVaultFormatError(msg) + + def verify_secret_is_not_empty(secret, msg=None): '''Check the secret against minimal requirements. @@ -526,7 +583,8 @@ class VaultLib: msg += "%s is not a vault encrypted file" % filename raise AnsibleError(msg) - b_vaulttext, dummy, cipher_name, vault_id = parse_vaulttext_envelope(b_vaulttext) + b_vaulttext, dummy, cipher_name, vault_id = parse_vaulttext_envelope(b_vaulttext, + filename=filename) # create the cipher object, note that the cipher used for decrypt can # be different than the cipher used for encrypt @@ -582,6 +640,13 @@ class VaultLib: vault_id_used = vault_secret_id display.vvvvv('decrypt succesful with secret=%s and vault_id=%s' % (vault_secret, vault_secret_id)) break + except AnsibleVaultFormatError as exc: + msg = "There was a vault format error" + if filename: + msg += ' in %s' % (filename) + msg += ': %s' % exc + display.warning(msg) + raise except AnsibleError as e: display.vvvv('Tried to use the vault secret (%s) to decrypt (%s) but it failed. Error: %s' % (vault_secret_id, filename, e)) @@ -779,7 +844,8 @@ 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) + 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 @@ -1053,7 +1119,7 @@ class VaultAES: 'switch to the newer VaultAES256 format', version='2.3') # http://stackoverflow.com/a/14989032 - b_vaultdata = unhexlify(b_vaulttext) + b_vaultdata = _unhexlify(b_vaulttext) b_salt = b_vaultdata[len(b'Salted__'):16] b_ciphertext = b_vaultdata[16:] @@ -1206,7 +1272,7 @@ class VaultAES256: hmac = HMAC(b_key2, hashes.SHA256(), CRYPTOGRAPHY_BACKEND) hmac.update(b_ciphertext) try: - hmac.verify(unhexlify(b_crypted_hmac)) + hmac.verify(_unhexlify(b_crypted_hmac)) except InvalidSignature as e: raise AnsibleVaultError('HMAC verification failed: %s' % e) @@ -1268,11 +1334,8 @@ class VaultAES256: @classmethod def decrypt(cls, b_vaulttext, secret): - # SPLIT SALT, DIGEST, AND DATA - b_vaulttext = unhexlify(b_vaulttext) - b_salt, b_crypted_hmac, b_ciphertext = b_vaulttext.split(b"\n", 2) - b_salt = unhexlify(b_salt) - b_ciphertext = unhexlify(b_ciphertext) + + b_ciphertext, b_salt, b_crypted_hmac = parse_vaulttext(b_vaulttext) # TODO: would be nice if a VaultSecret could be passed directly to _decrypt_* # (move _gen_key_initctr() to a AES256 VaultSecret or VaultContext impl?) diff --git a/test/integration/targets/vault/invalid_format/README.md b/test/integration/targets/vault/invalid_format/README.md new file mode 100644 index 0000000000..cbbc07a965 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/README.md @@ -0,0 +1 @@ +Based on https://github.com/yves-vogl/ansible-inline-vault-issue diff --git a/test/integration/targets/vault/invalid_format/broken-group-vars-tasks.yml b/test/integration/targets/vault/invalid_format/broken-group-vars-tasks.yml new file mode 100644 index 0000000000..71dbacc0a3 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/broken-group-vars-tasks.yml @@ -0,0 +1,23 @@ +--- +- hosts: broken-group-vars + gather_facts: false + tasks: + - name: EXPECTED FAILURE + debug: + msg: "some_var_that_fails: {{ some_var_that_fails }}" + + - name: EXPECTED FAILURE Display hostvars + debug: + msg: "{{inventory_hostname}} hostvars: {{ hostvars[inventory_hostname] }}" + + +# ansible-vault --vault-password-file=vault-secret encrypt_string test +# !vault | +# $ANSIBLE_VAULT;1.1;AES256 +# 64323332393930623633306662363165386332376638653035356132646165663632616263653366 +# 6233383362313531623238613461323861376137656265380a366464663835633065616361636231 +# 39653230653538366165623664326661653135306132313730393232343432333635326536373935 +# 3366323866663763660a323766383531396433663861656532373663373134376263383263316261 +# 3137 + +# $ ansible-playbook -i inventory --vault-password-file=vault-secret tasks.yml diff --git a/test/integration/targets/vault/invalid_format/broken-host-vars-tasks.yml b/test/integration/targets/vault/invalid_format/broken-host-vars-tasks.yml new file mode 100644 index 0000000000..9afbd58e14 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/broken-host-vars-tasks.yml @@ -0,0 +1,7 @@ +--- +- hosts: broken-host-vars + gather_facts: false + tasks: + - name: EXPECTED FAILURE Display hostvars + debug: + msg: "{{inventory_hostname}} hostvars: {{ hostvars[inventory_hostname] }}" diff --git a/test/integration/targets/vault/invalid_format/group_vars/broken-group-vars.yml b/test/integration/targets/vault/invalid_format/group_vars/broken-group-vars.yml new file mode 100644 index 0000000000..5f4774310d --- /dev/null +++ b/test/integration/targets/vault/invalid_format/group_vars/broken-group-vars.yml @@ -0,0 +1,8 @@ +$ANSIBLE_VAULT;1.1;AES256 +64306566356165343030353932383461376334336665626135343932356431383134306338353664 +6435326361306561633165633536333234306665346437330a366265346466626464396264393262 +34616366626565336637653032336465363165363334356535353833393332313239353736623237 +6434373738633039650a353435303366323139356234616433613663626334643939303361303764 +3636363333333333333333333 +36313937643431303637353931366363643661396238303530323262326334343432383637633439 +6365373237336535353661356430313965656538363436333836 diff --git a/test/integration/targets/vault/invalid_format/host_vars/broken-host-vars.example.com/vars b/test/integration/targets/vault/invalid_format/host_vars/broken-host-vars.example.com/vars new file mode 100644 index 0000000000..2d309eb5e8 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/host_vars/broken-host-vars.example.com/vars @@ -0,0 +1,11 @@ +--- +example_vars: + some_key: + another_key: some_value + bad_vault_dict_key: !vault | + $ANSIBLE_VAULT;1.1;AES256 + 64323332393930623633306662363165386332376638653035356132646165663632616263653366 + 623338xyz2313531623238613461323861376137656265380a366464663835633065616361636231 + 3366323866663763660a323766383531396433663861656532373663373134376263383263316261 + 3137 + diff --git a/test/integration/targets/vault/invalid_format/inventory b/test/integration/targets/vault/invalid_format/inventory new file mode 100644 index 0000000000..e6e259a406 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/inventory @@ -0,0 +1,5 @@ +[broken-group-vars] +broken.example.com + +[broken-host-vars] +broken-host-vars.example.com diff --git a/test/integration/targets/vault/invalid_format/original-broken-host-vars b/test/integration/targets/vault/invalid_format/original-broken-host-vars new file mode 100644 index 0000000000..6be696b5cb --- /dev/null +++ b/test/integration/targets/vault/invalid_format/original-broken-host-vars @@ -0,0 +1,6 @@ +$ANSIBLE_VAULT;1.1;AES256 +64323332393930623633306662363165386332376638653035356132646165663632616263653366 +6233383362313531623238613461323861376137656265380a366464663835633065616361636231 +3366323866663763660a323766383531396433663861656532373663373134376263383263316261 +3137 + diff --git a/test/integration/targets/vault/invalid_format/original-group-vars.yml b/test/integration/targets/vault/invalid_format/original-group-vars.yml new file mode 100644 index 0000000000..817557be5d --- /dev/null +++ b/test/integration/targets/vault/invalid_format/original-group-vars.yml @@ -0,0 +1,2 @@ +--- +some_var_that_fails: blippy diff --git a/test/integration/targets/vault/invalid_format/some-vars b/test/integration/targets/vault/invalid_format/some-vars new file mode 100644 index 0000000000..e841a262a7 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/some-vars @@ -0,0 +1,6 @@ +$ANSIBLE_VAULT;1.1;AES256 +37303462633933386339386465613039363964643466663866356261313966663465646262636333 +3965643566363764356563363334363431656661636634380a333837343065326239336639373238 +64316236383836383434366662626339643561616630326137383262396331396538363136323063 +6236616130383264620a613863373631316234656236323332633166623738356664353531633239 +3533 diff --git a/test/integration/targets/vault/invalid_format/vault-secret b/test/integration/targets/vault/invalid_format/vault-secret new file mode 100644 index 0000000000..4406e35cd5 --- /dev/null +++ b/test/integration/targets/vault/invalid_format/vault-secret @@ -0,0 +1 @@ +enemenemu \ No newline at end of file diff --git a/test/integration/targets/vault/runme.sh b/test/integration/targets/vault/runme.sh index 25bd65f69c..edc4cc4209 100755 --- a/test/integration/targets/vault/runme.sh +++ b/test/integration/targets/vault/runme.sh @@ -23,6 +23,7 @@ echo "This is a test file for edit2" > "${TEST_FILE_EDIT2}" FORMAT_1_1_HEADER="\$ANSIBLE_VAULT;1.1;AES256" FORMAT_1_2_HEADER="\$ANSIBLE_VAULT;1.2;AES256" + VAULT_PASSWORD_FILE=vault-password # old format @@ -359,3 +360,9 @@ WRONG_RC=$? echo "rc was $WRONG_RC (1 is expected)" [ $WRONG_RC -eq 1 ] +# test invalid format ala https://github.com/ansible/ansible/issues/28038 +EXPECTED_ERROR='Vault format unhexlify error: Non-hexadecimal digit found' +ansible-playbook "$@" -i invalid_format/inventory --vault-id invalid_format/vault-secret invalid_format/broken-host-vars-tasks.yml 2>&1 | grep "${EXPECTED_ERROR}" + +EXPECTED_ERROR='Vault format unhexlify error: Odd-length string' +ansible-playbook "$@" -i invalid_format/inventory --vault-id invalid_format/vault-secret invalid_format/broken-group-vars-tasks.yml 2>&1 | grep "${EXPECTED_ERROR}" diff --git a/test/units/parsing/vault/test_vault.py b/test/units/parsing/vault/test_vault.py index 127aae1890..73d6f164f0 100644 --- a/test/units/parsing/vault/test_vault.py +++ b/test/units/parsing/vault/test_vault.py @@ -41,6 +41,62 @@ from units.mock.loader import DictDataLoader from units.mock.vault_helper import TextVaultSecret +class TestUnhexlify(unittest.TestCase): + def test(self): + b_plain_data = b'some text to hexlify' + b_data = hexlify(b_plain_data) + res = vault._unhexlify(b_data) + self.assertEquals(res, b_plain_data) + + def test_odd_length(self): + b_data = b'123456789abcdefghijklmnopqrstuvwxyz' + + self.assertRaisesRegexp(vault.AnsibleVaultFormatError, + '.*Vault format unhexlify error.*', + vault._unhexlify, + b_data) + + def test_nonhex(self): + b_data = b'6z36316566653264333665333637623064303639353237620a636366633565663263336335656532' + + self.assertRaisesRegexp(vault.AnsibleVaultFormatError, + '.*Vault format unhexlify error.*Non-hexadecimal digit found', + vault._unhexlify, + b_data) + + +class TestParseVaulttext(unittest.TestCase): + def test(self): + vaulttext_envelope = u'''$ANSIBLE_VAULT;1.1;AES256 +33363965326261303234626463623963633531343539616138316433353830356566396130353436 +3562643163366231316662386565383735653432386435610a306664636137376132643732393835 +63383038383730306639353234326630666539346233376330303938323639306661313032396437 +6233623062366136310a633866373936313238333730653739323461656662303864663666653563 +3138''' + + b_vaulttext_envelope = to_bytes(vaulttext_envelope, errors='strict', encoding='utf-8') + b_vaulttext, b_version, cipher_name, vault_id = vault.parse_vaulttext_envelope(b_vaulttext_envelope) + res = vault.parse_vaulttext(b_vaulttext) + self.assertIsInstance(res[0], bytes) + self.assertIsInstance(res[1], bytes) + self.assertIsInstance(res[2], bytes) + + def test_non_hex(self): + vaulttext_envelope = u'''$ANSIBLE_VAULT;1.1;AES256 +3336396J326261303234626463623963633531343539616138316433353830356566396130353436 +3562643163366231316662386565383735653432386435610a306664636137376132643732393835 +63383038383730306639353234326630666539346233376330303938323639306661313032396437 +6233623062366136310a633866373936313238333730653739323461656662303864663666653563 +3138''' + + b_vaulttext_envelope = to_bytes(vaulttext_envelope, errors='strict', encoding='utf-8') + b_vaulttext, b_version, cipher_name, vault_id = vault.parse_vaulttext_envelope(b_vaulttext_envelope) + self.assertRaisesRegexp(vault.AnsibleVaultFormatError, + '.*Vault format unhexlify error.*Non-hexadecimal digit found', + vault.parse_vaulttext, + b_vaulttext_envelope) + + class TestVaultSecret(unittest.TestCase): def test(self): secret = vault.VaultSecret()