| Message ID | 20190726224141.14044-10-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | fscrypt: key management improvements | expand |
On Fri, Jul 26, 2019 at 03:41:34PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Add an implementation of HKDF (RFC 5869) to fscrypt, for the purpose of > deriving additional key material from the fscrypt master keys for v2 > encryption policies. HKDF is a key derivation function built on top of > HMAC. We choose SHA-512 for the underlying unkeyed hash, and use an > "hmac(sha512)" transform allocated from the crypto API. > > We'll be using this to replace the AES-ECB based KDF currently used to > derive the per-file encryption keys. While the AES-ECB based KDF is > believed to meet the original security requirements, it is nonstandard > and has problems that don't exist in modern KDFs such as HKDF: > > 1. It's reversible. Given a derived key and nonce, an attacker can > easily compute the master key. This is okay if the master key and > derived keys are equally hard to compromise, but now we'd like to be > more robust against threats such as a derived key being compromised > through a timing attack, or a derived key for an in-use file being > compromised after the master key has already been removed. > > 2. It doesn't evenly distribute the entropy from the master key; each 16 > input bytes only affects the corresponding 16 output bytes. > > 3. It isn't easily extensible to deriving other values or keys, such as > a public hash for securely identifying the key, or per-mode keys. > Per-mode keys will be immediately useful for Adiantum encryption, for > which fscrypt currently uses the master key directly, introducing > unnecessary usage constraints. Per-mode keys will also be useful for > hardware inline encryption, which is currently being worked on. > > HKDF solves all the above problems. > > Signed-off-by: Eric Biggers <ebiggers@google.com> Unless I missed something there's nothing here which is fscrypt specific. Granted that it's somewhat unlikely that someone would want to implement (the very bloated) IKE from IPSEC in the kernel, I wonder if there might be other users of HKDF, and whether this would be better placed in lib/ or crypto/ instead of fs/crypto? Other than that, looks good. Feel free to add: Reviewed-by: Theodore Ts'o <tytso@mit.edu>
On Sun, Jul 28, 2019 at 03:39:49PM -0400, Theodore Y. Ts'o wrote: > On Fri, Jul 26, 2019 at 03:41:34PM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > Add an implementation of HKDF (RFC 5869) to fscrypt, for the purpose of > > deriving additional key material from the fscrypt master keys for v2 > > encryption policies. HKDF is a key derivation function built on top of > > HMAC. We choose SHA-512 for the underlying unkeyed hash, and use an > > "hmac(sha512)" transform allocated from the crypto API. > > > > We'll be using this to replace the AES-ECB based KDF currently used to > > derive the per-file encryption keys. While the AES-ECB based KDF is > > believed to meet the original security requirements, it is nonstandard > > and has problems that don't exist in modern KDFs such as HKDF: > > > > 1. It's reversible. Given a derived key and nonce, an attacker can > > easily compute the master key. This is okay if the master key and > > derived keys are equally hard to compromise, but now we'd like to be > > more robust against threats such as a derived key being compromised > > through a timing attack, or a derived key for an in-use file being > > compromised after the master key has already been removed. > > > > 2. It doesn't evenly distribute the entropy from the master key; each 16 > > input bytes only affects the corresponding 16 output bytes. > > > > 3. It isn't easily extensible to deriving other values or keys, such as > > a public hash for securely identifying the key, or per-mode keys. > > Per-mode keys will be immediately useful for Adiantum encryption, for > > which fscrypt currently uses the master key directly, introducing > > unnecessary usage constraints. Per-mode keys will also be useful for > > hardware inline encryption, which is currently being worked on. > > > > HKDF solves all the above problems. > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > Unless I missed something there's nothing here which is fscrypt > specific. Granted that it's somewhat unlikely that someone would want > to implement (the very bloated) IKE from IPSEC in the kernel, I wonder > if there might be other users of HKDF, and whether this would be > better placed in lib/ or crypto/ instead of fs/crypto? > This is standard HKDF-SHA512; only the choice of parameters is fscrypt-specific. So it could indeed use a common implementation of HKDF if one were available. However, I don't think there are any other HKDF users in the kernel currently. Also, while there was a patch to support HKDF via the crypto_rng API, there was no consensus about whether this was actually the best way to add KDF support: https://lore.kernel.org/linux-crypto/2423373.Zd5ThvQH5g@positron.chronox.de So for now, to avoid unnecessarily blocking this patchset I think we should just go with this implementation in fs/crypto/. It can always be changed later, once we decide on the best way to add KDFs to the crypto API. [To be clear: this patch already uses "hmac(sha512)" from the crypto API. It's only the actual HKDF part that we're talking about here. Also, its correctness is tested by the ciphertext verification xfstests.] - Eric
On Mon, 2019-07-29 at 13:29 -0700, Eric Biggers wrote: > On Sun, Jul 28, 2019 at 03:39:49PM -0400, Theodore Y. Ts'o wrote: > > On Fri, Jul 26, 2019 at 03:41:34PM -0700, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> [...] > > > HKDF solves all the above problems. > > > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > Unless I missed something there's nothing here which is fscrypt > > specific. Granted that it's somewhat unlikely that someone would > > want to implement (the very bloated) IKE from IPSEC in the kernel, > > I wonder if there might be other users of HKDF, and whether this > > would be better placed in lib/ or crypto/ instead of fs/crypto? > > This is standard HKDF-SHA512; only the choice of parameters is > fscrypt-specific. So it could indeed use a common implementation of > HKDF if one were available. > > However, I don't think there are any other HKDF users in the kernel > currently. Well, I'm still trying to add the TPM ones, but they're based on SP800- 108 for arbitrary keys and SP800-56A for elliptic curve ones. These are similar to the RFC5869 except that they do extract/expand in a single operation. Plus, of course, the TPM mandates we use the name algorithm (usually sha256, which is what I hardcoded) as the hash. Note: since you don't use the extract step either in your implementation, effectively you're equivalent to SP800-108 as well. This is effectively the same reason as the TPM: we need deterministic keys, so we've nowhere to get the salt from that would persist. > Also, while there was a patch to support HKDF via the crypto_rng API, > there was no consensus about whether this was actually the best way > to add KDF support: > https://lore.kernel.org/linux-crypto/2423373.Zd5ThvQH5g@positron.chro > nox.de > > So for now, to avoid unnecessarily blocking this patchset I think we > should just go with this implementation in fs/crypto/. It can always > be changed later, once we decide on the best way to add KDFs to the > crypto API. > > [To be clear: this patch already uses "hmac(sha512)" from the crypto > API. It's only the actual HKDF part that we're talking about here. Right, once you have the hmac + hash available, the rest is easy, so this is what we have for the TPM KDFa: static void KDFa(u8 *key, int keylen, const char *label, u8 *u, u8 *v, int bytes, u8 *out) { u32 counter; const __be32 bits = cpu_to_be32(bytes * 8); for (counter = 1; bytes > 0; bytes -= SHA256_DIGEST_SIZE, counter++, out += SHA256_DIGEST_SIZE) { SHASH_DESC_ON_STACK(desc, sha256_hash); __be32 c = cpu_to_be32(counter); hmac_init(desc, key, keylen); crypto_shash_update(desc, (u8 *)&c, sizeof(c)); crypto_shash_update(desc, label, strlen(label)+1); crypto_shash_update(desc, u, SHA256_DIGEST_SIZE); crypto_shash_update(desc, v, SHA256_DIGEST_SIZE); crypto_shash_update(desc, (u8 *)&bits, sizeof(bits)); hmac_final(desc, key, keylen, out); } } I honestly think these things are so simplistic with the correct hmac that it would make it more confusing to try to produce a general KDF than it would simply to hard code them where we need them. James
diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig index 5fdf24877c178..ff5a1746cbae4 100644 --- a/fs/crypto/Kconfig +++ b/fs/crypto/Kconfig @@ -7,6 +7,8 @@ config FS_ENCRYPTION select CRYPTO_ECB select CRYPTO_XTS select CRYPTO_CTS + select CRYPTO_SHA512 + select CRYPTO_HMAC select KEYS help Enable encryption of files and directories. This diff --git a/fs/crypto/Makefile b/fs/crypto/Makefile index a640d486800da..3333e2ac859fa 100644 --- a/fs/crypto/Makefile +++ b/fs/crypto/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_FS_ENCRYPTION) += fscrypto.o fscrypto-y := crypto.o \ fname.o \ + hkdf.o \ hooks.o \ keyring.o \ keysetup.o \ diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 3616232b4798e..92567efec2cd5 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -172,6 +172,21 @@ extern bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len, u32 max_len, u32 *encrypted_len_ret); +/* hkdf.c */ + +struct fscrypt_hkdf { + struct crypto_shash *hmac_tfm; +}; + +extern int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, + unsigned int master_key_size); + +extern int fscrypt_hkdf_expand(struct fscrypt_hkdf *hkdf, u8 context, + const u8 *info, unsigned int infolen, + u8 *okm, unsigned int okmlen); + +extern void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf); + /* keyring.c */ /* diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c new file mode 100644 index 0000000000000..f21873e1b4674 --- /dev/null +++ b/fs/crypto/hkdf.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation + * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010): + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme". + * + * This is used to derive keys from the fscrypt master keys. + * + * Copyright 2019 Google LLC + */ + +#include <crypto/hash.h> +#include <crypto/sha.h> + +#include "fscrypt_private.h" + +/* + * HKDF supports any unkeyed cryptographic hash algorithm, but fscrypt uses + * SHA-512 because it is reasonably secure and efficient; and since it produces + * a 64-byte digest, deriving an AES-256-XTS key preserves all 64 bytes of + * entropy from the master key and requires only one iteration of HKDF-Expand. + */ +#define HKDF_HMAC_ALG "hmac(sha512)" +#define HKDF_HASHLEN SHA512_DIGEST_SIZE + +/* + * HKDF consists of two steps: + * + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from + * the input keying material and optional salt. + * 2. HKDF-Expand: expand the pseudorandom key into output keying material of + * any length, parameterized by an application-specific info string. + * + * HKDF-Extract can be skipped if the input is already a pseudorandom key of + * length HKDF_HASHLEN bytes. However, cipher modes other than AES-256-XTS take + * shorter keys, and we don't want to force users of those modes to provide + * unnecessarily long master keys. Thus fscrypt still does HKDF-Extract. No + * salt is used, since fscrypt master keys should already be pseudorandom and + * there's no way to persist a random salt per master key from kernel mode. + */ + +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */ +static int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm, + unsigned int ikmlen, u8 prk[HKDF_HASHLEN]) +{ + static const u8 default_salt[HKDF_HASHLEN]; + SHASH_DESC_ON_STACK(desc, hmac_tfm); + int err; + + err = crypto_shash_setkey(hmac_tfm, default_salt, HKDF_HASHLEN); + if (err) + return err; + + desc->tfm = hmac_tfm; + err = crypto_shash_digest(desc, ikm, ikmlen, prk); + shash_desc_zero(desc); + return err; +} + +/* + * Compute HKDF-Extract using the given master key as the input keying material, + * and prepare an HMAC transform object keyed by the resulting pseudorandom key. + * + * Afterwards, the keyed HMAC transform object can be used for HKDF-Expand many + * times without having to recompute HKDF-Extract each time. + */ +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, + unsigned int master_key_size) +{ + struct crypto_shash *hmac_tfm; + u8 prk[HKDF_HASHLEN]; + int err; + + hmac_tfm = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0); + if (IS_ERR(hmac_tfm)) { + fscrypt_err(NULL, "Error allocating " HKDF_HMAC_ALG ": %ld", + PTR_ERR(hmac_tfm)); + return PTR_ERR(hmac_tfm); + } + + if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != sizeof(prk))) { + err = -EINVAL; + goto err_free_tfm; + } + + err = hkdf_extract(hmac_tfm, master_key, master_key_size, prk); + if (err) + goto err_free_tfm; + + err = crypto_shash_setkey(hmac_tfm, prk, sizeof(prk)); + if (err) + goto err_free_tfm; + + hkdf->hmac_tfm = hmac_tfm; + goto out; + +err_free_tfm: + crypto_free_shash(hmac_tfm); +out: + memzero_explicit(prk, sizeof(prk)); + return err; +} + +/* + * HKDF-Expand (RFC 5869 section 2.3). This expands the pseudorandom key, which + * was already keyed into 'hkdf->hmac_tfm' by fscrypt_init_hkdf(), into 'okmlen' + * bytes of output keying material parameterized by the application-specific + * 'info' of length 'infolen' bytes, prefixed by "fscrypt\0" and the 'context' + * byte. This is thread-safe and may be called by multiple threads in parallel. + * + * ('context' isn't part of the HKDF specification; it's just a prefix fscrypt + * adds to its application-specific info strings to guarantee that it doesn't + * accidentally repeat an info string when using HKDF for different purposes.) + */ +int fscrypt_hkdf_expand(struct fscrypt_hkdf *hkdf, u8 context, + const u8 *info, unsigned int infolen, + u8 *okm, unsigned int okmlen) +{ + SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm); + u8 prefix[9]; + unsigned int i; + int err; + const u8 *prev = NULL; + u8 counter = 1; + u8 tmp[HKDF_HASHLEN]; + + if (WARN_ON(okmlen > 255 * HKDF_HASHLEN)) + return -EINVAL; + + desc->tfm = hkdf->hmac_tfm; + + memcpy(prefix, "fscrypt\0", 8); + prefix[8] = context; + + for (i = 0; i < okmlen; i += HKDF_HASHLEN) { + + err = crypto_shash_init(desc); + if (err) + goto out; + + if (prev) { + err = crypto_shash_update(desc, prev, HKDF_HASHLEN); + if (err) + goto out; + } + + err = crypto_shash_update(desc, prefix, sizeof(prefix)); + if (err) + goto out; + + err = crypto_shash_update(desc, info, infolen); + if (err) + goto out; + + BUILD_BUG_ON(sizeof(counter) != 1); + if (okmlen - i < HKDF_HASHLEN) { + err = crypto_shash_finup(desc, &counter, 1, tmp); + if (err) + goto out; + memcpy(&okm[i], tmp, okmlen - i); + memzero_explicit(tmp, sizeof(tmp)); + } else { + err = crypto_shash_finup(desc, &counter, 1, &okm[i]); + if (err) + goto out; + } + counter++; + prev = &okm[i]; + } + err = 0; +out: + if (unlikely(err)) + memzero_explicit(okm, okmlen); /* so caller doesn't need to */ + shash_desc_zero(desc); + return err; +} + +void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf) +{ + crypto_free_shash(hkdf->hmac_tfm); +}