[PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[David Gstir]

The DCP trusted key type uses the wrong helper function to store
the blob's payload length which can lead to the wrong byte order
being used in case this would ever run on big endian architectures.

Fix by using correct helper function.

Signed-off-by: David Gstir <david@sigma-star.at>
Suggested-by: Richard Weinberger <richard@nod.at>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202405240610.fj53EK0q-lkp@intel.com/
Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")
---
 security/keys/trusted-keys/trusted_dcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/trusted-keys/trusted_dcp.c b/security/keys/trusted-keys/trusted_dcp.c
index b5f81a05be36..b0947f072a98 100644
--- a/security/keys/trusted-keys/trusted_dcp.c
+++ b/security/keys/trusted-keys/trusted_dcp.c
@@ -222,7 +222,7 @@ static int trusted_dcp_seal(struct trusted_key_payload *p, char *datablob)
 		return ret;
 	}
 
-	b->payload_len = get_unaligned_le32(&p->key_len);
+	put_unaligned_le32(p->key_len, &b->payload_len);
 	p->blob_len = blen;
 	return 0;
 }
-- 
2.35.3

[PATCH 2/2] KEYS: trusted: dcp: fix leak of blob encryption key

[David Gstir]

Trusted keys unseal the key blob on load, but keep the sealed payload in
the blob field so that every subsequent read (export) will simply
convert this field to hex and send it to userspace.

With DCP-based trusted keys, we decrypt the blob encryption key (BEK)
in the Kernel due hardware limitations and then decrypt the blob payload.
BEK decryption is done in-place which means that the trusted key blob
field is modified and it consequently holds the BEK in plain text.
Every subsequent read of that key thus send the plain text BEK instead
of the encrypted BEK to userspace.

This issue only occurs when importing a trusted DCP-based key and
then exporting it again. This should rarely happen as the common use cases
are to either create a new trusted key and export it, or import a key
blob and then just use it without exporting it again.

Fix this by performing BEK decryption and encryption in a dedicated
buffer. Further always wipe the plain text BEK buffer to prevent leaking
the key via uninitialized memory.

Signed-off-by: David Gstir <david@sigma-star.at>
Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")
---
 security/keys/trusted-keys/trusted_dcp.c | 33 +++++++++++++++---------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_dcp.c b/security/keys/trusted-keys/trusted_dcp.c
index b0947f072a98..4edc5bbbcda3 100644
--- a/security/keys/trusted-keys/trusted_dcp.c
+++ b/security/keys/trusted-keys/trusted_dcp.c
@@ -186,20 +186,21 @@ static int do_aead_crypto(u8 *in, u8 *out, size_t len, u8 *key, u8 *nonce,
 	return ret;
 }
 
-static int decrypt_blob_key(u8 *key)
+static int decrypt_blob_key(u8 *encrypted_key, u8 *plain_key)
 {
-	return do_dcp_crypto(key, key, false);
+	return do_dcp_crypto(encrypted_key, plain_key, false);
 }
 
