[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Stefan Berger
Subject: Re: [Qemu-devel] [PATCH V8 10/14] Encrypt state blobs using AES CBC encryption
Date: Tue, 06 Sep 2011 20:32:41 -0400
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20110621 Fedora/3.1.11-1.fc14 Lightning/1.0b3pre Thunderbird/3.1.11

On 09/04/2011 12:58 PM, Michael S. Tsirkin wrote:
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

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?

Ok, I won't build the intermediate version then.
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.

   - cleaned up function parsing key

   - 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?
So then I'd keep the possibility to pass the key via command line and add the option to read it from a file as well.
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.
It so far doesn't.
+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 ...

So then I will require to have the exact size of a 128, 192 or 256 bit key then and maybe allow 'passwords', which really was what the last example above was falling back to. Looking around in the QCoW2 code passwords seem to be also possible there (block/qcow2.c:qcow_set_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.

These should be different options.



To summarize it:
enc_mode=<aes-cbc> # redundant for now since this is the only supported encryption scheme; so could drop it and assume as default

key_format=<hex/binary> # hex for a string hex number; binary would mean the found string is directly converted to a char[] that is then directly used as the AES key (like a password)

key=<128, 192, or 256 bit>hex key or string

key_file=<path to file containing 128,192 or 256 bit hex key or string>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]