Re: [Qemu-devel] [PATCH V8 10/14] Encrypt state blobs using AES CBC encryption

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 09/01/2011 03:26 PM, Michael S. Tsirkin wrote:
> On Wed, Aug 31, 2011 at 10:36:01AM -0400, Stefan Berger wrote:
>> This patch adds encryption of the individual state blobs that are written
>> into the block storage. The 'directory' at the beginnig of the block
>> storage is not encrypted.
> Does this mean that there's a new format that we store data
> in, and that qemu needs to support?
No, this type of encryption is completely handled inside this file.
> If so, this needs an entry under docs documenting the format.
>
>> The encryption support added in this patch would also work if QCoW2 was not
>> to be used as the (only) image file format to store the TPM's state.
> What does 'was not to be used' above mean?
>
The only image file format that the libtpms based 'builtin' backend 
currently accepts is the QCoW2 file format. QCoW2 provides encryption of 
its own (I find it a bit problematic). From what I know none of the 
other file formats provides encryption, i.e., 'raw' for sure does not. 
So if the image type format requirement was to be relaxed to also allow 
for example 'raw', then the encryption added in this patch would still work.
>> Keys can be passed as a string of hexadecimal digits forming a 256, 192 or
>> 128 bit AES key. The string can optionally start with '0x'. If the
>> parser does not recognize it as a hexadecimal number, the string itself is
>> taken as the AES key, which makes for example 'my_key' a valid AES key
>> parameter. It is also necessary to provide the encryption scheme.
>> Currently only 'aes-cbc' is supported.  An example for a valid key command
>> line argument is:
>>
>> -tpm builtin,key=aes-cbc:0x1234567890abcdef123456
>>
>> The key passed via command line argument is wiped from the command
>> line after parsing. If for example key=aes-cbc:0x1234... was passed it will
>> then be changed to key=------... so that 'ps' does not show the key anymore.
>> Obviously it cannot be completely prevented that the key is visible during a
>> very short period of time until qemu gets to the point where the code wiping
>> the key is reached.
>>
>> A byte indicating the encryption type being used is introduced in the
>> directory structure indicating whether blobs are encrypted and if so, what
>> encryption type was used, i.e., aes-cbc.
>>
>> An additional 'layer' for reading and writing the blobs to the underlying
>> block storage is added. This layer encrypts the blobs for writing if a key is
>> available. Similarly it decrypts the blobs after reading.
>>
>> Checks are added that test
>> - whether encryption is supported follwing the revision of the directory
>>    structure (rev>= 2)
> You never generate rev 1 code, right?
I did this in the previous patch that implemented rev 1 that knew 
nothing about the encryption added in rev 2.
> So why keep that support around in code?
> The first version merged into qemu should be revision 0 (or 1, as you like).
I chose '1'. See patch 9:

+#define BS_DIR_REV1         1
+
+#define BS_DIR_REV_CURRENT  BS_DIR_REV1
+

