linux-ext4 - Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170719173237.GA78811@gmail.com>
Date:   Wed, 19 Jul 2017 10:32:37 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     Michael Halcrow <mhalcrow@...gle.com>
Cc:     linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-mtd@...ts.infradead.org, linux-crypto@...r.kernel.org,
        "Theodore Y . Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Alex Cope <alexcope@...gle.com>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode
 encryption keys

On Fri, Jul 14, 2017 at 09:24:40AM -0700, Michael Halcrow wrote:
> > +static int hkdf_expand(struct crypto_shash *hmac, u8 context,
> > +		       const u8 *info, unsigned int infolen,
> > +		       u8 *okm, unsigned int okmlen)
> > +{
> > +	SHASH_DESC_ON_STACK(desc, hmac);
> > +	int err;
> > +	const u8 *prev = NULL;
> > +	unsigned int i;
> > +	u8 counter = 1;
> > +	u8 tmp[HKDF_HASHLEN];
> > +
> > +	desc->tfm = hmac;
> > +	desc->flags = 0;
> > +
> > +	if (unlikely(okmlen > 255 * HKDF_HASHLEN))
> > +		return -EINVAL;
> > +
> > +	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, &context, 1);
> 
> One potential shortcut would be to just increment context on each
> iteration rather than maintain the counter.
> 

That's not a good idea because then it wouldn't be standard HKDF, and it would
be relying on the "feedback" mode to keep the HMAC inputs unique which isn't
guaranteed to be sufficient.

> >  
> > -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> > -				keysize);
> > -	if (res && inode->i_sb->s_cop->key_prefix) {
> > -		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> > -					     inode->i_sb->s_cop->key_prefix,
> > -					     keysize);
> > -		if (res2) {
> > -			if (res2 == -ENOKEY)
> > -				res = -ENOKEY;
> > +	if (ctx.version == FSCRYPT_CONTEXT_V1) {
> > +		res = find_and_derive_key_v1(inode, &ctx, derived_key,
> > +					     derived_keysize);
> 
> Why not make this consistent with the else clause, i.e. doing
> load_master_key_from_keyring() followed by derive_key_v1()?
> 

struct fscrypt_master_key contains the HMAC transform but not the raw master
key.  For the v1 key derivation we need the raw master key.  We could put it in
the fscrypt_master_key and then try to allow fscrypt_master_key's both with and
without HMAC transforms depending on the policy versions they are used for, but
there's no point in doing so currently.

Eric