Better handling of malformed vault data envelope

lib/ansible/parsing/vault/__init__.py

@@ -29,6 +29,7 @@
 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,28 +153,14 @@ 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';')
 
     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.
 
@@ -609,7 +666,8 @@ def decrypt_and_get_vault_id(self, vaulttext, filename=None):
                 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
@@ -665,6 +723,13 @@ def decrypt_and_get_vault_id(self, vaulttext, filename=None):
                     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))
@@ -862,7 +927,8 @@ def edit_file(self, filename):
 
         # 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
@@ -1136,7 +1202,7 @@ def decrypt(cls, b_vaulttext, secret, key_length=32):
                            '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:]
 
@@ -1289,7 +1355,7 @@ def _decrypt_cryptography(cls, b_ciphertext, b_crypted_hmac, b_key1, b_key2, b_i
         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)
 
@@ -1351,11 +1417,8 @@ def _decrypt_pycrypto(cls, b_ciphertext, b_crypted_hmac, b_key1, b_key2, b_iv):
 
     @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?)

test/integration/targets/vault/invalid_format/README.md

@@ -0,0 +1 @@
+Based on https://github.com/yves-vogl/ansible-inline-vault-issue

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 

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] }}"

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

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
+

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

test/integration/targets/vault/invalid_format/original-broken-host-vars

@@ -0,0 +1,6 @@
+$ANSIBLE_VAULT;1.1;AES256
+64323332393930623633306662363165386332376638653035356132646165663632616263653366
+6233383362313531623238613461323861376137656265380a366464663835633065616361636231
+3366323866663763660a323766383531396433663861656532373663373134376263383263316261
+3137
+

test/integration/targets/vault/invalid_format/original-group-vars.yml

@@ -0,0 +1,2 @@
+---
+some_var_that_fails: blippy

test/integration/targets/vault/invalid_format/some-vars

@@ -0,0 +1,6 @@
+$ANSIBLE_VAULT;1.1;AES256
+37303462633933386339386465613039363964643466663866356261313966663465646262636333
+3965643566363764356563363334363431656661636634380a333837343065326239336639373238
+64316236383836383434366662626339643561616630326137383262396331396538363136323063
+6236616130383264620a613863373631316234656236323332633166623738356664353531633239
+3533

test/integration/targets/vault/invalid_format/vault-secret

@@ -0,0 +1 @@
+enemenemu

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
 # new format, view, using password client script
 ansible-vault view "$@" --vault-id vault-password@test-vault-client.py format_1_1_AES256.yml
@@ -367,3 +368,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}"

test/units/parsing/vault/test_vault.py

@@ -41,6 +41,62 @@
 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()