| 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: <CAE=gft41=5uWwPfDZ=nyjcOzk21YCAeg6cheUNy-m0j79CgNfQ@mail.gmail.com>
Date: Wed, 9 Nov 2022 16:30:10 -0800
From: Evan Green <evgreen@...omium.org>
To: Kees Cook <keescook@...omium.org>
Cc: linux-kernel@...r.kernel.org, corbet@....net,
linux-pm@...r.kernel.org, rjw@...ysocki.net, gwendal@...omium.org,
apronin@...omium.org, Pavel Machek <pavel@....cz>,
Matthew Garrett <mgarrett@...ora.tech>,
linux-integrity@...r.kernel.org, jejb@...ux.ibm.com,
zohar@...ux.ibm.com, dlunev@...gle.com,
Eric Biggers <ebiggers@...nel.org>,
Ben Boeckel <me@...boeckel.net>, jarkko@...nel.org,
Len Brown <len.brown@...el.com>,
"Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v4 09/11] PM: hibernate: Mix user key in encrypted hibernate
On Fri, Nov 4, 2022 at 11:54 AM Kees Cook <keescook@...omium.org> wrote:
>
> On Thu, Nov 03, 2022 at 11:01:17AM -0700, Evan Green wrote:
> > Usermode may have their own data protection requirements when it comes
> > to encrypting the hibernate image. For example, users may want a policy
> > where the hibernate image is protected by a key derived both from
> > platform-level security as well as authentication data (such as a
> > password or PIN). This way, even if the platform is compromised (ie a
> > stolen laptop), sensitive data cannot be exfiltrated via the hibernate
> > image without additional data (like the user's password).
> >
> > The kernel is already doing the encryption, but will be protecting its
> > key with the TPM alone. Allow usermode to mix in key content of their own
> > for the data portion of the hibernate image, so that the image
> > encryption key is determined both by a TPM-backed secret and
> > user-defined data.
> >
> > To mix the user key in, we hash the kernel key followed by the user key,
> > and use the resulting hash as the new key. This allows usermode to mix
> > in its key material without giving it too much control over what key is
> > actually driving the encryption (which might be used to attack the
> > secret kernel key).
> >
> > Limiting this to the data portion allows the kernel to receive the page
> > map and prepare its giant allocation even if this user key is not yet
> > available (ie the user has not yet finished typing in their password).
> > Once the user key becomes available, the data portion can be pushed
> > through to the kernel as well. This enables "preloading" scenarios,
> > where the hibernate image is loaded off of disk while the additional
> > key material (eg password) is being collected.
> >
> > One annoyance of the "preloading" scheme is that hibernate image memory
> > is effectively double-allocated: first by the usermode process pulling
> > encrypted contents off of disk and holding it, and second by the kernel
> > in its giant allocation in prepare_image(). An interesting future
> > optimization would be to allow the kernel to accept and store encrypted
> > page data before the user key is available. This would remove the
> > double allocation problem, as usermode could push the encrypted pages
> > loaded from disk immediately without storing them. The kernel could defer
> > decryption of the data until the user key is available, while still
> > knowing the correct page locations to store the encrypted data in.
> >
> > Signed-off-by: Evan Green <evgreen@...omium.org>
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - Add missing static on snapshot_encrypted_byte_count()
> > - Fold in only the used kernel key bytes to the user key.
> > - Make the user key length 32 (Eric)
> > - Use CRYPTO_LIB_SHA256 for less boilerplate (Eric)
> >
> > include/uapi/linux/suspend_ioctls.h | 15 ++-
> > kernel/power/Kconfig | 1 +
> > kernel/power/power.h | 1 +
> > kernel/power/snapenc.c | 158 ++++++++++++++++++++++++++--
> > kernel/power/snapshot.c | 5 +
> > kernel/power/user.c | 4 +
> > kernel/power/user.h | 12 +++
> > 7 files changed, 185 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/uapi/linux/suspend_ioctls.h b/include/uapi/linux/suspend_ioctls.h
> > index b73026ef824bb9..f93a22eac52dc2 100644
> > --- a/include/uapi/linux/suspend_ioctls.h
> > +++ b/include/uapi/linux/suspend_ioctls.h
> > @@ -25,6 +25,18 @@ struct uswsusp_key_blob {
> > __u8 nonce[USWSUSP_KEY_NONCE_SIZE];
> > } __attribute__((packed));
> >
> > +/*
> > + * Allow user mode to fold in key material for the data portion of the hibernate
> > + * image.
> > + */
> > +struct uswsusp_user_key {
> > + /* Kernel returns the metadata size. */
> > + __kernel_loff_t meta_size;
> > + __u32 key_len;
> > + __u8 key[32];
>
> Why is this 32? (Is there a non-literal we can put here?)
Sure, I can make a new define for this: USWSUSP_USER_KEY_SIZE. Really
it just needs to be enough key material that usermode feels like
they've swizzled things up enough. I wanted to avoid using a
particular implementation constant like AES_KEYSIZE_256 because I
wanted that to be a kernel implementation detail, and also wanted to
avoid adding additional header dependencies to suspend_ioctls.h.
>
> > + __u32 pad;
>
> And why the pad?
I added the padding because I was finding myself struggling with what
I think are compiler differences when the structure size isn't a
multiple of its required alignment (which is 8 due to the
__kernel_loff_t). My usermode bindings in Rust were generating the
wrong ioctl numbers because it computed a different structure size.
Adding the padding removes the opportunity for misinterpretation.
>
> > +};
> > +
> > #define SNAPSHOT_IOC_MAGIC '3'
> > #define SNAPSHOT_FREEZE _IO(SNAPSHOT_IOC_MAGIC, 1)
> > #define SNAPSHOT_UNFREEZE _IO(SNAPSHOT_IOC_MAGIC, 2)
> > @@ -42,6 +54,7 @@ struct uswsusp_key_blob {
> > #define SNAPSHOT_AVAIL_SWAP_SIZE _IOR(SNAPSHOT_IOC_MAGIC, 19, __kernel_loff_t)
> > #define SNAPSHOT_ALLOC_SWAP_PAGE _IOR(SNAPSHOT_IOC_MAGIC, 20, __kernel_loff_t)
> > #define SNAPSHOT_ENABLE_ENCRYPTION _IOWR(SNAPSHOT_IOC_MAGIC, 21, struct uswsusp_key_blob)
> > -#define SNAPSHOT_IOC_MAXNR 21
> > +#define SNAPSHOT_SET_USER_KEY _IOWR(SNAPSHOT_IOC_MAGIC, 22, struct uswsusp_user_key)
> > +#define SNAPSHOT_IOC_MAXNR 22
> >
> > #endif /* _LINUX_SUSPEND_IOCTLS_H */
> > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > index 2f8acbd87b34dc..35bf48b925ebf6 100644
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -97,6 +97,7 @@ config ENCRYPTED_HIBERNATION
> > depends on HIBERNATION_SNAPSHOT_DEV
> > depends on CRYPTO_AEAD2=y
> > depends on TRUSTED_KEYS=y
> > + select CRYPTO_LIB_SHA256
> > default n
> > help
> > Enable support for kernel-based encryption of hibernation snapshots
> > diff --git a/kernel/power/power.h b/kernel/power/power.h
> > index b4f43394320961..5955e5cf692302 100644
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -151,6 +151,7 @@ struct snapshot_handle {
> >
> > extern unsigned int snapshot_additional_pages(struct zone *zone);
> > extern unsigned long snapshot_get_image_size(void);
> > +extern unsigned long snapshot_get_meta_page_count(void);
> > extern int snapshot_read_next(struct snapshot_handle *handle);
> > extern int snapshot_write_next(struct snapshot_handle *handle);
> > extern void snapshot_write_finalize(struct snapshot_handle *handle);
> > diff --git a/kernel/power/snapenc.c b/kernel/power/snapenc.c
> > index 7ff4fc66f7500c..50167a37c5bf23 100644
> > --- a/kernel/power/snapenc.c
> > +++ b/kernel/power/snapenc.c
> > @@ -6,6 +6,7 @@
> > #include <crypto/gcm.h>
> > #include <keys/trusted-type.h>
> > #include <linux/key-type.h>
> > +#include <crypto/sha.h>
> > #include <linux/random.h>
> > #include <linux/mm.h>
> > #include <linux/tpm.h>
> > @@ -21,6 +22,38 @@ static struct tpm_digest known_digest = { .alg_id = TPM_ALG_SHA256,
> > 0xf1, 0x22, 0x38, 0x6c, 0x33, 0xb1, 0x14, 0xb7, 0xec, 0x05,
> > 0x5f, 0x49}};
> >
> > +/* Derive a key from the kernel and user keys for data encryption. */
> > +static int snapshot_use_user_key(struct snapshot_data *data)
> > +{
> > + u8 digest[SHA256_DIGEST_SIZE];
> > + struct trusted_key_payload *payload = data->key->payload.data[0];
> > + struct sha256_state sha256_state;
> > +
> > + /*
> > + * Hash the kernel key and the user key together. This folds in the user
> > + * key, but not in a way that gives the user mode predictable control
> > + * over the key bits.
> > + */
> > + sha256_init(&sha256_state);
> > + sha256_update(&sha256_state, payload->key, SNAPSHOT_ENCRYPTION_KEY_SIZE);
> > + sha256_update(&sha256_state, data->user_key, sizeof(data->user_key));
> > + sha256_final(&sha256_state, digest);
> > + return crypto_aead_setkey(data->aead_tfm,
> > + digest,
> > + SNAPSHOT_ENCRYPTION_KEY_SIZE);
> > +}
> > +
> > +/* Check to see if it's time to switch to the user key, and do it if so. */
> > +static int snapshot_check_user_key_switch(struct snapshot_data *data)
> > +{
> > + if (data->user_key_valid && data->meta_size &&
> > + data->crypt_total == data->meta_size) {
> > + return snapshot_use_user_key(data);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /* Encrypt more data from the snapshot into the staging area. */
> > static int snapshot_encrypt_refill(struct snapshot_data *data)
> > {
> > @@ -32,6 +65,15 @@ static int snapshot_encrypt_refill(struct snapshot_data *data)
> > int pg_idx;
> > int res;
> >
> > + if (data->crypt_total == 0) {
> > + data->meta_size = snapshot_get_meta_page_count() << PAGE_SHIFT;
> > +
> > + } else {
> > + res = snapshot_check_user_key_switch(data);
> > + if (res)
> > + return res;
> > + }
> > +
> > /*
> > * The first buffer is the associated data, set to the offset to prevent
> > * attacks that rearrange chunks.
> > @@ -42,6 +84,11 @@ static int snapshot_encrypt_refill(struct snapshot_data *data)
> > for (pg_idx = 0; pg_idx < CHUNK_SIZE; pg_idx++) {
> > void *buf = data->crypt_pages[pg_idx];
> >
> > + /* Stop at the meta page boundary to potentially switch keys. */
> > + if (total &&
> > + ((data->crypt_total + total) == data->meta_size))
> > + break;
> > +
> > res = snapshot_read_next(&data->handle);
> > if (res < 0)
> > return res;
> > @@ -114,10 +161,10 @@ static int snapshot_decrypt_drain(struct snapshot_data *data)
> > sg_set_buf(&data->sg[1 + pg_idx], data->crypt_pages[pg_idx], PAGE_SIZE);
> >
> > /*
> > - * It's possible this is the final decrypt, and there are fewer than
> > - * CHUNK_SIZE pages. If this is the case we would have just written the
> > - * auth tag into the first few bytes of a new page. Copy to the tag if
> > - * so.
> > + * It's possible this is the final decrypt, or the final decrypt of the
> > + * meta region, and there are fewer than CHUNK_SIZE pages. If this is
> > + * the case we would have just written the auth tag into the first few
> > + * bytes of a new page. Copy to the tag if so.
> > */
> > if ((page_count < CHUNK_SIZE) &&
> > (data->crypt_offset - total) == sizeof(data->auth_tag)) {
> > @@ -172,7 +219,14 @@ static int snapshot_decrypt_drain(struct snapshot_data *data)
> > total += PAGE_SIZE;
> > }
> >
> > + if (data->crypt_total == 0)
> > + data->meta_size = snapshot_get_meta_page_count() << PAGE_SHIFT;
> > +
> > data->crypt_total += total;
> > + res = snapshot_check_user_key_switch(data);
> > + if (res)
> > + return res;
> > +
> > return 0;
> > }
> >
> > @@ -221,8 +275,26 @@ static ssize_t snapshot_write_next_encrypted(struct snapshot_data *data,
> > if (data->crypt_offset < (PAGE_SIZE * CHUNK_SIZE)) {
> > size_t pg_idx = data->crypt_offset >> PAGE_SHIFT;
> > size_t pg_off = data->crypt_offset & (PAGE_SIZE - 1);
> > + size_t size_avail = PAGE_SIZE;
> > *buf = data->crypt_pages[pg_idx] + pg_off;
> > - return PAGE_SIZE - pg_off;
> > +
> > + /*
> > + * If this is the boundary where the meta pages end, then just
> > + * return enough for the auth tag.
> > + */
> > + if (data->meta_size && (data->crypt_total < data->meta_size)) {
> > + uint64_t total_done =
> > + data->crypt_total + data->crypt_offset;
> > +
> > + if ((total_done >= data->meta_size) &&
> > + (total_done <
> > + (data->meta_size + SNAPSHOT_AUTH_TAG_SIZE))) {
> > +
> > + size_avail = SNAPSHOT_AUTH_TAG_SIZE;
> > + }
> > + }
> > +
> > + return size_avail - pg_off;
> > }
> >
> > /* Use offsets just beyond the size to return the tag. */
> > @@ -304,9 +376,15 @@ ssize_t snapshot_write_encrypted(struct snapshot_data *data,
> > break;
> > }
> >
> > - /* Drain the encrypted buffer if it's full. */
> > + /*
> > + * Drain the encrypted buffer if it's full, or if we hit the end
> > + * of the meta pages and need a key change.
> > + */
> > if ((data->crypt_offset >=
> > - ((PAGE_SIZE * CHUNK_SIZE) + SNAPSHOT_AUTH_TAG_SIZE))) {
> > + ((PAGE_SIZE * CHUNK_SIZE) + SNAPSHOT_AUTH_TAG_SIZE)) ||
> > + (data->meta_size && (data->crypt_total < data->meta_size) &&
> > + ((data->crypt_total + data->crypt_offset) ==
> > + (data->meta_size + SNAPSHOT_AUTH_TAG_SIZE)))) {
> >
> > int rc;
> >
> > @@ -350,6 +428,8 @@ void snapshot_teardown_encryption(struct snapshot_data *data)
> > data->crypt_pages[i] = NULL;
> > }
> > }
> > +
> > + memset(data->user_key, 0, sizeof(data->user_key));
> > }
> >
> > static int snapshot_setup_encryption_common(struct snapshot_data *data)
> > @@ -359,6 +439,7 @@ static int snapshot_setup_encryption_common(struct snapshot_data *data)
> > data->crypt_total = 0;
> > data->crypt_offset = 0;
> > data->crypt_size = 0;
> > + data->user_key_valid = false;
> > memset(data->crypt_pages, 0, sizeof(data->crypt_pages));
> > /* This only works once per hibernate. */
> > if (data->aead_tfm)
> > @@ -661,15 +742,72 @@ int snapshot_set_encryption_key(struct snapshot_data *data,
> > return rc;
> > }
> >
> > -loff_t snapshot_get_encrypted_image_size(loff_t raw_size)
> > +static loff_t snapshot_encrypted_byte_count(loff_t plain_size)
> > {
> > - loff_t pages = raw_size >> PAGE_SHIFT;
> > + loff_t pages = plain_size >> PAGE_SHIFT;
> > loff_t chunks = (pages + (CHUNK_SIZE - 1)) / CHUNK_SIZE;
> > /*
> > * The encrypted size is the normal size, plus a stitched in
> > * authentication tag for every chunk of pages.
> > */
> > - return raw_size + (chunks * SNAPSHOT_AUTH_TAG_SIZE);
> > + return plain_size + (chunks * SNAPSHOT_AUTH_TAG_SIZE);
> > +}
> > +
> > +static loff_t snapshot_get_meta_data_size(void)
> > +{
> > + loff_t pages = snapshot_get_meta_page_count();
> > +
> > + return snapshot_encrypted_byte_count(pages << PAGE_SHIFT);
> > +}
> > +
> > +int snapshot_set_user_key(struct snapshot_data *data,
> > + struct uswsusp_user_key __user *key)
> > +{
> > + struct uswsusp_user_key user_key;
> > + unsigned int key_len;
> > + int rc;
> > + loff_t size;
> > +
> > + /*
> > + * Return the metadata size, the number of bytes that can be fed in before
> > + * the user data key is needed at resume time.
> > + */
> > + size = snapshot_get_meta_data_size();
> > + rc = put_user(size, &key->meta_size);
> > + if (rc)
> > + return rc;
> > +
> > + rc = copy_from_user(&user_key, key, sizeof(struct uswsusp_user_key));
> > + if (rc)
> > + return rc;
> > +
> > + key_len = min_t(__u32, user_key.key_len, sizeof(data->user_key));
> > + if (key_len < 8)
> > + return -EINVAL;
> > +
> > + /* Don't allow it if it's too late. */
> > + if (data->crypt_total > data->meta_size)
> > + return -EBUSY;
> > +
> > + memset(data->user_key, 0, sizeof(data->user_key));
> > + memcpy(data->user_key, user_key.key, key_len);
>
> Is struct snapshot_data::user_key is supposed to be %NUL terminated? Or
> is it just 0-padded up to 32 bytes? If the latter, it might be worth
> marking struct snapshot_data::user_data with the __non_string attribute.
It's just zero padded up to 32 bytes, and is stored here until it's
ready to be folded in by snapshot_use_user_key(). I'll add the
attribute as well.
>
> I don't like the dissociation of struct uswsusp_user_key::user_key and
> struct snapshot_data::user_key, since a mistake here can lead to copying
> kernel memory into struct snapshot_data::user_key. It would be nice to
> see something like:
>
> BUILD_BUG_ON(sizeof(data->user_key) < sizeof(user_key.key));
Ok, now that I've got a define for the size in suspend_ioctls.h, I'll
use that in snapshot_data.user_key as well. I'll also add the
BUILD_BUG_ON here, and for a couple of other compile-time size
requirements in snapshot_use_user_key().
>
> --
> Kees Cook
Powered by blists - more mailing lists