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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote:
> >>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).

Look, the code is not merged yet and you already have
legacy with revision history to support? Why create work?

> >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?

binary

> >>   - 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'.

But than might be a typo.
0x1234567O
for example stops parsing at 'O' since this is not a digit but
looks like one.


> >Two empty lines in a row :)
> >
> Probably this is not the only occurrence... Is this a problem?

Yes. They aren't helpful. checkpatch should complain about these
not sure whether it does.

> >>+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.

Oh my. I presume this makes sense in some world ...

> >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

These should be different options.

key_format=hex/string

etc.

-- 
MST