-static int encrypt_blob_key(u8 *key)
+static int encrypt_blob_key(u8 *plain_key, u8 *encrypted_key)
 {
-	return do_dcp_crypto(key, key, true);
+	return do_dcp_crypto(plain_key, encrypted_key, true);
 }
 
 static int trusted_dcp_seal(struct trusted_key_payload *p, char *datablob)
 {
 	struct dcp_blob_fmt *b = (struct dcp_blob_fmt *)p->blob;
 	int blen, ret;
+	u8 plain_blob_key[AES_KEYSIZE_128];
 
 	blen = calc_blob_len(p->key_len);
 	if (blen > MAX_BLOB_SIZE)
@@ -207,30 +208,36 @@ static int trusted_dcp_seal(struct trusted_key_payload *p, char *datablob)
 
 	b->fmt_version = DCP_BLOB_VERSION;
 	get_random_bytes(b->nonce, AES_KEYSIZE_128);
-	get_random_bytes(b->blob_key, AES_KEYSIZE_128);
+	get_random_bytes(plain_blob_key, AES_KEYSIZE_128);
 
-	ret = do_aead_crypto(p->key, b->payload, p->key_len, b->blob_key,
+	ret = do_aead_crypto(p->key, b->payload, p->key_len, plain_blob_key,
 			     b->nonce, true);
 	if (ret) {
 		pr_err("Unable to encrypt blob payload: %i\n", ret);
-		return ret;
+		goto out;
 	}
 
-	ret = encrypt_blob_key(b->blob_key);
+	ret = encrypt_blob_key(plain_blob_key, b->blob_key);
 	if (ret) {
 		pr_err("Unable to encrypt blob key: %i\n", ret);
-		return ret;
+		goto out;
 	}
 
 	put_unaligned_le32(p->key_len, &b->payload_len);
 	p->blob_len = blen;
-	return 0;
+	ret = 0;
+
+out:
+	memzero_explicit(plain_blob_key, sizeof(plain_blob_key));
+
+	return ret;
 }
 
 static int trusted_dcp_unseal(struct trusted_key_payload *p, char *datablob)
 {
 	struct dcp_blob_fmt *b = (struct dcp_blob_fmt *)p->blob;
 	int blen, ret;
+	u8 plain_blob_key[AES_KEYSIZE_128];
 
 	if (b->fmt_version != DCP_BLOB_VERSION) {
 		pr_err("DCP blob has bad version: %i, expected %i\n",
@@ -248,14 +255,14 @@ static int trusted_dcp_unseal(struct trusted_key_payload *p, char *datablob)
 		goto out;
 	}
 
-	ret = decrypt_blob_key(b->blob_key);
+	ret = decrypt_blob_key(b->blob_key, plain_blob_key);
 	if (ret) {
 		pr_err("Unable to decrypt blob key: %i\n", ret);
 		goto out;
 	}
 
 	ret = do_aead_crypto(b->payload, p->key, p->key_len + DCP_BLOB_AUTHLEN,
-			     b->blob_key, b->nonce, false);
+			     plain_blob_key, b->nonce, false);
 	if (ret) {
 		pr_err("Unwrap of DCP payload failed: %i\n", ret);
 		goto out;
@@ -263,6 +270,8 @@ static int trusted_dcp_unseal(struct trusted_key_payload *p, char *datablob)
 
 	ret = 0;
 out:
+	memzero_explicit(plain_blob_key, sizeof(plain_blob_key));
+
 	return ret;
 }
 
-- 
2.35.3

Re: [PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[Jarkko Sakkinen]

On Wed Jul 3, 2024 at 3:53 PM EEST, David Gstir wrote:
> The DCP trusted key type uses the wrong helper function to store
> the blob's payload length which can lead to the wrong byte order
> being used in case this would ever run on big endian architectures.
>
> Fix by using correct helper function.
>
> Signed-off-by: David Gstir <david@sigma-star.at>
> Suggested-by: Richard Weinberger <richard@nod.at>

You cannot suggest a change that you author yourself.

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202405240610.fj53EK0q-lkp@intel.com/
> Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")

Tags are in wrong order. For next round:

Cc: stable@vger.kernel.org # v6.10+
Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202405240610.fj53EK0q-lkp@intel.com/
Signed-off-by: David Gstir <david@sigma-star.at>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>


BR, Jarkko

Re: [PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[Richard Weinberger]

Am Mittwoch, 17. Juli 2024, 12:07:58 CEST schrieb Jarkko Sakkinen:
> On Wed Jul 3, 2024 at 3:53 PM EEST, David Gstir wrote:
> > The DCP trusted key type uses the wrong helper function to store
> > the blob's payload length which can lead to the wrong byte order
> > being used in case this would ever run on big endian architectures.
> >
> > Fix by using correct helper function.
> >
> > Signed-off-by: David Gstir <david@sigma-star.at>
> > Suggested-by: Richard Weinberger <richard@nod.at>
> 
> You cannot suggest a change that you author yourself.

Well, I suggested the change, not David.
So, I think you're implying that David's s-o-b cannot be before
the Suggested-by tag but after?
 
Thanks,
//richard

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: [PATCH 2/2] KEYS: trusted: dcp: fix leak of blob encryption key

[Jarkko Sakkinen]

On Wed Jul 3, 2024 at 3:53 PM EEST, David Gstir wrote:
> Trusted keys unseal the key blob on load, but keep the sealed payload in
> the blob field so that every subsequent read (export) will simply
> convert this field to hex and send it to userspace.
>
> With DCP-based trusted keys, we decrypt the blob encryption key (BEK)
> in the Kernel due hardware limitations and then decrypt the blob payload.
> BEK decryption is done in-place which means that the trusted key blob
> field is modified and it consequently holds the BEK in plain text.
> Every subsequent read of that key thus send the plain text BEK instead
> of the encrypted BEK to userspace.
>
> This issue only occurs when importing a trusted DCP-based key and
> then exporting it again. This should rarely happen as the common use cases
> are to either create a new trusted key and export it, or import a key
> blob and then just use it without exporting it again.
>
> Fix this by performing BEK decryption and encryption in a dedicated
> buffer. Further always wipe the plain text BEK buffer to prevent leaking
> the key via uninitialized memory.
>
> Signed-off-by: David Gstir <david@sigma-star.at>
> Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")

Similar comments, fixes before sob etc and CC to stable with "# v6.10+"

BR, Jarkko

Re: [PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[David Gstir]

Jarkko,

> On 17.07.2024, at 12:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Wed Jul 3, 2024 at 3:53 PM EEST, David Gstir wrote:
>> The DCP trusted key type uses the wrong helper function to store
>> the blob's payload length which can lead to the wrong byte order
>> being used in case this would ever run on big endian architectures.
>> 
>> Fix by using correct helper function.
>> 
>> Signed-off-by: David Gstir <david@sigma-star.at>
>> Suggested-by: Richard Weinberger <richard@nod.at>
> 
> You cannot suggest a change that you author yourself.
> 
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202405240610.fj53EK0q-lkp@intel.com/
>> Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")
> 
> Tags are in wrong order. For next round:

here’s me relying on checkpatch.pl to tell me this, but it did not. :-/
Anyways, thanks for reviewing! I’ll fix the tags and send v2.

BR, David

Re: [PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[Jarkko Sakkinen]

On Wed Jul 17, 2024 at 1:19 PM EEST, Richard Weinberger wrote:
> Am Mittwoch, 17. Juli 2024, 12:07:58 CEST schrieb Jarkko Sakkinen:
> > On Wed Jul 3, 2024 at 3:53 PM EEST, David Gstir wrote:
> > > The DCP trusted key type uses the wrong helper function to store
> > > the blob's payload length which can lead to the wrong byte order
> > > being used in case this would ever run on big endian architectures.
> > >
> > > Fix by using correct helper function.
> > >
> > > Signed-off-by: David Gstir <david@sigma-star.at>
> > > Suggested-by: Richard Weinberger <richard@nod.at>
> > 
> > You cannot suggest a change that you author yourself.
>
> Well, I suggested the change, not David.
> So, I think you're implying that David's s-o-b cannot be before
> the Suggested-by tag but after?

I have dyslexia, I actually read it incorrectly so thanks
for pointing this ;-)

So keeping the tag is fine just reorder the tags, and the fix
will be fine.

Don't expect fast response, I'm on holiday still this and next
week. Just shuffling through inbox weekly basis.

BR, Jarkko

Re: [PATCH 1/2] KEYS: trusted: fix DCP blob payload length assignment

[Jarkko Sakkinen]

On Wed Jul 17, 2024 at 2:03 PM EEST, David Gstir wrote:
> Jarkko,
>
> > On 17.07.2024, at 12:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Wed Jul 3, 2024 at 3:53 PM EEST, David Gstir wrote:
> >> The DCP trusted key type uses the wrong helper function to store
> >> the blob's payload length which can lead to the wrong byte order
> >> being used in case this would ever run on big endian architectures.
> >> 
> >> Fix by using correct helper function.
> >> 
> >> Signed-off-by: David Gstir <david@sigma-star.at>
> >> Suggested-by: Richard Weinberger <richard@nod.at>
> > 
> > You cannot suggest a change that you author yourself.
> > 
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Closes: https://lore.kernel.org/oe-kbuild-all/202405240610.fj53EK0q-lkp@intel.com/
> >> Fixes: 2e8a0f40a39c ("KEYS: trusted: Introduce NXP DCP-backed trusted keys")
> > 
> > Tags are in wrong order. For next round:
>
> here’s me relying on checkpatch.pl to tell me this, but it did not. :-/
> Anyways, thanks for reviewing! I’ll fix the tags and send v2.

Cool, might take over a week before response from my side but I'm sure
we get this to some rc of 6.11. I've purposely kept my 6.11 PR feature
free because the merge window was right in the middle of my holiday
:-)

BR, Jarkko