If we are encoding a block of data that is 16 bytes in length,
we cannot leave it as 16 bytes, we must pad it out to the next
block boundary, 32 bytes. Without this padding, the decoder will
incorrectly treat the last byte of plain text as the padding
length, as it can't distinguish padded from non-padded data.

The problem exhibited itself when using a 16 byte passphrase
for a LUKS volume

  $ virsh secret-set-value 55806c7d-8e93-456f-829b-607d8c198367 \
       $(echo -n 1234567812345678 | base64)
  Secret value set

  $ virsh start demo
  error: Failed to start domain demo
  error: internal error: process exited while connecting to monitor: >>>>>>>>>>Len 16
  2017-05-02T10:35:40.016390Z qemu-system-x86_64: -object \
    secret,id=virtio-disk1-luks-secret0,data=SEtNi5vDUeyseMKHwc1c1Q==,\
    keyid=masterKey0,iv=zm7apUB1A6dPcH53VW960Q==,format=base64: \
    Incorrect number of padding bytes (56) found on decrypted data

Notice how the padding '56' corresponds to the ordinal value of
the character '8'.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 src/util/vircrypto.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 8748e1c..48b04fc 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -152,8 +152,14 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg,
     uint8_t *ciphertext;
     size_t ciphertextlen;
 
-    /* Allocate a padded buffer, copy in the data */
-    ciphertextlen = VIR_ROUND_UP(datalen, 16);
+    /* Allocate a padded buffer, copy in the data.
+     *
+     * NB, we must *always* have at least 1 byte of
+     * padding - we can't skip it on multiples of
+     * 16, otherwise decoder can't distinguish padded
+     * data from non-padded data. Hence datalen + 1
+     */
+    ciphertextlen = VIR_ROUND_UP(datalen + 1, 16);
     if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
         return -1;
     memcpy(ciphertext, data, datalen);
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, May 02, 2017 at 12:02:23PM +0100, Daniel P. Berrange wrote:
> If we are encoding a block of data that is 16 bytes in length,
> we cannot leave it as 16 bytes, we must pad it out to the next
> block boundary, 32 bytes. Without this padding, the decoder will
> incorrectly treat the last byte of plain text as the padding
> length, as it can't distinguish padded from non-padded data.
>
> The problem exhibited itself when using a 16 byte passphrase
> for a LUKS volume
>
>   $ virsh secret-set-value 55806c7d-8e93-456f-829b-607d8c198367 \
>        $(echo -n 1234567812345678 | base64)
>   Secret value set
>
>   $ virsh start demo
>   error: Failed to start domain demo
>   error: internal error: process exited while connecting to monitor: >>>>>>>>>>Len 16
>   2017-05-02T10:35:40.016390Z qemu-system-x86_64: -object \
>     secret,id=virtio-disk1-luks-secret0,data=SEtNi5vDUeyseMKHwc1c1Q==,\
>     keyid=masterKey0,iv=zm7apUB1A6dPcH53VW960Q==,format=base64: \
>     Incorrect number of padding bytes (56) found on decrypted data
>
> Notice how the padding '56' corresponds to the ordinal value of
> the character '8'.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

ACK

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list