Better handling of malformed vault data envelope (#32515)

* Better handling of malformed vault data envelope

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

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.
 
@@ -609,7 +666,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
@@ -665,6 +723,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))
@@ -862,7 +927,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
@@ -1136,7 +1202,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:]
 
@@ -1289,7 +1355,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)
 
@@ -1351,11 +1417,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?)

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.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()