From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52409) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aSoIA-0003Cy-7l for qemu-devel@nongnu.org; Mon, 08 Feb 2016 11:04:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aSoI3-0006mY-0v for qemu-devel@nongnu.org; Mon, 08 Feb 2016 11:03:58 -0500 Date: Mon, 8 Feb 2016 16:03:35 +0000 From: "Daniel P. Berrange" Message-ID: <20160208160335.GD20236@redhat.com> References: <1453311539-1193-1-git-send-email-berrange@redhat.com> <1453311539-1193-8-git-send-email-berrange@redhat.com> <56B4DE25.6040900@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56B4DE25.6040900@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 07/17] crypto: implement the LUKS block encryption format Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org 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 > > + > > +/* > > + * 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,/ > > > + * > > + * -[:] > > + * > > + * 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 :|