So I think it's the proper thing to do to increase the revision number 
from 1 to 2 since it's in two separate patches (even if they were to be 
applied immediately).
> Don't support legacy with old version of your patch.
>
>> - whether a key has been provided although all data are stored in clear-text
>> - whether a key is missing for decryption.
>>
>> In either one of the cases the backend reports an error message to the user
>> and Qemu terminates.
>>
>> -v7:
>>    - cleaned up function parsing key
>>
>> -v6:
>>    - changed the format of the key= to take the type of encryption into
>>      account: key=aes-cbc:0x12345... and reworked code for encryption and
>>      decryption of blobs;
> separate type and data:
> keytype=aes-cbc,key=0x123  to avoid introducing more option parsing.
> Also, are people likely to have the key in a file?
> If yes maybe read a key from there and skip parsing completely?
>
I think both choices should probably exist. Now what's a good file 
format? Would we expect to find a hex number in there or should it 
always be assumed to be a binary file?
>>    - modified directory entry to hold a uint_8 describing the encryption
>>      type (none, aes-cbc) being used for the blobs.
>>    - incrementing revision of the directory to '2' indicating encryption
>>      support
>>
>> -v5:
>>    - -tpmdev now also gets a key parameter
>>    - add documentation about key parameter
>>
>> Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
>>
>> ---
>>   hw/tpm_builtin.c |  285 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   qemu-config.c    |   10 +
>>   qemu-options.hx  |   22 +++-
>>   tpm.c            |   10 +
>>   4 files changed, 318 insertions(+), 9 deletions(-)
>>
>> Index: qemu-git/hw/tpm_builtin.c
>> ===================================================================
>> --- qemu-git.orig/hw/tpm_builtin.c
>> +++ qemu-git/hw/tpm_builtin.c
>> @@ -27,6 +27,7 @@
>>   #include "hw/pc.h"
>>   #include "migration.h"
>>   #include "sysemu.h"
>> +#include "aes.h"
>>
>>   #include<libtpms/tpm_library.h>
>>   #include<libtpms/tpm_error.h>
>> @@ -110,14 +111,27 @@ typedef struct BSDir {
>>       uint16_t  rev;
>>       uint32_t  checksum;
>>       uint32_t  num_entries;
>> -    uint32_t  reserved[10];
>> +    uint8_t   enctype;
>> +    uint8_t   reserved1[3];
>> +    uint32_t  reserved[8];
>>       BSEntry   entries[BS_DIR_MAX_NUM_ENTRIES];
>>   } __attribute__((packed)) BSDir;
>>
>>
>>   #define BS_DIR_REV1         1
>> +/* rev 2 added encryption */
>> +#define BS_DIR_REV2         2
>>
>> -#define BS_DIR_REV_CURRENT  BS_DIR_REV1
>> +
>> +#define BS_DIR_REV_CURRENT  BS_DIR_REV2
>> +
>> +/* above enctype */
>> +enum BSEnctype {
>> +    BS_DIR_ENCTYPE_NONE = 0,
>> +    BS_DIR_ENCTYPE_AES_CBC,
>> +
>> +    BS_DIR_ENCTYPE_LAST,
>> +};
>>
>>   /* local variables */
>>
>> @@ -150,6 +164,11 @@ static const unsigned char tpm_std_fatal
>>
>>   static char dev_description[80];
>>
>> +static struct enckey {
>> +    uint8_t enctype;
>> +    AES_KEY tpm_enc_key;
>> +    AES_KEY tpm_dec_key;
>> +} enckey;
>>
>>   static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs,
>>                                                  enum BSEntryType be,
>> @@ -264,7 +283,7 @@ static uint32_t tpm_builtin_calc_dir_che
>>
>>   static bool tpm_builtin_is_valid_bsdir(BSDir *dir)
>>   {
>> -    if (dir->rev != BS_DIR_REV_CURRENT ||
>> +    if (dir->rev>  BS_DIR_REV_CURRENT ||
>>           dir->num_entries>  BS_DIR_MAX_NUM_ENTRIES) {
>>           return false;
>>       }
>> @@ -295,6 +314,33 @@ static bool tpm_builtin_has_valid_conten
>>       return rc;
>>   }
>>
>> +static bool tpm_builtin_supports_encryption(const BSDir *dir)
>> +{
>> +    return (dir->rev>= BS_DIR_REV2);
>> +}
>> +
>> +
>> +static bool tpm_builtin_has_missing_key(const BSDir *dir)
>> +{
>> +    return ((dir->enctype   != BS_DIR_ENCTYPE_NONE)&&
>> +            (enckey.enctype == BS_DIR_ENCTYPE_NONE));
>> +}
>> +
>> +
>> +static bool tpm_builtin_has_unnecessary_key(const BSDir *dir)
>> +{
>> +    return (((dir->enctype   == BS_DIR_ENCTYPE_NONE)&&
>> +             (enckey.enctype != BS_DIR_ENCTYPE_NONE)) ||
>> +            ((!tpm_builtin_supports_encryption(dir))&&
>> +             (enckey.enctype != BS_DIR_ENCTYPE_NONE)));
>> +}
>> +
>> +
>> +static bool tpm_builtin_uses_unsupported_enctype(const BSDir *dir)
>> +{
>> +    return (dir->enctype>= BS_DIR_ENCTYPE_LAST);
>> +}
>> +
>>
>>   static int tpm_builtin_create_blank_dir(BlockDriverState *bs)
>>   {
>> @@ -306,6 +352,7 @@ static int tpm_builtin_create_blank_dir(
>>       dir = (BSDir *)buf;
>>       dir->rev = BS_DIR_REV_CURRENT;
>>       dir->num_entries = 0;
>> +    dir->enctype = enckey.enctype;
>>
>>       dir->checksum = tpm_builtin_calc_dir_checksum(dir);
>>
>> @@ -407,6 +454,38 @@ static int tpm_builtin_startup_bs(BlockD
>>
>>       tpm_builtin_dir_be_to_cpu(dir);
>>
>> +    if (tpm_builtin_is_valid_bsdir(dir)) {
>> +        if (tpm_builtin_supports_encryption(dir)&&
>> +            tpm_builtin_has_missing_key(dir)) {
>> +            fprintf(stderr,
>> +                    "tpm: the data are encrypted but I am missing the key.\n");
>> +            rc = -EIO;
>> +            goto err_exit;
>> +        }
>> +        if (tpm_builtin_has_unnecessary_key(dir)) {
>> +            fprintf(stderr,
>> +                    "tpm: I have a key but the data are not encrypted.\n");
>> +            rc = -EIO;
>> +            goto err_exit;
>> +        }
>> +        if (tpm_builtin_supports_encryption(dir)&&
>> +            tpm_builtin_uses_unsupported_enctype(dir)) {
>> +            fprintf(stderr,
>> +                    "tpm: State is encrypted with an unsupported encryption "
>> +                    "scheme.\n");
>> +            rc = -EIO;
>> +            goto err_exit;
>> +        }
>> +        if (tpm_builtin_supports_encryption(dir)&&
>> +            (dir->enctype != BS_DIR_ENCTYPE_NONE)&&
>> +            !tpm_builtin_has_valid_content(dir)) {
>> +            fprintf(stderr, "tpm: cannot read the data - "
>> +                    "is this the wrong key?\n");
>> +            rc = -EIO;
>> +            goto err_exit;
>> +        }
>> +    }
>> +
>>       if (!tpm_builtin_is_valid_bsdir(dir) ||
>>           !tpm_builtin_has_valid_content(dir)) {
>>           /* if it's encrypted and has something else than null-content,
>> @@ -569,6 +648,105 @@ static int set_bs_entry_size_crc(BlockDr
>>   }
>>
>>
>> +static int tpm_builtin_blocksize_roundup(uint8_t enctype, int plainsize)
>> +{
>> +    switch (enctype) {
>> +    case BS_DIR_ENCTYPE_NONE:
>> +        return plainsize;
>> +    case BS_DIR_ENCTYPE_AES_CBC:
>> +        return ALIGN(plainsize, AES_BLOCK_SIZE);
>> +    default:
>> +        assert(false);
>> +        return 0;
>> +    }
>> +}
>> +
>> +
>> +static int tpm_builtin_bdrv_pread(BlockDriverState *bs, int64_t offset,
>> +                                  void *buf, int count,
>> +                                  enum BSEntryType type)
>> +{
>> +    int ret;
>> +    union {
>> +        uint64_t ll[2];
>> +        uint8_t b[16];
>> +    } ivec;
>> +    int toread = count;
>> +
>> +    toread = tpm_builtin_blocksize_roundup(enckey.enctype, count);
>> +
>> +    ret = bdrv_pread(bs, offset, buf, toread);
>> +
>> +    if (ret != toread) {
>> +        return ret;
>> +    }
>> +
>> +    switch (enckey.enctype) {
>> +    case BS_DIR_ENCTYPE_NONE:
>> +        break;
>> +    case BS_DIR_ENCTYPE_AES_CBC:
>> +        ivec.ll[0] = cpu_to_be64(type);
>> +        ivec.ll[1] = 0;
>> +
>> +        AES_cbc_encrypt(buf, buf, toread,&enckey.tpm_dec_key, ivec.b, 0);
>> +        break;
>> +    default:
>> +        assert(false);
>> +    }
>> +
>> +    return count;
>> +}
>> +
>> +
>> +static int tpm_builtin_bdrv_pwrite(BlockDriverState *bs, int64_t offset,
>> +                                   void *buf, int count,
>> +                                   enum BSEntryType type)
>> +{
>> +    int ret;
>> +    union {
>> +        uint64_t ll[2];
>> +        uint8_t b[16];
>> +    } ivec;
>> +    int towrite = count;
>> +    void *out_buf = buf;
>> +
>> +    switch (enckey.enctype) {
>> +    case BS_DIR_ENCTYPE_NONE:
>> +        break;
>> +    case BS_DIR_ENCTYPE_AES_CBC:
>> +        ivec.ll[0] = cpu_to_be64(type);
>> +        ivec.ll[1] = 0;
>> +
>> +        towrite = ALIGN(count, AES_BLOCK_SIZE);
>> +
>> +        if (towrite != count) {
>> +            out_buf = g_malloc(towrite);
>> +
>> +            if (out_buf == NULL) {
>> +                return -ENOMEM;
>> +            }
>> +        }
>> +
>> +        AES_cbc_encrypt(buf, out_buf, towrite,&enckey.tpm_enc_key, ivec.b, 1);
>> +        break;
>> +    default:
>> +        assert(false);
>> +    }
>> +
>> +    ret = bdrv_pwrite(bs, offset, out_buf, towrite);
>> +
>> +    if (out_buf != buf) {
>> +        g_free(out_buf);
>> +    }
>> +
>> +    if (ret == towrite) {
>> +        return count;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +
>>   static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs,
>>                                                  enum BSEntryType be,
>>                                                  TPMSizedBuffer *tsb)
>> @@ -594,7 +772,7 @@ static int tpm_builtin_load_sized_data_f
>>           goto err_exit;
>>       }
>>
>> -    tsb->buffer = g_malloc(entry.blobsize);
>> +    tsb->buffer = g_malloc(ALIGN(entry.blobsize, AES_BLOCK_SIZE));
>>       if (!tsb->buffer) {
>>           rc = -ENOMEM;
>>           goto err_exit;
>> @@ -602,7 +780,8 @@ static int tpm_builtin_load_sized_data_f
>>
>>       tsb->size = entry.blobsize;
>>
>> -    if (bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size) != tsb->size) {
>> +    if (tpm_builtin_bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size, be) !=
>> +        tsb->size) {
>>           clear_sized_buffer(tsb);
>>           fprintf(stderr, "tpm: Error while reading stored data!\n");
>>           rc = -EIO;
>> @@ -667,7 +846,8 @@ static int tpm_builtin_save_sized_data_t
>>       }
>>
>>       if (data_len>  0) {
>> -        if (bdrv_pwrite(bs, entry.offset, data, data_len) != data_len) {
>> +        if (tpm_builtin_bdrv_pwrite(bs, entry.offset, data, data_len, be) !=
>> +            data_len) {
>>               rc = -EIO;
>>           }
>>       }
>> @@ -1492,11 +1672,77 @@ static const char *tpm_builtin_create_de
>>   }
>>
>>
>> +/*
>> + * Convert a string of hex digits to its binary representation.
>> + * The conversion stops once either the maximum size of the binary
>> + * array has been reached or an non-hex digit was encountered.
>> + */
> Don't we care about non-hex following a valid key?
Do you have an example? This function is meant to convert 0x1234567 to a 
binary stream. An equivalent would be 0x1234567X, since it would 
terminate parsing on 'X'.
> This will silently discard them if length matches
> a legal value by luck.
>
>> +static size_t stream_to_bin(const char *stream,
>> +                            unsigned char *bin, size_t bin_size)
>> +{
>> +    size_t c = 0;
>> +    unsigned char nib = 0;
>> +
>> +    while (c<  bin_size&&  stream[c] != 0) {
>> +        if (stream[c]>= '0'&&  stream[c]<= '9') {
>> +            nib |= stream[c] - '0';
>> +        } else if (stream[c]>= 'A'&&  stream[c]<= 'F') {
>> +            nib |= stream[c] - 'A' + 10;
>> +        } else if (stream[c]>= 'a'&&  stream[c]<= 'f') {
>> +            nib |= stream[c] - 'a' + 10;
>> +        } else {
>> +            break;
>> +        }
>> +
>> +        if ((c&  1) == 1) {
>> +            bin[c/2] = nib;
>> +            nib = 0;
>> +        } else {
>> +            nib<<= 4;
>> +            bin[c/2] = nib;
>> +        }
>> +
>> +        c++;
>> +    }
>> +
>> +    return c;
>> +}
> Can't this use something like scanf %x instead?
Sure it could...

> Something like the below seems to work for me,
> and gives length in bytes and not nibbles.
>
> #include<stdio.h>
> #include<assert.h>
>
> int main(int argc, char **argv)
> {
>          int l = 0, b = 0, s, n;
>          char buf[256 / 8];
>          for (b = 0; b<  sizeof(buf); ++b) {
>                  s = sscanf(argv[1] + l, "%2hhx%n", buf + b,&n);
>                  if (s == 0) {
>                          printf("invalid input. scanned %d bytes, text left: %s\n", b, argv[1] + l);
>                          return 1;
>                  }
>                  assert(s != EOF&&  n>= 1&&  n<= 2);
>                  l += n;
>                  if (!argv[1][l]) {
>                          printf("scanned %d bytes length %d\n", b + 1, l);
>                          return 0;
>                  }
>          }
>          printf("key too long. scanned %d bytes, text left: %s\n", b, argv[1] + l);
>          return 2;
> }
>
>
>
>> +
>> +
> Two empty lines in a row :)
>
Probably this is not the only occurrence... Is this a problem?
>> +static bool tpm_builtin_parse_as_hexkey(const char *rawkey,
>> +                                        unsigned char *keyvalue,
>> +                                        int *keysize)
>> +{
>> +    size_t c = 0;
>> +
>> +    /* skip over leading '0x' */
>> +    if (!strncmp(rawkey, "0x", 2)) {
>> +        rawkey += 2;
>> +    }
>> +
>> +    c = stream_to_bin(rawkey, keyvalue, *keysize);
>> +
>> +    if (c == 256/4) {
>> +        *keysize = 256;
>> +    } else if (c>= 192/4) {
>> +        *keysize = 192;
>> +    } else if (c>= 128/4) {
>> +        *keysize = 128;
>> +    } else {
>> +        return false;
> Want to tell the user what went wrong?
Here's what the key parser handles:
- all keys >= 256 bits are truncated to 256 bits
- all keys >= 192 bits are truncated to 192 bits
- all keys >= 128 bits are truncated to 128 bits
- all keys < 128 bits are assumed to not be given as a hexadecimal 
number but the string itself is the key, i.e. 'HELLOWORLD' becomes a 
valid key.
> Also, you don't allow skipping leading zeroes?
An AES key should be allowed to have leading zeros, no?
>> +    }
>> +
>> +    return true;
> Always put spaces around /.
> But where does the /4 come from? 4 bits per character?
>
c is the number of 'nibbles'. 4 bits in a nibble - that's what the /4 
comes from.
>> +}
>> +
>> +
>>   static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id,
>>                                         const char *model)
>>   {
>>       TPMBackend *driver;
>>       const char *value;
>> +    unsigned char keyvalue[256/8];
>> +    int keysize = sizeof(keyvalue);
>>
>>       driver = g_malloc(sizeof(TPMBackend));
>>       if (!driver) {
>> @@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe
>>           goto err_exit;
>>       }
>>
>> +    value = qemu_opt_get(opts, "key");
>> +    if (value) {
>> +        if (!strncasecmp(value, "aes-cbc:", 8)) {
>> +            memset(keyvalue, 0x0, sizeof(keyvalue));
>> +
>> +            if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue,&keysize)) {
>> +                keysize = 128;
>> +                strncpy((char *)keyvalue, value, 128/8);
Here first the input is attempted to be parsed as hex key and if that 
fails the input string is taken as the key. It should be &value[8] here 
-- so that's a bug.

   Stefan