[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 07/17] crypto: implement the LUKS block encry
|
From: |
Daniel P. Berrange |
|
Subject: |
Re: [Qemu-block] [PATCH v2 07/17] crypto: implement the LUKS block encryption format |
|
Date: |
Mon, 8 Feb 2016 16:03:35 +0000 |
|
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Fri, Feb 05, 2016 at 10:38:45AM -0700, Eric Blake wrote:
> On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> > Provide a block encryption implementation that follows the
> > LUKS/dm-crypt specification.
> >
> > This supports all combinations of hash, cipher algorithm,
> > cipher mode and iv generator that are implemented by the
> > current crypto layer.
> >
> > The notable missing feature is support for the 'xts'
> > cipher mode, which is commonly used for disk encryption
> > instead of 'cbc'. This is because it is not provided by
> > either nettle or libgcrypt. A suitable implementation
> > will be identified & integrated later.
> >
> > There is support for opening existing volumes formatted
> > by dm-crypt, and for formatting new volumes. In the latter
> > case it will only use key slot 0.
> >
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > +
> > +/*
> > + * Reference for format is
> > + *
> > + * docs/on-disk-format.pdf
> > + *
> > + * In 'cryptsetup' package source code
> > + *
>
> Worth calling out the document version number? I reviewed based on
> version 1.2.1, dated Oct 2011.
Yep
> > +};
> > +
> > +
> > +struct QCryptoBlockLUKSKeySlot {
> > + /* state of keyslot, enabled/disable */
> > + uint32_t active;
>
> May be worth a comment that this struct is written to disk in
> big-endian, but used in native-endian for most operations for
> convenience; to make sure future developers remember to put byteswaps in
> the correct locations. But it looks like you handled endianness correctly.
Yes, will note it.
>
> > + /* iterations for PBKDF2 */
> > + uint32_t iterations;
> > + /* salt for PBKDF2 */
> > + uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
> > + /* start sector of key material */
> > + uint32_t key_offset;
> > + /* number of anti-forensic stripes */
> > + uint32_t stripes;
> > +} QEMU_PACKED;
>
> Packed makes sense since we are writing it straight to disk; although it
> looks like natural alignment prevails and this did not need to be
> packed. Do we want some sort of BUG_ON() that we have the right size
> struct?
Yes, I'll add QEMU_BUILD_BUG_ON
> > +
> > +struct QCryptoBlockLUKSHeader {
> > + /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */
> > + char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN];
> > +
> > + /* LUKS version, currently 1 */
> > + uint16_t version;
> > +
> > + /* cipher name specification (aes, etc */
>
> Missing )
>
> > + char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN];
> > +
> > + /* cipher mode specification () */
>
> text in the ()?
>
> > + char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN];
> > +
> > + /* hash specication () */
>
> s/specication/specification/, text in the ()?
Will fix all these examples
>
> > + char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN];
> > +
> > + /* start offset of the volume data (in sectors) */
> > + uint32_t payload_offset;
>
> Should we say "in 512-byte sectors"? I could not see anything in the
> LUKS specification that said whether SECTOR_SIZE can be larger than 512
> - if you have a bare metal 4k sector disk, does a payload_offset of 2
> mean 1024 or 8192 bytes into the disk? Eww. Sounds like we should
> submit a bug against the LUKS spec to have it clarified.
cryptsetup is hardcoded to use 512 byte sectors currently. IIUC
they'll need a LUKS spec update before doing 4k sectors natively.
>
> > +
> > + /* Number of key bytes */
> > + uint32_t key_bytes;
> > +
> > + /* master key checksum after PBKDF2 */
> > + uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
> > +
> > + /* salt for master key PBKDF2 */
> > + uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
> > +
> > + /* iterations for master key PBKDF2 */
> > + uint32_t master_key_iterations;
> > +
> > + /* UUID of the partition */
>
> Should we say "in standard ASCII representation"?
>
> > + uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN];
> > +
> > + /* key slots */
> > + QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS];
> > +} QEMU_PACKED;
>
> Again, alignment should mean that we could get away without packing; but
> the packing doesn't hurt; and a BUG_ON may be nice to ensure size.
Yep, add QEMU_BUILD_BUG_ON
> > +/* XXX doesn't QEMU already have a string -> enum val convertor ? */
> > +static int qcrypto_block_luks_name_lookup(const char *name,
>
> Yes, spelled qapi_enum_parse(). Works if the enum was defined in a
> .json file.
>
>
> > +
> > + error_setg(errp, "%s %s not supported", type, name);
> > + return 0;
> > +}
> > +
> > +#define qcrypto_block_luks_cipher_mode_lookup(name, errp) \
> > + qcrypto_block_luks_name_lookup(name, \
> > + QCryptoCipherMode_lookup, \
> > + QCRYPTO_CIPHER_MODE__MAX, \
> > + "Cipher mode", \
> > + errp)
> > +
> > +#define qcrypto_block_luks_hash_name_lookup(name, errp) \
> > + qcrypto_block_luks_name_lookup(name, \
> > + QCryptoHashAlgorithm_lookup, \
> > + QCRYPTO_HASH_ALG__MAX, \
> > + "Hash algorithm", \
> > + errp)
>
> That said, your macro wrappers tweak the error message, so that it is
> perhaps more legible than the standard failure mode of
> qapi_enum_parse(). So up to you if you want to keep the wrappers.
I'll change my XXX note to mention qapi_enum_parse() as a future
todo if we can improve its error reporting in some manner.
> > +static int
> > +qcrypto_block_luks_load_key(QCryptoBlock *block,
> > + QCryptoBlockLUKSKeySlot *slot,
> > + const char *password,
> > + QCryptoCipherAlgorithm cipheralg,
> > + QCryptoCipherMode ciphermode,
> > + QCryptoHashAlgorithm hash,
> > + QCryptoIVGenAlgorithm ivalg,
> > + QCryptoHashAlgorithm ivhash,
> > + uint8_t *masterkey,
> > + size_t masterkeylen,
> > + QCryptoBlockReadFunc readfunc,
> > + void *opaque,
> > + Error **errp)
> > +{
> > + QCryptoBlockLUKS *luks = block->opaque;
> > + uint8_t *splitkey;
> > + size_t splitkeylen;
> > + uint8_t *possiblekey;
> > + int ret = -1;
> > + ssize_t rv;
> > + QCryptoCipher *cipher = NULL;
> > + uint8_t keydigest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
> > + QCryptoIVGen *ivgen = NULL;
> > + size_t niv;
> > +
> > + if (slot->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
> > + return 0;
> > + }
>
> Would be worth documenting the convention of '0' (no key found, but no
> errp set), '1' (successful key), and negative (errp set).
Yes, will document that.
> > + splitkeylen = masterkeylen * slot->stripes;
>
> Should we do any validation that masterkeylen was in bounds and this
> doesn't overflow? I don't want a malicious byte stream masquerading as
> a LUKS header to cause us to misbehave.
masterkeylen comes from luks->header.key_bytes. We should put some
validation in for that earlier when we first get it off disk.
> For that matter, the spec states that luks->payload_offset is a
> write-once calculation at creation time, and that it could be recomputed
> from key layout. Should we validate that the offset listed makes
> algorithmic sense with what we know about header and key data sizes for
> the parameters listed in the header?
While it says that the offsets could be recomputed, from a technical POV
there's no reason why all impls must use the same layout / alignment for
the key slots. So I feel it safer to just rely on the header values and
not try to enforce that they match a particular offset.
> Side note: I'm a bit surprised the upstream LUKS spec does not include
> any checksum over the header contents - it does a great job at ensuring
> the master key is only going to be found by someone knowing a correct
> password and that a single byte error in a key material section
> invalidates that entire slot's usefulness, but it is not as careful on
> what happens with certain single-byte errors in the header.
>
> > + splitkey = g_new0(uint8_t, splitkeylen);
> > + possiblekey = g_new0(uint8_t, masterkeylen);
> > +
> > + /*
> > + * The user password is used to generate a (possible)
> > + * decryption key. This may or may not successfully
> > + * decrypt the master key - we just blindly assume
> > + * the key is correct and validate the results of
> > + * decryption later.
> > + */
> > + if (qcrypto_pbkdf2(hash,
> > + (const uint8_t *)password, strlen(password),
> > + slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN,
> > + slot->iterations,
> > + possiblekey, masterkeylen,
> > + errp) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + /*
> > + * We need to read the master key material from the
> > + * LUKS key material header. What we're reading is
> > + * not the raw master key, but rather the data after
> > + * it has been passed through AFSplit and the result
> > + * then encrypted.
> > + */
> > + rv = readfunc(block,
> > + slot->key_offset * 512ll,
>
> I tend to write LL rather than ll, to make it obvious I didn't typo '1'.
> Looks like we're hard-coding our sector size (not the first time); but
> maybe a constant instead of a magic number will make it easier for the
> poor guy down the road that tries to switch to 4k sectors.
I'll introduce a QCRYPTO_LUKS_SECTOR_SIZE constant throughout for
now.
> > + * it back together to get the actual master key.
> > + */
> > + if (qcrypto_afsplit_decode(hash,
>
> ...
> > +
> > + if (memcmp(keydigest, luks->header.master_key_digest,
> > + QCRYPTO_BLOCK_LUKS_DIGEST_LEN) == 0) {
> > + /* Success, we got the right master key */
> > + ret = 1;
> > + goto cleanup;
> > + }
> > +
> > + /* Fail, user's password was not valid for this key slot,
> > + * tell caller to try another slot */
> > + ret = 0;
> > +
>
> Matches the spec. Yay!
>
> Side observation: The LUKS spec doesn't say anything about parallelizing
> the lookup loop across all 8 key slots, nor any randomization on which
> slot should be attempted first. The whole point of PBKDF2 iteration
> count is to burn CPU time so that an attacker can't brute force things
> easily. That means it is VERY easy to tell by timing how many active
> slots were attempted before a key was found, if slots are tried serially
> and we immediately break the loop on the first successful decryption.
> Is there any information leak caused by the timing observations when
> serially searching among 8 active slots, when the master key is only
> found in slot 7 vs. slot 0? But I don't see it as your problem to
> solve, so much as something to ask the upstream LUKS design team.
FWIW, cryptsetup will short circuit the search as soon as it finds
a valid slot, so we're matching their behaviour at least.
> > +static int
> > +qcrypto_block_luks_open(QCryptoBlock *block,
> > + QCryptoBlockOpenOptions *options,
> > + QCryptoBlockReadFunc readfunc,
> > + void *opaque,
> > + unsigned int flags,
> > + Error **errp)
> > +{
>
> > + /*
> > + * The cipher_mode header contains a string that we have
> > + * to further parse of the format
>
> s/parse/parse,/
>
> > + *
> > + * <cipher-mode>-<iv-generator>[:<iv-hash>]
> > + *
> > + * eg cbc-essiv:sha256, cbc-plain64
> > + */
> > + ivgen_name = strchr(luks->header.cipher_mode, '-');
> > + if (!ivgen_name) {
> > + ret = -EINVAL;
> > + error_setg(errp, "Unexpected cipher mode string format %s",
> > + luks->header.cipher_mode);
> > + goto fail;
> > + }
> > + *ivgen_name = '\0';
> > + ivgen_name++;
>
> We're modifying luks->header in place; we'd better not write it back out
> to disk in the modified form. I guess you are okay for now - your code
> only reads existing LUKS disks, and can only create a single key in slot
> 0 on conversion (that is, I don't see code for key management of an
> existing image). Probably things we should add later, though, at which
> point we'll need to be careful that we don't overwrite too much in the
> header.
Yeah, certainly something to be careful of later.
> > + block->payload_offset = luks->header.payload_offset;
>
> Earlier, I argued that block->payload_offset should be in bytes. You
> are constrained that luks->header.payload_offset is in sectors, but we
> need not propagate that craziness any higher.
Yes, I've made that change.
> > +static int
> > +qcrypto_block_luks_create(QCryptoBlock *block,
> > + QCryptoBlockCreateOptions *options,
> > + QCryptoBlockInitFunc initfunc,
> > + QCryptoBlockWriteFunc writefunc,
> > + void *opaque,
> > + Error **errp)
> > +{
>
> > +
> > + memcpy(&luks_opts, options->u.luks, sizeof(luks_opts));
> > + if (!luks_opts.has_cipher_alg) {
> > + luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
> > + }
> > + if (!luks_opts.has_cipher_mode) {
> > + /* XXX switch to XTS */
> > + luks_opts.cipher_mode = QCRYPTO_CIPHER_MODE_CBC;
> > + }
>
> Yeah, my (quick) reading made it seem like XTS is a slightly more secure
> default than CBC. But as you said, XTS implementation will come later.
Yes, I still hope to post an XTS impl before 2.6, so if this code
merges before 2.6, hopefully we'll stil have time to use XTS as
the default too, matching cryptsetup
> > + luks = g_new0(QCryptoBlockLUKS, 1);
> > + block->opaque = luks;
> > +
> > + memcpy(luks->header.magic, qcrypto_block_luks_magic,
> > + QCRYPTO_BLOCK_LUKS_MAGIC_LEN);
> > +
> > + luks->header.version = QCRYPTO_BLOCK_LUKS_VERSION;
>
> Maybe a comment here that endianness will be addressed later, as a lot
> of code appears between here and the actual write-to-disk.
Will do
> > + uuid_generate(uuid);
> > + uuid_unparse(uuid, (char *)luks->header.uuid);
> > +
> > + switch (luks_opts.cipher_alg) {
> > + case QCRYPTO_CIPHER_ALG_AES_128:
> > + case QCRYPTO_CIPHER_ALG_AES_192:
> > + case QCRYPTO_CIPHER_ALG_AES_256:
> > + cipher_alg = "aes";
> > + break;
> > + default:
> > + error_setg(errp, "Cipher algorithm is not supported");
> > + goto error;
>
> That is, we aren't supporting "twofish", "serpent", "cast5", or "cast6".
> Fine for now.
>
> > + }
> > +
> > + cipher_mode = QCryptoCipherMode_lookup[luks_opts.cipher_mode];
> > + ivgen_alg = QCryptoIVGenAlgorithm_lookup[luks_opts.ivgen_alg];
> > + if (luks_opts.has_ivgen_hash_alg) {
> > + ivgen_hash_alg =
> > QCryptoHashAlgorithm_lookup[luks_opts.ivgen_hash_alg];
> > + cipher_mode_spec = g_strdup_printf("%s-%s:%s", cipher_mode,
> > ivgen_alg,
> > + ivgen_hash_alg);
> > + } else {
> > + cipher_mode_spec = g_strdup_printf("%s-%s", cipher_mode,
> > ivgen_alg);
> > + }
>
> Should we validate that the mode spec is among the set documented by the
> LUKS spec ("ecb", "cbc-plain", "cbc-essiv:hash", "xts-plain64") (well,
> we don't support xts yet), and reject combinations that aren't supported
> ("cbc-plain64" comes to mind as something the LUKS spec doesn't allow,
> even though we have the pieces to logically make it happen)?
The spec appears to be non-exhaustive, in that cryptsetup / linux allow
for cbc-plain64 and other combinations that aren't listed. So QEMU should
follow that for interoperability.
> > + hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.hash_alg];
>
> Likewise, should we validate that the hash is one of the specified names
> ("sha1", "sha256", "sha512", "ripemd160")?
The only one we're permitting that's not listed there is 'md5' and
that should fail anyway since it provides too small a digest size
but I see we're not validating the digest size.
> > +
> > + memcpy(luks->header.cipher_name, cipher_alg,
> > + strlen(cipher_alg) + 1);
> > + memcpy(luks->header.cipher_mode, cipher_mode_spec,
> > + strlen(cipher_mode_spec) + 1);
> > + memcpy(luks->header.hash_spec, hash_alg,
> > + strlen(hash_alg) + 1);
>
> Couldn't these just be strcpy()?
Yes
> > + /* Although LUKS has multiple key slots, we're just going
> > + * to use the first key slot */
> > +
> > + splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;
>
> The LUKS spec says that it is okay to allow the user to specify stripes.
> Should that be one of our options, with a sane default? But it can be
> added later as a followup, this patch is already quite big and I'm fine
> with hardcoding stripes for now.
Cryptsetup doesn't allow the user to specify the stripes value, just
fixing it at 4k, so I figure we're best todo the same, otherwise we'll
create images that cant be read by linux.
>
> > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > + luks->header.key_slots[i].active = i == 0 ?
> > + QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED :
> > + QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > + luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> > + luks->header.key_slots[i].key_offset =
> > + (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512) +
> > + (ROUND_UP(((splitkeylen + 511) / 512),
> > + (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512)) * i);
>
> Eww. The spec seems buggy, saying:
>
> > // integer divisions, result rounded down:
> > baseOffset = (size of phdr)/SECTOR SIZE + 1
> > keymaterialSectors = (stripes*masterKeyLength)/SECTOR SIZE + 1
>
> First, look at the calculation of baseOffset. Since size of phdr is 592
> bytes, and that is NOT a SECTOR SIZE in any disk I know of, that makes
> sense that baseOffset will be at least 2 (512-byte) sectors into the
> disk, or, if SECTOR SIZE is 4, that will result in an offset of 1
> (4096-byte) sector. However, you've defined
> QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET as 4096, which means for our 512-byte
> sector usage, you are setting the first key material at sector 4 no
> matter what. I guess that matches what cryptsetup does? Maybe worth a
> comment?
>
> Now, look at the calculation of keymaterial Sectors. The algorithm in
> the spec mishandles the case where stripes happens to be an exact
> multiple of sector size (that is, for a keysize of 20 bytes coupled with
> 512 stripes, it would reserve 21 sectors rather than the 20 actually
> required). I think your use of ROUND_UP() makes more sense, but it
> would be nice to file a bug against the LUKS spec to have them fix their
> math, and/or document that it is okay to use values larger than the
> absolute minimum.
Yeah, what I am doing matches the cryptsetup code. I had originally
written it to be more or less what the spec suggests, and noticed
that my images were different from those created by cryptsetup. So
I switched to use the same algorithm. I figure it is probably an
optimization for on disk alignment to stop crossing sector boundaries.
> > + * slot headers, rounded up to the nearest sector, combined with
> > + * the size of each master key material region, also rounded up
> > + * to the nearest sector */
> > + luks->header.payload_offset =
> > + (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512) +
> > + (ROUND_UP(((splitkeylen + 511) / 512),
> > + (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512)) *
> > + QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +
> > + block->payload_offset = luks->header.payload_offset;
>
> Again, I argue that block->payload_offset would be saner in bytes.
Agreed
> > + /* Reserve header space to match payload offset */
> > + initfunc(block, block->payload_offset * 512, &local_err, opaque);
>
> Especially since initfunc() takes bytes.
>
> > +
> > +static int
> > +qcrypto_block_luks_decrypt(QCryptoBlock *block,
> > + uint64_t startsector,
> > + uint8_t *buf,
> > + size_t len,
> > + Error **errp)
> > +{
> > + return qcrypto_block_decrypt_helper(block->cipher,
> > + block->niv, block->ivgen,
> > + startsector, buf, len, errp);
> > +}
>
> What happens when we try to read a LUKS device with 4k sectors? Worth
> worrying about, maybe just as a comment that we are hard-coded to 512
> bytes at the moment?
qcrypto_block_decrypt_helper() method processes 'buf' in units of 512
so even if the underling dev is 4k, we'll also do 512 byte I/O
> > ##
> > +# QCryptoBlockOptionsLUKS:
> > +#
> > +# The options that apply to LUKS encryption format
> > +#
> > +# @key-secret: #optional the ID of a QCryptoSecret object providing the
> > +# decryption key
>
> Maybe add "Mandatory except when probing the image for metadata only"
Yes
> > +# Since: 2.6
> > +##
> > +{ 'struct': 'QCryptoBlockOptionsLUKS',
> > + 'data': { '*key-secret': 'str' }}
> > +
> > +
> > +##
> > +# QCryptoBlockCreateOptionsLUKS:
> > +#
> > +# The options that apply to LUKS encryption format initialization
> > +#
> > +# @cipher-alg: #optional the cipher algorithm for data encryption
> > +# @cipher-mode: #optional the cipher mode for data encryption
> > +# @ivgen-alg: #optional the initialization vector generator
> > +# @ivgen-hash-alg: #optional the initialization vector generator hash
> > +# @hash-alg: #optional the master key hash algorithm
>
> Would be nice to mention the defaults, particularly since we may later
> change the default from cbc to xts.
Yes, will do
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|