| 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
| ||
|
Message-ID: <20190107153327.GB4210@linux-l9pv.suse>
Date: Mon, 7 Jan 2019 23:33:27 +0800
From: joeyli <jlee@...e.com>
To: Stephan Mueller <smueller@...onox.de>
Cc: "Lee, Chun-Yi" <joeyli.kernel@...il.com>,
"Rafael J . Wysocki" <rjw@...ysocki.net>,
Pavel Machek <pavel@....cz>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, keyrings@...r.kernel.org,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Chen Yu <yu.c.chen@...el.com>,
Oliver Neukum <oneukum@...e.com>,
Ryan Chen <yu.chen.surf@...il.com>,
David Howells <dhowells@...hat.com>,
Giovanni Gherdovich <ggherdovich@...e.cz>,
Randy Dunlap <rdunlap@...radead.org>,
Jann Horn <jannh@...gle.com>, Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler
Hi Stephan,
First, thanks for your review!
On Sun, Jan 06, 2019 at 09:01:27AM +0100, Stephan Mueller wrote:
> Am Donnerstag, 3. Januar 2019, 15:32:23 CET schrieb Lee, Chun-Yi:
>
> Hi Chun,
>
> > This patch adds a snapshot keys handler for using the key retention
> > service api to create keys for snapshot image encryption and
> > authentication.
> >
> > This handler uses TPM trusted key as the snapshot master key, and the
> > encryption key and authentication key are derived from the snapshot
> > key. The user defined key can also be used as the snapshot master key
> > , but user must be aware that the security of user key relies on user
> > space.
> >
[...snip]
> > +static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen,
> > + bool may_sleep)
> > +{
> > + struct shash_desc *desc;
> > + int err;
> > +
> > + desc = kzalloc(sizeof(struct shash_desc) +
> > + crypto_shash_descsize(hash_tfm),
> > + may_sleep ? GFP_KERNEL : GFP_ATOMIC);
>
> Why not using SHASH_DESC_ON_STACK?
>
Because security concern and bad runtime performance. Please looking at
c2cd0b08e1e patch for hibernation. And reference:
https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com/T/#u
https://lwn.net/Articles/749064/
> > + if (!desc)
> > + return -ENOMEM;
> > +
> > + desc->tfm = hash_tfm;
> > + desc->flags = may_sleep ? CRYPTO_TFM_REQ_MAY_SLEEP : 0;
> > + err = crypto_shash_digest(desc, buf, buflen, digest);
> > + shash_desc_zero(desc);
> > + kzfree(desc);
> > +
> > + return err;
> > +}
> > +
> > +static int calc_key_hash(u8 *key, unsigned int key_len, const char *salt,
> > + u8 *hash, bool may_sleep)
> > +{
> > + unsigned int salted_buf_len;
> > + u8 *salted_buf;
> > + int ret;
> > +
> > + if (!key || !hash_tfm || !hash)
> > + return -EINVAL;
> > +
> > + salted_buf_len = strlen(salt) + 1 + SNAPSHOT_KEY_SIZE;
>
> strlen on binary data? I guess that will not work. May I suggest to hand down
> the length of salt to this function?
>
hm... The salt is actually a "salt string" that's gave from
snapshot_get_auth_key() or snapshot_get_enc_key(). So I use
strlen() here. I will change the name to salt_string to avoid
confusion.
> > + salted_buf = kzalloc(salted_buf_len,
> > + may_sleep ? GFP_KERNEL : GFP_ATOMIC);
> > + if (!salted_buf)
> > + return -ENOMEM;
> > +
> > + strcpy(salted_buf, salt);
> > + memcpy(salted_buf + strlen(salted_buf) + 1, key, key_len);
> > +
> > + ret = calc_hash(hash, salted_buf, salted_buf_len, may_sleep);
> > + memzero_explicit(salted_buf, salted_buf_len);
> > + kzfree(salted_buf);
> > +
> > + return ret;
> > +}
>
> This function looks very much like a key derivation. May I strongly propose to
Actually key derivation function is modified from the get_derived_key() from
the encrypted.c file in encrypted key.
> use an official KDF type like SP800-108 or HKDF?
>
> You find the counter-KDF according to SP800-108 in security/keys/dh.c (search
> for functions *kdf*).
>
> Or we may start pulling in KDF support into the kernel crypto API via the
> patches along the line of [1].
>
> [1] http://www.chronox.de/kdf.html
>
Thanks for your suggestion. I didn't touch any key derivation standard
before. I will study it.
But I still want to use my original function currently. Because the same
logic is also used in trusted key. I will happy to move to SP800-108 or
HKDF when it's available in kernel.
> > +
> > +/* Derive authentication/encryption key */
> > +static int get_derived_key(u8 *derived_key, const char *derived_type_str,
> > + bool may_sleep)
[...snip]
> > +static int trusted_key_init(void)
> > +{
> > + struct trusted_key_payload *tkp;
> > + struct key *key;
> > + int err = 0;
> > +
> > + pr_debug("%s\n", __func__);
> > +
> > + /* find out swsusp-key */
> > + key = request_key(&key_type_trusted, skey.key_name, NULL);
> > + if (IS_ERR(key)) {
> > + pr_err("Request key error: %ld\n", PTR_ERR(key));
> > + err = PTR_ERR(key);
> > + return err;
> > + }
> > +
> > + down_write(&key->sem);
> > + tkp = key->payload.data[0];
> > + if (invalid_key(tkp->key, tkp->key_len)) {
> > + err = -EINVAL;
> > + goto key_invalid;
> > + }
> > + skey.key_len = tkp->key_len;
> > + memcpy(skey.key, tkp->key, tkp->key_len);
> > + /* burn the original key contents */
> > + memzero_explicit(tkp->key, tkp->key_len);
> > +
> > +key_invalid:
> > + up_write(&key->sem);
> > + key_put(key);
> > +
> > + return err;
> > +}
> > +
> > +static int user_key_init(void)
>
> This function and trusted_key_init look very similar - could they be collapsed
> into one function?
>
The data structure is different between trusted key with user key. I will try to
extract the duplicate part but may not collapse into one.
> > +{
> > + struct user_key_payload *ukp;
> > + struct key *key;
> > + int err = 0;
> > +
> > + pr_debug("%s\n", __func__);
> > +
> > + /* find out swsusp-key */
> > + key = request_key(&key_type_user, skey.key_name, NULL);
> > + if (IS_ERR(key)) {
> > + pr_err("Request key error: %ld\n", PTR_ERR(key));
> > + err = PTR_ERR(key);
> > + return err;
> > + }
> > +
> > + down_write(&key->sem);
> > + ukp = user_key_payload_locked(key);
> > + if (!ukp) {
> > + /* key was revoked before we acquired its semaphore */
> > + err = -EKEYREVOKED;
> > + goto key_invalid;
> > + }
> > + if (invalid_key(ukp->data, ukp->datalen)) {
> > + err = -EINVAL;
> > + goto key_invalid;
> > + }
> > + skey.key_len = ukp->datalen;
> > + memcpy(skey.key, ukp->data, ukp->datalen);
>
> Where would skey.key be destroyed again?
>
Yes, you saw it in later patch.
Thanks a lot!
Joey Lee
Powered by blists - more mailing lists