| 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
| ||
|
Date: Fri, 4 Nov 2022 12:00:52 -0700
From: Kees Cook <keescook@...omium.org>
To: Evan Green <evgreen@...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,
Matthew Garrett <mjg59@...gle.com>,
Len Brown <len.brown@...el.com>,
"Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v4 10/11] PM: hibernate: Verify the digest encryption key
On Thu, Nov 03, 2022 at 11:01:18AM -0700, Evan Green wrote:
> We want to ensure that the key used to encrypt the digest was created by
> the kernel during hibernation. To do this we request that the TPM
> include information about the value of PCR 23 at the time of key
> creation in the sealed blob. On resume, we can make sure that the PCR
> information in the creation data blob (already certified by the TPM to
> be accurate) corresponds to the expected value. Since only
> the kernel can touch PCR 23, if an attacker generates a key themselves
> the value of PCR 23 will have been different, allowing us to reject the
> key and boot normally instead of resuming.
>
> Co-developed-by: Matthew Garrett <mjg59@...gle.com>
> Signed-off-by: Matthew Garrett <mjg59@...gle.com>
> Signed-off-by: Evan Green <evgreen@...omium.org>
>
> ---
> Matthew's original version of this patch is here:
> https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@google.com/
>
> I moved the TPM2_CC_CERTIFYCREATION code into a separate change in the
> trusted key code because the blob_handle was being flushed and was no
> longer valid for use in CC_CERTIFYCREATION after the key was loaded. As
> an added benefit of moving the certification into the trusted keys code,
> we can drop the other patch from the original series that squirrelled
> the blob_handle away.
>
> Changes in v4:
> - Local variable reordering (Jarkko)
>
> Changes in v3:
> - Changed funky tag to Co-developed-by (Kees). Matthew, holler if you
> want something different.
>
> Changes in v2:
> - Fixed some sparse warnings
> - Use CRYPTO_LIB_SHA256 to get rid of sha256_data() (Eric)
> - Adjusted offsets due to new ASN.1 format, and added a creation data
> length check.
>
> kernel/power/snapenc.c | 67 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/power/snapenc.c b/kernel/power/snapenc.c
> index 50167a37c5bf23..2f421061498246 100644
> --- a/kernel/power/snapenc.c
> +++ b/kernel/power/snapenc.c
> @@ -22,6 +22,12 @@ static struct tpm_digest known_digest = { .alg_id = TPM_ALG_SHA256,
> 0xf1, 0x22, 0x38, 0x6c, 0x33, 0xb1, 0x14, 0xb7, 0xec, 0x05,
> 0x5f, 0x49}};
>
> +/* sha256(sha256(empty_pcr | known_digest)) */
> +static const char expected_digest[] = {0x2f, 0x96, 0xf2, 0x1b, 0x70, 0xa9, 0xe8,
> + 0x42, 0x25, 0x8e, 0x66, 0x07, 0xbe, 0xbc, 0xe3, 0x1f, 0x2c, 0x84, 0x4a,
> + 0x3f, 0x85, 0x17, 0x31, 0x47, 0x9a, 0xa5, 0x53, 0xbb, 0x23, 0x0c, 0x32,
> + 0xf3};
> +
> /* Derive a key from the kernel and user keys for data encryption. */
> static int snapshot_use_user_key(struct snapshot_data *data)
> {
> @@ -486,7 +492,7 @@ static int snapshot_setup_encryption_common(struct snapshot_data *data)
> static int snapshot_create_kernel_key(struct snapshot_data *data)
> {
> /* Create a key sealed by the SRK. */
> - char *keyinfo = "new\t32\tkeyhandle=0x81000000";
> + char *keyinfo = "new\t32\tkeyhandle=0x81000000\tcreationpcrs=0x00800000";
> const struct cred *cred = current_cred();
> struct tpm_digest *digests = NULL;
> struct key *key = NULL;
> @@ -613,6 +619,8 @@ static int snapshot_load_kernel_key(struct snapshot_data *data,
>
> char *keytemplate = "load\t%s\tkeyhandle=0x81000000";
> const struct cred *cred = current_cred();
> + struct trusted_key_payload *payload;
> + char certhash[SHA256_DIGEST_SIZE];
> struct tpm_digest *digests = NULL;
> char *blobstring = NULL;
> struct key *key = NULL;
> @@ -635,8 +643,10 @@ static int snapshot_load_kernel_key(struct snapshot_data *data,
>
> digests = kcalloc(chip->nr_allocated_banks, sizeof(struct tpm_digest),
> GFP_KERNEL);
> - if (!digests)
> + if (!digests) {
> + ret = -ENOMEM;
> goto out;
> + }
>
> for (i = 0; i < chip->nr_allocated_banks; i++) {
> digests[i].alg_id = chip->allocated_banks[i].alg_id;
> @@ -676,6 +686,59 @@ static int snapshot_load_kernel_key(struct snapshot_data *data,
> if (ret != 0)
> goto out;
>
> + /* Verify the creation hash matches the creation data. */
> + payload = key->payload.data[0];
> + if (!payload->creation || !payload->creation_hash ||
> + (payload->creation_len < 3) ||
Later accesses are reaching into indexes, 6, 8, 12, 14, etc. Shouldn't
this test be:
(payload->creation_len < 14 + SHA256_DIGEST_SIZE) ||
> + (payload->creation_hash_len < SHA256_DIGEST_SIZE)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + sha256(payload->creation + 2, payload->creation_len - 2, certhash);
Why +2 offset?
> + if (memcmp(payload->creation_hash + 2, certhash, SHA256_DIGEST_SIZE) != 0) {
And if this is +2 also, shouldn't the earlier test be:
(payload->creation_hash_len - 2 != SHA256_DIGEST_SIZE)) {
?
> + if (be32_to_cpu(*(__be32 *)&payload->creation[2]) != 1) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (be16_to_cpu(*(__be16 *)&payload->creation[6]) != TPM_ALG_SHA256) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (*(char *)&payload->creation[8] != 3) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* PCR 23 selected */
> + if (be32_to_cpu(*(__be32 *)&payload->creation[8]) != 0x03000080) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (be16_to_cpu(*(__be16 *)&payload->creation[12]) !=
> + SHA256_DIGEST_SIZE) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* Verify PCR 23 contained the expected value when the key was created. */
> + if (memcmp(&payload->creation[14], expected_digest,
> + SHA256_DIGEST_SIZE) != 0) {
These various literals (2, 6, 8, 3, 8, 0x03000080, 12, 14) should be
explicit #defines so their purpose/meaning is more clear.
I can guess at it, but better to avoid the guessing. :)
> +
> + ret = -EINVAL;
> + goto out;
> + }
> +
> data->key = key;
> key = NULL;
>
> --
> 2.38.1.431.g37b22c650d-goog
>
--
Kees Cook
Powered by blists - more mailing lists