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

[Stefan Berger]

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
> > +
> > +#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?
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.
> > > >
> > > > -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
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.
> >
> >    Stefan
> These should be different options.
>
> key_format=hex/string
>
> etc.
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>

   Stefan