|
| From: | Stefan Berger |
| Subject: | Re: [Qemu-devel] [PATCH V8 10/14] Encrypt state blobs using AES CBC encryption |
| Date: | Thu, 01 Sep 2011 22:23:51 -0400 |
| User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.18) Gecko/20110621 Fedora/3.1.11-1.fc14 Lightning/1.0b3pre Thunderbird/3.1.11 |
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.
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.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?
I did this in the previous patch that implemented rev 1 that knew nothing about the encryption added in rev 2.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).
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).
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?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?
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'.- 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<address@hidden> --- 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?
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?
c is the number of 'nibbles'. 4 bits in a nibble - that's what the /4 comes from.+ } + + return true;Always put spaces around /. But where does the /4 come from? 4 bits per character?
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.+} + + 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);
Stefan
| [Prev in Thread] | Current Thread | [Next in Thread] |