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

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

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

> 
> 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?
So why keep that support around in code?
The first version merged into qemu should be revision 0 (or 1, as you like).
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?


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

> +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?
Also, you don't allow skipping leading zeroes?

> +    }
> +
> +    return true;

Always put spaces around /.
But where does the /4 come from? 4 bits per character?


> +}
> +
> +
>  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);
> +            }
> +
> +            if (AES_set_encrypt_key(keyvalue, keysize,
> +                                    &enckey.tpm_enc_key) != 0 ||
> +                AES_set_decrypt_key(keyvalue, keysize,
> +                                    &enckey.tpm_dec_key) != 0) {
> +                fprintf(stderr, "tpm: Error setting AES key.\n");
> +                goto err_exit;
> +            }
> +            enckey.enctype = BS_DIR_ENCTYPE_AES_CBC;
> +        } else {
> +            fprintf(stderr, "tpm: Unknown encryption scheme. Known types are: "
> +                            "aes-cbc.\n");
> +            goto err_exit;
> +        }
> +    } else {
> +        enckey.enctype = BS_DIR_ENCTYPE_NONE;
> +    }
> +
>      return driver;
>  
>  err_exit:
> Index: qemu-git/qemu-config.c
> ===================================================================
> --- qemu-git.orig/qemu-config.c
> +++ qemu-git/qemu-config.c
> @@ -522,6 +522,11 @@ static QemuOptsList qemu_tpmdev_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Persistent storage for TPM state",
>          },
> +        {
> +            .name = "key",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Data encryption key",
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -546,6 +551,11 @@ static QemuOptsList qemu_tpm_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Persistent storage for TPM state",
>          },
> +        {
> +            .name = "key",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Data encryption key",
> +        },
>          { /* end of list */ }
>      },
>  };
> Index: qemu-git/tpm.c
> ===================================================================
> --- qemu-git.orig/tpm.c
> +++ qemu-git/tpm.c
> @@ -245,6 +245,7 @@ void tpm_cleanup(void)
>  void tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
>  {
>      QemuOpts *opts;
> +    char *key;
>  
>      if (strcmp("none", optarg) != 0) {
>          if (*optarg == '?') {
> @@ -255,6 +256,15 @@ void tpm_config_parse(QemuOptsList *opts
>          if (!opts) {
>              exit(1);
>          }
> +
> +        /* if a key is provided, wipe it out so no one can see it with 'ps' */
> +        key = strstr(optarg, "key=");
> +        if (key) {
> +            key += 4;
> +            while (key[0] && key[0] != ',') {
> +                *key++ = '-';
> +            }
> +        }
>      }
>  }
>  
> Index: qemu-git/qemu-options.hx
> ===================================================================
> --- qemu-git.orig/qemu-options.hx
> +++ qemu-git/qemu-options.hx
> @@ -1766,8 +1766,9 @@ DEFHEADING(TPM device options:)
>  # ifdef CONFIG_TPM
>  DEF("tpm", HAS_ARG, QEMU_OPTION_tpm, \
>      "" \
> -    "-tpm builtin,path=<path>[,model=<model>]\n" \
> +    "-tpm builtin,path=<path>[,model=<model>][,key=<aes key>]\n" \
>      "                enable a builtin TPM with state in file in path\n" \
> +    "                and encrypt the TPM's state with the given AES key\n" \
>      "-tpm model=?    to list available TPM device models\n" \
>      "-tpm ?          to list available TPM backend types\n",
>      QEMU_ARCH_I386)
> @@ -1796,13 +1797,22 @@ Use ? to print all available TPM backend
>  qemu -tpmdev ?
>  @end example
>  
> -@item -tpmdev builtin ,id=@var{id}, path=@var{path}
> +@item -tpmdev builtin ,id=@var{id}, path=@var{path} [,key=@var{key}]
>  
>  Creates an instance of the built-in TPM.
>  
>  @option{path} specifies the path to the QCoW2 image that will store
>  the TPM's persistent data. @option{path} is required.
>  
> +@option{key} specifies the AES key to use to encrypt the TPM's persistent
> +data. If encryption is to be used, the key must be provided the first
> +time a Qemu VM with attached TPM is started and the same key must subsequently
> +be used. The format of the key is the type of encryption to use, i.e.,
> +@code{aes-cbc}, followed by a colon and then the actual key. The key can
> +be a hex number with optional leading @code{0x}
> +and 32, 48 or 64 hex digits for 128, 192 or 256 bit AES keys respectively.
> +@option{key} is optional.
> +
>  To create a built-in TPM use the following two options:
>  @example
>  -tpmdev builtin,id=tpm0,path=<path_to_qcow2> -device tpm-tis,tpmdev=tpm0
> @@ -1810,12 +1820,18 @@ To create a built-in TPM use the followi
>  Not that the @code{-tpmdev} id is @code{tpm0} and is referenced by
>  @code{tpmdev=tpm0} in the device option.
>  
> +
> +To create a built-in TPM whose state is encrypted with a 128 bit AES key
> +using AES-CBC encryption scheme supply the following two options:
> +@example
> +-tpmdev builtin,id=tpm0,path=<path_to_qcow2>,key=aes-cbc:0x1234567890abcdef01234567890abcdef -device tpm-tis,tpmdev=tpm0
> +@end example
>  @end table
>  
>  The short form of a TPM device option is:
>  @table @option
>  
> -@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}]
> +@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}] [,key=@var{key}]
>  @findex -tpm
>  
>  @option{model} specifies the device model. The default device model is a
>