[PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

Only the kernel random pool should be used for generating random numbers.
TPM contributes to that pool among the other sources of entropy. In here it
is not, agreed, absolutely critical because TPM is what is trusted anyway
but in order to remove tpm_get_random() we need to first remove all the
call sites.

Cc: stable@vger.kernel.org
Fixes: 0c36264aa1d5 ("KEYS: asym_tpm: Add loadkey2 and flushspecific [ver #2]")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 crypto/asymmetric_keys/asym_tpm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index 76d2ce3a1b5b..c14b8d186e93 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -6,6 +6,7 @@
 #include <linux/kernel.h>
 #include <linux/seq_file.h>
 #include <linux/scatterlist.h>
+#include <linux/random.h>
 #include <linux/tpm.h>
 #include <linux/tpm_command.h>
 #include <crypto/akcipher.h>
@@ -54,11 +55,7 @@ static int tpm_loadkey2(struct tpm_buf *tb,
 	}
 
 	/* generate odd nonce */
-	ret = tpm_get_random(NULL, nonceodd, TPM_NONCE_SIZE);
-	if (ret < 0) {
-		pr_info("tpm_get_random failed (%d)\n", ret);
-		return ret;
-	}
+	get_random_bytes(nonceodd, TPM_NONCE_SIZE);
 
 	/* calculate authorization HMAC value */
 	ret = TSS_authhmac(authdata, keyauth, SHA1_DIGEST_SIZE, enonce,
-- 
2.20.1

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jerry Snitselaar]

On Thu Sep 26 19, Jarkko Sakkinen wrote:
>Only the kernel random pool should be used for generating random numbers.
>TPM contributes to that pool among the other sources of entropy. In here it
>is not, agreed, absolutely critical because TPM is what is trusted anyway
>but in order to remove tpm_get_random() we need to first remove all the
>call sites.
>
>Cc: stable@vger.kernel.org
>Fixes: 0c36264aa1d5 ("KEYS: asym_tpm: Add loadkey2 and flushspecific [ver #2]")
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>---
> crypto/asymmetric_keys/asym_tpm.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
>diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
>index 76d2ce3a1b5b..c14b8d186e93 100644
>--- a/crypto/asymmetric_keys/asym_tpm.c
>+++ b/crypto/asymmetric_keys/asym_tpm.c
>@@ -6,6 +6,7 @@
> #include <linux/kernel.h>
> #include <linux/seq_file.h>
> #include <linux/scatterlist.h>
>+#include <linux/random.h>
> #include <linux/tpm.h>
> #include <linux/tpm_command.h>
> #include <crypto/akcipher.h>
>@@ -54,11 +55,7 @@ static int tpm_loadkey2(struct tpm_buf *tb,
> 	}
>
> 	/* generate odd nonce */
>-	ret = tpm_get_random(NULL, nonceodd, TPM_NONCE_SIZE);
>-	if (ret < 0) {
>-		pr_info("tpm_get_random failed (%d)\n", ret);
>-		return ret;
>-	}
>+	get_random_bytes(nonceodd, TPM_NONCE_SIZE);
>
> 	/* calculate authorization HMAC value */
> 	ret = TSS_authhmac(authdata, keyauth, SHA1_DIGEST_SIZE, enonce,
>-- 
>2.20.1
>

Should tpm_unbind and tpm_sign in asym_tpm.c be switched as well then?

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Sat, Sep 28, 2019 at 11:05:59AM -0700, Jerry Snitselaar wrote:
> On Thu Sep 26 19, Jarkko Sakkinen wrote:
> > Only the kernel random pool should be used for generating random numbers.
> > TPM contributes to that pool among the other sources of entropy. In here it
> > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > but in order to remove tpm_get_random() we need to first remove all the > > call sites.  > > 
> > Cc: stable@vger.kernel.org
> > Fixes: 0c36264aa1d5 ("KEYS: asym_tpm: Add loadkey2 and flushspecific [ver #2]")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > crypto/asymmetric_keys/asym_tpm.c | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
> > index 76d2ce3a1b5b..c14b8d186e93 100644
> > --- a/crypto/asymmetric_keys/asym_tpm.c
> > +++ b/crypto/asymmetric_keys/asym_tpm.c
> > @@ -6,6 +6,7 @@
> > #include <linux/kernel.h>
> > #include <linux/seq_file.h>
> > #include <linux/scatterlist.h>
> > +#include <linux/random.h>
> > #include <linux/tpm.h>
> > #include <linux/tpm_command.h>
> > #include <crypto/akcipher.h>
> > @@ -54,11 +55,7 @@ static int tpm_loadkey2(struct tpm_buf *tb,
> > 	}
> > 
> > 	/* generate odd nonce */
> > -	ret = tpm_get_random(NULL, nonceodd, TPM_NONCE_SIZE);
> > -	if (ret < 0) {
> > -		pr_info("tpm_get_random failed (%d)\n", ret);
> > -		return ret;
> > -	}
> > +	get_random_bytes(nonceodd, TPM_NONCE_SIZE);
> > 
> > 	/* calculate authorization HMAC value */
> > 	ret = TSS_authhmac(authdata, keyauth, SHA1_DIGEST_SIZE, enonce,
> > -- 
> > 2.20.1
> > 
> 
> Should tpm_unbind and tpm_sign in asym_tpm.c be switched as well then?

Without doubt. Thanks. I'll send an update soon.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Mimi Zohar]

On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> Only the kernel random pool should be used for generating random numbers.
> TPM contributes to that pool among the other sources of entropy. In here it
> is not, agreed, absolutely critical because TPM is what is trusted anyway
> but in order to remove tpm_get_random() we need to first remove all the
> call sites.

At what point during boot is the kernel random pool available?  Does
this imply that you're planning on changing trusted keys as well?

Mimi

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > Only the kernel random pool should be used for generating random numbers.
> > TPM contributes to that pool among the other sources of entropy. In here it
> > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > but in order to remove tpm_get_random() we need to first remove all the
> > call sites.
> 
> At what point during boot is the kernel random pool available?  Does
> this imply that you're planning on changing trusted keys as well?

Well trusted keys *must* be changed to use it. It is not a choice
because using a proprietary random number generator instead of defacto
one in the kernel can be categorized as a *regression*.

Also, TEE trusted keys cannot use the TPM option.

If it was not initialized early enough we would need fix that too.

I don't think there should be a problem anyway since encrypted keys is
already using get_random_bytes().

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 02:41:19PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > Only the kernel random pool should be used for generating random numbers.
> > > TPM contributes to that pool among the other sources of entropy. In here it
> > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > but in order to remove tpm_get_random() we need to first remove all the
> > > call sites.
> > 
> > At what point during boot is the kernel random pool available?  Does
> > this imply that you're planning on changing trusted keys as well?
> 
> Well trusted keys *must* be changed to use it. It is not a choice
> because using a proprietary random number generator instead of defacto
> one in the kernel can be categorized as a *regression*.
> 
> Also, TEE trusted keys cannot use the TPM option.
> 
> If it was not initialized early enough we would need fix that too.
> 
> I don't think there should be a problem anyway since encrypted keys is
> already using get_random_bytes().

Looking at asym_tpm.c the implementation copies all the anti-patterns
from trusted keys, which is really unfortunate. I don't know how that
has passed through all the filters.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Mimi Zohar]

On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > Only the kernel random pool should be used for generating random numbers.
> > > TPM contributes to that pool among the other sources of entropy. In here it
> > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > but in order to remove tpm_get_random() we need to first remove all the
> > > call sites.
> > 
> > At what point during boot is the kernel random pool available?  Does
> > this imply that you're planning on changing trusted keys as well?
> 
> Well trusted keys *must* be changed to use it. It is not a choice
> because using a proprietary random number generator instead of defacto
> one in the kernel can be categorized as a *regression*.

I really don't see how using the TPM random number for TPM trusted
keys would be considered a regression.  That by definition is a
trusted key.  If anything, changing what is currently being done would
be the regression. 

> Also, TEE trusted keys cannot use the TPM option.

That isn't a valid justification for changing the original definition
of trusted keys.  Just as the kernel supports different methods of
implementing the same function on different architectures, trusted
keys will need to support different methods of generating a random
number.   

> 
> If it was not initialized early enough we would need fix that too.

Shouldn't this be determined and fixed, before making any changes?

> 
> I don't think there should be a problem anyway since encrypted keys is
> already using get_random_bytes().

Encrypted keys has no bearing on trusted keys.

Mimi

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > Only the kernel random pool should be used for generating random numbers.
> > > > TPM contributes to that pool among the other sources of entropy. In here it
> > > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > > but in order to remove tpm_get_random() we need to first remove all the
> > > > call sites.
> > > 
> > > At what point during boot is the kernel random pool available?  Does
> > > this imply that you're planning on changing trusted keys as well?
> > 
> > Well trusted keys *must* be changed to use it. It is not a choice
> > because using a proprietary random number generator instead of defacto
> > one in the kernel can be categorized as a *regression*.
> 
> I really don't see how using the TPM random number for TPM trusted
> keys would be considered a regression.  That by definition is a
> trusted key.  If anything, changing what is currently being done would
> be the regression. 

It is really not a TPM trusted key. It trusted key that gets sealed with
the TPM. The key itself is used in clear by kernel. The random number
generator exists in the kernel to for a reason.

It is without doubt a regression.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Mimi Zohar]

[Cc'ing David Safford]

On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > > Only the kernel random pool should be used for generating random numbers.
> > > > > TPM contributes to that pool among the other sources of entropy. In here it
> > > > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > > > but in order to remove tpm_get_random() we need to first remove all the
> > > > > call sites.
> > > > 
> > > > At what point during boot is the kernel random pool available?  Does
> > > > this imply that you're planning on changing trusted keys as well?
> > > 
> > > Well trusted keys *must* be changed to use it. It is not a choice
> > > because using a proprietary random number generator instead of defacto
> > > one in the kernel can be categorized as a *regression*.
> > 
> > I really don't see how using the TPM random number for TPM trusted
> > keys would be considered a regression.  That by definition is a
> > trusted key.  If anything, changing what is currently being done would
> > be the regression. 
> 
> It is really not a TPM trusted key. It trusted key that gets sealed with
> the TPM. The key itself is used in clear by kernel. The random number
> generator exists in the kernel to for a reason.
> 
> It is without doubt a regression.

You're misusing the term "regression" here.  A regression is something
that previously worked and has stopped working.  In this case, trusted
keys has always been based on the TPM random number generator.  Before
changing this, there needs to be some guarantees that the kernel
random number generator has a pool of random numbers early, on all
systems including embedded devices, not just servers.

Mimi

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 02:53:47PM -0400, Mimi Zohar wrote:
> [Cc'ing David Safford]
> 
> On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > > > Only the kernel random pool should be used for generating random numbers.
> > > > > > TPM contributes to that pool among the other sources of entropy. In here it
> > > > > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > > > > but in order to remove tpm_get_random() we need to first remove all the
> > > > > > call sites.
> > > > > 
> > > > > At what point during boot is the kernel random pool available?  Does
> > > > > this imply that you're planning on changing trusted keys as well?
> > > > 
> > > > Well trusted keys *must* be changed to use it. It is not a choice
> > > > because using a proprietary random number generator instead of defacto
> > > > one in the kernel can be categorized as a *regression*.
> > > 
> > > I really don't see how using the TPM random number for TPM trusted
> > > keys would be considered a regression.  That by definition is a
> > > trusted key.  If anything, changing what is currently being done would
> > > be the regression. 
> > 
> > It is really not a TPM trusted key. It trusted key that gets sealed with
> > the TPM. The key itself is used in clear by kernel. The random number
> > generator exists in the kernel to for a reason.
> > 
> > It is without doubt a regression.
> 
> You're misusing the term "regression" here.  A regression is something
> that previously worked and has stopped working.  In this case, trusted
> keys has always been based on the TPM random number generator.  Before
> changing this, there needs to be some guarantees that the kernel
> random number generator has a pool of random numbers early, on all
> systems including embedded devices, not just servers.

I'm not using the term regression incorrectly here. Wrong function
was used to generate random numbers for the payload here. It is an
obvious bug.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Fri, Oct 04, 2019 at 12:51:25AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 03, 2019 at 02:53:47PM -0400, Mimi Zohar wrote:
> > [Cc'ing David Safford]
> > 
> > On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> > > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > > > > Only the kernel random pool should be used for generating random numbers.
> > > > > > > TPM contributes to that pool among the other sources of entropy. In here it
> > > > > > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > > > > > but in order to remove tpm_get_random() we need to first remove all the
> > > > > > > call sites.
> > > > > > 
> > > > > > At what point during boot is the kernel random pool available?  Does
> > > > > > this imply that you're planning on changing trusted keys as well?
> > > > > 
> > > > > Well trusted keys *must* be changed to use it. It is not a choice
> > > > > because using a proprietary random number generator instead of defacto
> > > > > one in the kernel can be categorized as a *regression*.
> > > > 
> > > > I really don't see how using the TPM random number for TPM trusted
> > > > keys would be considered a regression.  That by definition is a
> > > > trusted key.  If anything, changing what is currently being done would
> > > > be the regression. 
> > > 
> > > It is really not a TPM trusted key. It trusted key that gets sealed with
> > > the TPM. The key itself is used in clear by kernel. The random number
> > > generator exists in the kernel to for a reason.
> > > 
> > > It is without doubt a regression.
> > 
> > You're misusing the term "regression" here.  A regression is something
> > that previously worked and has stopped working.  In this case, trusted
> > keys has always been based on the TPM random number generator.  Before
> > changing this, there needs to be some guarantees that the kernel
> > random number generator has a pool of random numbers early, on all
> > systems including embedded devices, not just servers.
> 
> I'm not using the term regression incorrectly here. Wrong function
> was used to generate random numbers for the payload here. It is an
> obvious bug.

At the time when trusted keys was introduced I'd say that it was a wrong
design decision and badly implemented code. But you are right in that as
far that code is considered it would unfair to speak of a regression.

asym-tpm.c on the other hand this is fresh new code. There has been
*countless* of discussions over the years that random numbers should
come from multiple sources of entropy. There is no other categorization
than a bug for the tpm_get_random() there.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Mimi Zohar]

On Fri, 2019-10-04 at 00:57 +0300, Jarkko Sakkinen wrote:
> On Fri, Oct 04, 2019 at 12:51:25AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 03, 2019 at 02:53:47PM -0400, Mimi Zohar wrote:
> > > [Cc'ing David Safford]
> > > 
> > > On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > > > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > > > > > Only the kernel random pool should be used for generating random numbers.
> > > > > > > > TPM contributes to that pool among the other sources of entropy. In here it
> > > > > > > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > > > > > > but in order to remove tpm_get_random() we need to first remove all the
> > > > > > > > call sites.
> > > > > > > 
> > > > > > > At what point during boot is the kernel random pool available?  Does
> > > > > > > this imply that you're planning on changing trusted keys as well?
> > > > > > 
> > > > > > Well trusted keys *must* be changed to use it. It is not a choice
> > > > > > because using a proprietary random number generator instead of defacto
> > > > > > one in the kernel can be categorized as a *regression*.
> > > > > 
> > > > > I really don't see how using the TPM random number for TPM trusted
> > > > > keys would be considered a regression.  That by definition is a
> > > > > trusted key.  If anything, changing what is currently being done would
> > > > > be the regression. 
> > > > 
> > > > It is really not a TPM trusted key. It trusted key that gets sealed with
> > > > the TPM. The key itself is used in clear by kernel. The random number
> > > > generator exists in the kernel to for a reason.
> > > > 
> > > > It is without doubt a regression.
> > > 
> > > You're misusing the term "regression" here.  A regression is something
> > > that previously worked and has stopped working.  In this case, trusted
> > > keys has always been based on the TPM random number generator.  Before
> > > changing this, there needs to be some guarantees that the kernel
> > > random number generator has a pool of random numbers early, on all
> > > systems including embedded devices, not just servers.
> > 
> > I'm not using the term regression incorrectly here. Wrong function
> > was used to generate random numbers for the payload here. It is an
> > obvious bug.
> 
> At the time when trusted keys was introduced I'd say that it was a wrong
> design decision and badly implemented code. But you are right in that as
> far that code is considered it would unfair to speak of a regression.
> 
> asym-tpm.c on the other hand this is fresh new code. There has been
> *countless* of discussions over the years that random numbers should
> come from multiple sources of entropy. There is no other categorization
> than a bug for the tpm_get_random() there.

This week's LWN article on "5.4 Merge window, part 2" discusses "boot-
time entropy".  This article couldn't have been more perfectly timed.

Mimi

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Thu, 2019-10-03 at 18:08 -0400, Mimi Zohar wrote:
> On Fri, 2019-10-04 at 00:57 +0300, Jarkko Sakkinen wrote:
> > On Fri, Oct 04, 2019 at 12:51:25AM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Oct 03, 2019 at 02:53:47PM -0400, Mimi Zohar wrote:
> > > > [Cc'ing David Safford]
> > > > 
> > > > On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> > > > > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > > > > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > > > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar
> > > > > > > wrote:
> > > > > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen
> > > > > > > > wrote:
> > > > > > > > > Only the kernel random pool should be used for
> > > > > > > > > generating random numbers.
> > > > > > > > > TPM contributes to that pool among the other sources
> > > > > > > > > of entropy. In here it
> > > > > > > > > is not, agreed, absolutely critical because TPM is
> > > > > > > > > what is trusted anyway
> > > > > > > > > but in order to remove tpm_get_random() we need to
> > > > > > > > > first remove all the
> > > > > > > > > call sites.
> > > > > > > > 
> > > > > > > > At what point during boot is the kernel random pool
> > > > > > > > available?  Does this imply that you're planning on
> > > > > > > > changing trusted keys as well?
> > > > > > > 
> > > > > > > Well trusted keys *must* be changed to use it. It is not
> > > > > > > a choice because using a proprietary random number
> > > > > > > generator instead of defacto one in the kernel can be
> > > > > > > categorized as a *regression*.
> > > > > > 
> > > > > > I really don't see how using the TPM random number for TPM
> > > > > > trusted keys would be considered a regression.  That by
> > > > > > definition is a trusted key.  If anything, changing what is
> > > > > > currently being done would be the regression. 
> > > > > 
> > > > > It is really not a TPM trusted key. It trusted key that gets
> > > > > sealed with the TPM. The key itself is used in clear by
> > > > > kernel. The random number generator exists in the kernel to
> > > > > for a reason.
> > > > > 
> > > > > It is without doubt a regression.
> > > > 
> > > > You're misusing the term "regression" here.  A regression is
> > > > something that previously worked and has stopped working.  In
> > > > this case, trusted keys has always been based on the TPM random
> > > > number generator.  Before changing this, there needs to be some
> > > > guarantees that the kernel random number generator has a pool
> > > > of random numbers early, on all systems including embedded
> > > > devices, not just servers.
> > > 
> > > I'm not using the term regression incorrectly here. Wrong
> > > function was used to generate random numbers for the payload
> > > here. It is an obvious bug.
> > 
> > At the time when trusted keys was introduced I'd say that it was a
> > wrong design decision and badly implemented code. But you are right
> > in that as far that code is considered it would unfair to speak of
> > a regression.

I think that's a bit unfair: when the code was written, a decade ago,
the TPM was thought to be the state of the art in terms of random
number generators.  Of course, since then the kernel RNG has become way
better and we've become more aware of nation state actors thinking that
influencing the RNG is the best way to compromise strong crypto.

> > asym-tpm.c on the other hand this is fresh new code. There has been
> > *countless* of discussions over the years that random numbers
> > should come from multiple sources of entropy. There is no other
> > categorization than a bug for the tpm_get_random() there.
> 
> This week's LWN article on "5.4 Merge window, part 2" discusses
> "boot- time entropy".  This article couldn't have been more perfectly
> timed.

I think the principle of using multiple RNG sources for strong keys is
a sound one, so could I propose a compromise:  We have a tpm subsystem
random number generator that, when asked for <n> random bytes first
extracts <n> bytes from the TPM RNG and places it into the kernel
entropy pool and then asks for <n> random bytes from the kernel RNG? 
That way, it will always have the entropy to satisfy the request and in
the worst case, where the kernel has picked up no other entropy sources
at all it will be equivalent to what we have now (single entropy
source) but usually it will be a much better mixed entropy source.

James

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote:
> I think the principle of using multiple RNG sources for strong keys is
> a sound one, so could I propose a compromise:  We have a tpm subsystem
> random number generator that, when asked for <n> random bytes first
> extracts <n> bytes from the TPM RNG and places it into the kernel
> entropy pool and then asks for <n> random bytes from the kernel RNG? 
> That way, it will always have the entropy to satisfy the request and in
> the worst case, where the kernel has picked up no other entropy sources
> at all it will be equivalent to what we have now (single entropy
> source) but usually it will be a much better mixed entropy source.

I think we should rely the existing architecture where TPM is
contributing to the entropy pool as hwrng.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote:
> > I think the principle of using multiple RNG sources for strong keys
> > is a sound one, so could I propose a compromise:  We have a tpm
> > subsystem random number generator that, when asked for <n> random
> > bytes first extracts <n> bytes from the TPM RNG and places it into
> > the kernel entropy pool and then asks for <n> random bytes from the
> > kernel RNG? That way, it will always have the entropy to satisfy
> > the request and in the worst case, where the kernel has picked up
> > no other entropy sources at all it will be equivalent to what we
> > have now (single entropy source) but usually it will be a much
> > better mixed entropy source.
> 
> I think we should rely the existing architecture where TPM is
> contributing to the entropy pool as hwrng.

That doesn't seem to work: when I trace what happens I see us inject 32
bytes of entropy at boot time, but never again.  I think the problem is
the kernel entropy pool is push not pull and we have no triggering
event in the TPM to get us to push.  I suppose we could set a timer to
do this or perhaps there is a pull hook and we haven't wired it up
correctly?

James

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jerry Snitselaar]

On Fri Oct 04 19, James Bottomley wrote:
>On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
>> On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote:
>> > I think the principle of using multiple RNG sources for strong keys
>> > is a sound one, so could I propose a compromise:  We have a tpm
>> > subsystem random number generator that, when asked for <n> random
>> > bytes first extracts <n> bytes from the TPM RNG and places it into
>> > the kernel entropy pool and then asks for <n> random bytes from the
>> > kernel RNG? That way, it will always have the entropy to satisfy
>> > the request and in the worst case, where the kernel has picked up
>> > no other entropy sources at all it will be equivalent to what we
>> > have now (single entropy source) but usually it will be a much
>> > better mixed entropy source.
>>
>> I think we should rely the existing architecture where TPM is
>> contributing to the entropy pool as hwrng.
>
>That doesn't seem to work: when I trace what happens I see us inject 32
>bytes of entropy at boot time, but never again.  I think the problem is
>the kernel entropy pool is push not pull and we have no triggering
>event in the TPM to get us to push.  I suppose we could set a timer to
>do this or perhaps there is a pull hook and we haven't wired it up
>correctly?
>
>James
>

Shouldn't hwrng_fillfn be pulling from it?

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote:
> On Fri Oct 04 19, James Bottomley wrote:
> > On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
> > > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote:
> > > > I think the principle of using multiple RNG sources for strong
> > > > keys is a sound one, so could I propose a compromise:  We have
> > > > a tpm subsystem random number generator that, when asked for
> > > > <n> random bytes first extracts <n> bytes from the TPM RNG and
> > > > places it into the kernel entropy pool and then asks for <n>
> > > > random bytes from the kernel RNG? That way, it will always have
> > > > the entropy to satisfy the request and in the worst case, where
> > > > the kernel has picked up no other entropy sources at all it
> > > > will be equivalent to what we have now (single entropy source)
> > > > but usually it will be a much better mixed entropy source.
> > > 
> > > I think we should rely the existing architecture where TPM is
> > > contributing to the entropy pool as hwrng.
> > 
> > That doesn't seem to work: when I trace what happens I see us
> > inject 32 bytes of entropy at boot time, but never again.  I think
> > the problem is the kernel entropy pool is push not pull and we have
> > no triggering event in the TPM to get us to push.  I suppose we
> > could set a timer to do this or perhaps there is a pull hook and we
> > haven't wired it up correctly?
> > 
> > James
> > 
> 
> Shouldn't hwrng_fillfn be pulling from it?

It should, but the problem seems to be it only polls the "current" hw
rng ... it doesn't seem to have a concept that there may be more than
one.  What happens, according to a brief reading of the code, is when
multiple are registered, it determines what the "best" one is and then
only pulls from that.  What I think it should be doing is filling from
all of them using the entropy quality to adjust how many bits we get.

James

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jerry Snitselaar]

On Fri Oct 04 19, James Bottomley wrote:
>On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote:
>> On Fri Oct 04 19, James Bottomley wrote:
>> > On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
>> > > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote:
>> > > > I think the principle of using multiple RNG sources for strong
>> > > > keys is a sound one, so could I propose a compromise:  We have
>> > > > a tpm subsystem random number generator that, when asked for
>> > > > <n> random bytes first extracts <n> bytes from the TPM RNG and
>> > > > places it into the kernel entropy pool and then asks for <n>
>> > > > random bytes from the kernel RNG? That way, it will always have
>> > > > the entropy to satisfy the request and in the worst case, where
>> > > > the kernel has picked up no other entropy sources at all it
>> > > > will be equivalent to what we have now (single entropy source)
>> > > > but usually it will be a much better mixed entropy source.
>> > >
>> > > I think we should rely the existing architecture where TPM is
>> > > contributing to the entropy pool as hwrng.
>> >
>> > That doesn't seem to work: when I trace what happens I see us
>> > inject 32 bytes of entropy at boot time, but never again.  I think
>> > the problem is the kernel entropy pool is push not pull and we have
>> > no triggering event in the TPM to get us to push.  I suppose we
>> > could set a timer to do this or perhaps there is a pull hook and we
>> > haven't wired it up correctly?
>> >
>> > James
>> >
>>
>> Shouldn't hwrng_fillfn be pulling from it?
>
>It should, but the problem seems to be it only polls the "current" hw
>rng ... it doesn't seem to have a concept that there may be more than
>one.  What happens, according to a brief reading of the code, is when
>multiple are registered, it determines what the "best" one is and then
>only pulls from that.  What I think it should be doing is filling from
>all of them using the entropy quality to adjust how many bits we get.
>
>James
>

Most of them don't even set quality, including the tpm, so they end up
at the end of the list. For the ones that do I'm not sure how they determined
the value. For example virtio-rng sets quality to 1000.

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jerry Snitselaar]

On Fri Oct 04 19, Jerry Snitselaar wrote:
>On Fri Oct 04 19, James Bottomley wrote:
>>On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote:
>>>On Fri Oct 04 19, James Bottomley wrote:
>>>> On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
>>>> > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote:
>>>> > > I think the principle of using multiple RNG sources for strong
>>>> > > keys is a sound one, so could I propose a compromise:  We have
>>>> > > a tpm subsystem random number generator that, when asked for
>>>> > > <n> random bytes first extracts <n> bytes from the TPM RNG and
>>>> > > places it into the kernel entropy pool and then asks for <n>
>>>> > > random bytes from the kernel RNG? That way, it will always have
>>>> > > the entropy to satisfy the request and in the worst case, where
>>>> > > the kernel has picked up no other entropy sources at all it
>>>> > > will be equivalent to what we have now (single entropy source)
>>>> > > but usually it will be a much better mixed entropy source.
>>>> >
>>>> > I think we should rely the existing architecture where TPM is
>>>> > contributing to the entropy pool as hwrng.
>>>>
>>>> That doesn't seem to work: when I trace what happens I see us
>>>> inject 32 bytes of entropy at boot time, but never again.  I think
>>>> the problem is the kernel entropy pool is push not pull and we have
>>>> no triggering event in the TPM to get us to push.  I suppose we
>>>> could set a timer to do this or perhaps there is a pull hook and we
>>>> haven't wired it up correctly?
>>>>
>>>> James
>>>>
>>>
>>>Shouldn't hwrng_fillfn be pulling from it?
>>
>>It should, but the problem seems to be it only polls the "current" hw
>>rng ... it doesn't seem to have a concept that there may be more than
>>one.  What happens, according to a brief reading of the code, is when
>>multiple are registered, it determines what the "best" one is and then
>>only pulls from that.  What I think it should be doing is filling from
>>all of them using the entropy quality to adjust how many bits we get.
>>
>>James
>>
>
>Most of them don't even set quality, including the tpm, so they end up
>at the end of the list. For the ones that do I'm not sure how they determined
>the value. For example virtio-rng sets quality to 1000.

I should have added that I like that idea though.

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Fri, 2019-10-04 at 13:11 -0700, Jerry Snitselaar wrote:
> On Fri Oct 04 19, Jerry Snitselaar wrote:
> > On Fri Oct 04 19, James Bottomley wrote:
> > > On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote:
> > > > On Fri Oct 04 19, James Bottomley wrote:
> > > > > On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote:
> > > > > > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley
> > > > > > wrote:
> > > > > > > I think the principle of using multiple RNG sources for
> > > > > > > strong keys is a sound one, so could I propose a
> > > > > > > compromise:  We have a tpm subsystem random number
> > > > > > > generator that, when asked for <n> random bytes first
> > > > > > > extracts <n> bytes from the TPM RNG and places it into
> > > > > > > the kernel entropy pool and then asks for <n> random
> > > > > > > bytes from the kernel RNG? That way, it will always have
> > > > > > > the entropy to satisfy the request and in the worst case,
> > > > > > > where the kernel has picked up no other entropy sources
> > > > > > > at all it will be equivalent to what we have now (single
> > > > > > > entropy source) but usually it will be a much better
> > > > > > > mixed entropy source.
> > > > > > 
> > > > > > I think we should rely the existing architecture where TPM
> > > > > > is contributing to the entropy pool as hwrng.
> > > > > 
> > > > > That doesn't seem to work: when I trace what happens I see us
> > > > > inject 32 bytes of entropy at boot time, but never again.  I
> > > > > think the problem is the kernel entropy pool is push not pull
> > > > > and we have no triggering event in the TPM to get us to
> > > > > push.  I suppose we could set a timer to do this or perhaps
> > > > > there is a pull hook and we haven't wired it up correctly?
> > > > > 
> > > > > James
> > > > > 
> > > > 
> > > > Shouldn't hwrng_fillfn be pulling from it?
> > > 
> > > It should, but the problem seems to be it only polls the
> > > "current" hw rng ... it doesn't seem to have a concept that there
> > > may be more than one.  What happens, according to a brief reading
> > > of the code, is when multiple are registered, it determines what
> > > the "best" one is and then only pulls from that.  What I think it
> > > should be doing is filling from all of them using the entropy
> > > quality to adjust how many bits we get.
> > > 
> > > James
> > > 
> > 
> > Most of them don't even set quality, including the tpm, so they end
> > up at the end of the list. For the ones that do I'm not sure how
> > they determined the value. For example virtio-rng sets quality to
> > 1000.
> 
> I should have added that I like that idea though.

OK, so I looked at how others implement this.  It turns out there's
only one other: the atheros rng and this is what it does:

drivers/net/wireless/ath/ath9k/rng.c

so rather than redoing the entirety of the TPM rng like this, I thought
it's easier to keep what we have (direct hwrng device) and plug our
tpm_get_random() function into the kernel rng like the below.  

James

---

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 3d6d394a8661..0794521c0784 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -536,7 +536,7 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
 
-	return tpm_get_random(chip, data, max);
+	return __tpm_get_random(chip, data, max);
 }
 
 static int tpm_add_hwrng(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d7a3888ad80f..14631cba000c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -24,6 +24,7 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
+#include <linux/random.h>
 #include <linux/tpm_eventlog.h>
 
 #include "tpm.h"
@@ -424,15 +425,11 @@ int tpm_pm_resume(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
-/**
- * tpm_get_random() - get random bytes from the TPM's RNG
- * @chip:	a &struct tpm_chip instance, %NULL for the default chip
- * @out:	destination buffer for the random bytes
- * @max:	the max number of bytes to write to @out
- *
- * Return: number of random bytes read or a negative error value.
+/*
+ * Internal interface for tpm_get_random(): gets the random string
+ * directly from the TPM without mixing into the linux rng.
  */
-int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
+int __tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 {
 	int rc;
 
@@ -451,6 +448,38 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	tpm_put_ops(chip);
 	return rc;
 }
+
+/**
+ * tpm_get_random() - get random bytes influenced by the TPM's RNG
+ * @chip:	a &struct tpm_chip instance, %NULL for the default chip
+ * @out:	destination buffer for the random bytes
+ * @max:	the max number of bytes to write to @out
+ *
+ * Uses the TPM as a source of input to the kernel random number
+ * generator and then takes @max bytes directly from the kernel.  In
+ * the worst (no other entropy) case, this will return the pure TPM
+ * random number, but if the kernel RNG has any entropy at all it will
+ * return a mixed entropy output which doesn't rely on a single
+ * source.
+ *
+ * Return: number of random bytes read or a negative error value.
+ */
+int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
+{
+	int rc;
+
+	rc = __tpm_get_random(chip, out, max);
+	if (rc <= 0)
+		return rc;
+	/*
+	 * assume the TPM produces pure randomness, so the amount of
+	 * entropy is the number of bits returned
+	 */
+	add_hwgenerator_randomness(out, rc, rc * 8);
+	get_random_bytes(out, rc);
+
+	return rc;
+}
 EXPORT_SYMBOL_GPL(tpm_get_random);
 
 /**
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..25f6b347b194 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -398,6 +398,7 @@ int tpm1_get_pcr_allocation(struct tpm_chip *chip);
 unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm_pm_suspend(struct device *dev);
 int tpm_pm_resume(struct device *dev);
+int __tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
 
 static inline void tpm_msleep(unsigned int delay_msec)
 {

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Mimi Zohar]

On Fri, 2019-10-04 at 15:11 -0700, James Bottomley wrote:

> +
> +/**
> + * tpm_get_random() - get random bytes influenced by the TPM's RNG
> + * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> + * @out:	destination buffer for the random bytes
> + * @max:	the max number of bytes to write to @out
> + *
> + * Uses the TPM as a source of input to the kernel random number
> + * generator and then takes @max bytes directly from the kernel.  In
> + * the worst (no other entropy) case, this will return the pure TPM
> + * random number, but if the kernel RNG has any entropy at all it will
> + * return a mixed entropy output which doesn't rely on a single
> + * source.
> + *
> + * Return: number of random bytes read or a negative error value.
> + */
> +int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
> +{
> +	int rc;
> +
> +	rc = __tpm_get_random(chip, out, max);
> +	if (rc <= 0)
> +		return rc;
> +	/*
> +	 * assume the TPM produces pure randomness, so the amount of
> +	 * entropy is the number of bits returned
> +	 */
> +	add_hwgenerator_randomness(out, rc, rc * 8);
> +	get_random_bytes(out, rc);

Using the TPM as a source of input to the kernel random number
generator is fine, but please don't change the meaning of trusted
keys.  The trusted-encrypted keys documentation clearly states
"Trusted Keys use a TPM both to generate and to seal the keys."

If you really want to use a different random number source instead of
the TPM, then define a new trusted key option (eg. rng=kernel), with
the default being the TPM.

Mimi


> +
> +	return rc;
> +}
>  EXPORT_SYMBOL_GPL(tpm_get_random);

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Sat, Oct 05, 2019 at 08:38:53PM -0400, Mimi Zohar wrote:
> On Fri, 2019-10-04 at 15:11 -0700, James Bottomley wrote:
> 
> > +
> > +/**
> > + * tpm_get_random() - get random bytes influenced by the TPM's RNG
> > + * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> > + * @out:	destination buffer for the random bytes
> > + * @max:	the max number of bytes to write to @out
> > + *
> > + * Uses the TPM as a source of input to the kernel random number
> > + * generator and then takes @max bytes directly from the kernel.  In
> > + * the worst (no other entropy) case, this will return the pure TPM
> > + * random number, but if the kernel RNG has any entropy at all it will
> > + * return a mixed entropy output which doesn't rely on a single
> > + * source.
> > + *
> > + * Return: number of random bytes read or a negative error value.
> > + */
> > +int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
> > +{
> > +	int rc;
> > +
> > +	rc = __tpm_get_random(chip, out, max);
> > +	if (rc <= 0)
> > +		return rc;
> > +	/*
> > +	 * assume the TPM produces pure randomness, so the amount of
> > +	 * entropy is the number of bits returned
> > +	 */
> > +	add_hwgenerator_randomness(out, rc, rc * 8);
> > +	get_random_bytes(out, rc);
> 
> Using the TPM as a source of input to the kernel random number
> generator is fine, but please don't change the meaning of trusted
> keys.  The trusted-encrypted keys documentation clearly states
> "Trusted Keys use a TPM both to generate and to seal the keys."
> 
> If you really want to use a different random number source instead of
> the TPM, then define a new trusted key option (eg. rng=kernel), with
> the default being the TPM.

I'll add a patch that updates the documentation because it is clearly
not a good practice to use TPM to generate keys that are not keys
residing inside the TPM.

With TEE coming in, TPM is not the only hardware measure anymore sealing
the keys and we don't want a mess where every hardware asset does their
own proprietary key generation. The proprietary technology should only
take care of the sealing part.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Mimi Zohar]

On Mon, 2019-10-07 at 02:52 +0300, Jarkko Sakkinen wrote:
> 
> With TEE coming in, TPM is not the only hardware measure anymore sealing
> the keys and we don't want a mess where every hardware asset does their
> own proprietary key generation. The proprietary technology should only
> take care of the sealing part.

I'm fine with the concept of "trusted" keys being extended beyond just
TPM.  But just as the VFS layer defines a set of callbacks and generic
functions, which can be used in lieu of file system specific callback
functions, a similar approach could be used here.

Mimi

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 06:08:11PM -0400, Mimi Zohar wrote:
> > At the time when trusted keys was introduced I'd say that it was a wrong
> > design decision and badly implemented code. But you are right in that as
> > far that code is considered it would unfair to speak of a regression.
> > 
> > asym-tpm.c on the other hand this is fresh new code. There has been
> > *countless* of discussions over the years that random numbers should
> > come from multiple sources of entropy. There is no other categorization
> > than a bug for the tpm_get_random() there.
> 
> This week's LWN article on "5.4 Merge window, part 2" discusses "boot-
> time entropy".  This article couldn't have been more perfectly timed.

Do not see any obvious relation to this dicussion. Are you saying that
you should not use the defacto kernel API's but instead bake your own
hacks because even defacto stuff bumps into issues from time to time?

And BTW, at the time you call tpm_get_random(), TPM driver is already
contributing to the entropy pool (registered as hwrng).

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Fri, Oct 04, 2019 at 12:57:43AM +0300, Jarkko Sakkinen wrote:
> On Fri, Oct 04, 2019 at 12:51:25AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 03, 2019 at 02:53:47PM -0400, Mimi Zohar wrote:
> > > [Cc'ing David Safford]
> > > 
> > > On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > > > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > > > > > Only the kernel random pool should be used for generating random numbers.
> > > > > > > > TPM contributes to that pool among the other sources of entropy. In here it
> > > > > > > > is not, agreed, absolutely critical because TPM is what is trusted anyway
> > > > > > > > but in order to remove tpm_get_random() we need to first remove all the
> > > > > > > > call sites.
> > > > > > > 
> > > > > > > At what point during boot is the kernel random pool available?  Does
> > > > > > > this imply that you're planning on changing trusted keys as well?
> > > > > > 
> > > > > > Well trusted keys *must* be changed to use it. It is not a choice
> > > > > > because using a proprietary random number generator instead of defacto
> > > > > > one in the kernel can be categorized as a *regression*.
> > > > > 
> > > > > I really don't see how using the TPM random number for TPM trusted
> > > > > keys would be considered a regression.  That by definition is a
> > > > > trusted key.  If anything, changing what is currently being done would
> > > > > be the regression. 
> > > > 
> > > > It is really not a TPM trusted key. It trusted key that gets sealed with
> > > > the TPM. The key itself is used in clear by kernel. The random number
> > > > generator exists in the kernel to for a reason.
> > > > 
> > > > It is without doubt a regression.
> > > 
> > > You're misusing the term "regression" here.  A regression is something
> > > that previously worked and has stopped working.  In this case, trusted
> > > keys has always been based on the TPM random number generator.  Before
> > > changing this, there needs to be some guarantees that the kernel
> > > random number generator has a pool of random numbers early, on all
> > > systems including embedded devices, not just servers.
> > 
> > I'm not using the term regression incorrectly here. Wrong function
> > was used to generate random numbers for the payload here. It is an
> > obvious bug.
> 
> At the time when trusted keys was introduced I'd say that it was a wrong
> design decision and badly implemented code. But you are right in that as
> far that code is considered it would unfair to speak of a regression.
> 
> asym-tpm.c on the other hand this is fresh new code. There has been
> *countless* of discussions over the years that random numbers should
> come from multiple sources of entropy. There is no other categorization
> than a bug for the tpm_get_random() there.

Saying that regression is something that "stopped working" is very blunt
and naive definition of regression, and is not true. Any misbehaviour
can be categorized as a regression.

/Jarkko

[PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Safford, David (GE Global Research, US)]

> From: Mimi Zohar <zohar@linux.ibm.com>
> Sent: Thursday, October 3, 2019 2:54 PM
> To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Safford, David (GE
> Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> 
> [Cc'ing David Safford]
> 
> On Thu, 2019-10-03 at 20:58 +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > > On Thu, 2019-10-03 at 14:41 +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Oct 02, 2019 at 10:00:19AM -0400, Mimi Zohar wrote:
> > > > > On Thu, 2019-09-26 at 20:16 +0300, Jarkko Sakkinen wrote:
> > > > > > Only the kernel random pool should be used for generating random
> numbers.
> > > > > > TPM contributes to that pool among the other sources of
> > > > > > entropy. In here it is not, agreed, absolutely critical
> > > > > > because TPM is what is trusted anyway but in order to remove
> > > > > > tpm_get_random() we need to first remove all the call sites.
> > > > >
> > > > > At what point during boot is the kernel random pool available?
> > > > > Does this imply that you're planning on changing trusted keys as well?
> > > >
> > > > Well trusted keys *must* be changed to use it. It is not a choice
> > > > because using a proprietary random number generator instead of
> > > > defacto one in the kernel can be categorized as a *regression*.
> > >
> > > I really don't see how using the TPM random number for TPM trusted
> > > keys would be considered a regression.  That by definition is a
> > > trusted key.  If anything, changing what is currently being done
> > > would be the regression.
> >
> > It is really not a TPM trusted key. It trusted key that gets sealed
> > with the TPM. The key itself is used in clear by kernel. The random
> > number generator exists in the kernel to for a reason.
> >
> > It is without doubt a regression.
> 
> You're misusing the term "regression" here.  A regression is something that
> previously worked and has stopped working.  In this case, trusted keys has
> always been based on the TPM random number generator.  Before changing
> this, there needs to be some guarantees that the kernel random number
> generator has a pool of random numbers early, on all systems including
> embedded devices, not just servers.
> 
> Mimi

As the original author of trusted keys, let me make a few comments.
First, trusted keys were specifically implemented and *documented* to
use the TPM to both generate and seal keys. Its kernel documentation
specifically states this as a promise to user space. If you want to have 
a different key system that uses the random pool to generate the keys,
fine, but don't change trusted keys, as that changes the existing promise
to user space. 

There are many good reasons for wanting the keys to be based on the
TPM generator.  As the source for the kernel random number generator
itself says, some systems lack good randomness at startup, and systems
should preserve and reload the pool across shutdown and startup.
There are use cases for trusted keys which need to generate keys 
before such scripts have run. Also, in some use cases, we need to show
that trusted keys are FIPS compliant, which is possible with TPM
generated keys.

Second, the TPM is hardly a "proprietary random number generator".
It is an open standard with multiple implementations, many of which are
FIPS certified.

Third, as Mimi states, using a TPM is not a "regression". It would be a
regression to change trusted keys _not_ to use the TPM, because that
is what trusted keys are documented to provide to user space.

dave

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Fri, Oct 04, 2019 at 01:26:58PM +0000, Safford, David (GE Global Research, US) wrote:
> As the original author of trusted keys, let me make a few comments.
> First, trusted keys were specifically implemented and *documented* to
> use the TPM to both generate and seal keys. Its kernel documentation
> specifically states this as a promise to user space. If you want to have 
> a different key system that uses the random pool to generate the keys,
> fine, but don't change trusted keys, as that changes the existing promise
> to user space. 

TPM generating keys (i.e. the random number) would make sense if the key
would never leave from TPM (that kind of trusted keys would not be a
bad idea at all).

> There are many good reasons for wanting the keys to be based on the
> TPM generator.  As the source for the kernel random number generator
> itself says, some systems lack good randomness at startup, and systems
> should preserve and reload the pool across shutdown and startup.
> There are use cases for trusted keys which need to generate keys 
> before such scripts have run. Also, in some use cases, we need to show
> that trusted keys are FIPS compliant, which is possible with TPM
> generated keys.

If you are able to call tpm_get_random(), the driver has already
registered TPN as hwrng. With this solution you fail to follow the
principle of defense in depth. If the TPM random number generator
is compromissed (has a bug) using the entropy pool will decrease
the collateral damage.

> Second, the TPM is hardly a "proprietary random number generator".
> It is an open standard with multiple implementations, many of which are
> FIPS certified.
> 
> Third, as Mimi states, using a TPM is not a "regression". It would be a
> regression to change trusted keys _not_ to use the TPM, because that
> is what trusted keys are documented to provide to user space.

For asym-tpm.c it is without a question a regression because of the
evolution that has happened after trusted keys. For trusted keys
using kernel rng would be improvement.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

> > There are many good reasons for wanting the keys to be based on the
> > TPM generator.  As the source for the kernel random number generator
> > itself says, some systems lack good randomness at startup, and systems
> > should preserve and reload the pool across shutdown and startup.
> > There are use cases for trusted keys which need to generate keys 
> > before such scripts have run. Also, in some use cases, we need to show
> > that trusted keys are FIPS compliant, which is possible with TPM
> > generated keys.
> 
> If you are able to call tpm_get_random(), the driver has already
> registered TPN as hwrng. With this solution you fail to follow the
> principle of defense in depth. If the TPM random number generator
> is compromissed (has a bug) using the entropy pool will decrease
> the collateral damage.

I.e. you make everything depend on single point of failure instead
of multiple (e.g. rdrand, TPM, whatnot).

/Jarkko

[PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Safford, David (GE Global Research, US)]

> From: linux-integrity-owner@vger.kernel.org <linux-integrity-
> owner@vger.kernel.org> On Behalf Of Jarkko Sakkinen
> Sent: Friday, October 4, 2019 2:27 PM
> Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> 
> If you are able to call tpm_get_random(), the driver has already registered
> TPN as hwrng. With this solution you fail to follow the principle of defense in
> depth. If the TPM random number generator is compromissed (has a bug)
> using the entropy pool will decrease the collateral damage.

And if the entropy pool has a bug or is misconfigured, you lose everything.
That does not sound like defense in depth to me. In the real world
I am not aware of a single instance of RNG vulnerability on a TPM.
I am directly aware of several published vulnerabilities in embedded systems 
due to a badly ported version of the kernel random pool. In addition, 
the random generator in a TPM is hardware isolated, and less likely to be
vulnerable to side channel or memory manipulation errors. The TPM
RNG is typically FIPS certified.  The use of the TPM RNG was a deliberate
design choice in trusted keys.

> > Third, as Mimi states, using a TPM is not a "regression". It would be
> > a regression to change trusted keys _not_ to use the TPM, because that
> > is what trusted keys are documented to provide to user space.
> 
> For asym-tpm.c it is without a question a regression because of the evolution
> that has happened after trusted keys. For trusted keys using kernel rng
> would be improvement.

Perhaps this is a language issue, but you are not using "regression" correctly.
Changing to the kernel pool would not only be a debatable  "improvement", 
but also would certainly be a change to the documented trusted key  
behavior, which I thought was frowned upon.

dave

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Fri, Oct 04, 2019 at 07:56:01PM +0000, Safford, David (GE Global Research, US) wrote:
> 
> > From: linux-integrity-owner@vger.kernel.org <linux-integrity-
> > owner@vger.kernel.org> On Behalf Of Jarkko Sakkinen
> > Sent: Friday, October 4, 2019 2:27 PM
> > Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> > 
> > If you are able to call tpm_get_random(), the driver has already registered
> > TPN as hwrng. With this solution you fail to follow the principle of defense in
> > depth. If the TPM random number generator is compromissed (has a bug)
> > using the entropy pool will decrease the collateral damage.
> 
> And if the entropy pool has a bug or is misconfigured, you lose everything.
> That does not sound like defense in depth to me. In the real world
> I am not aware of a single instance of RNG vulnerability on a TPM.
> I am directly aware of several published vulnerabilities in embedded systems 
> due to a badly ported version of the kernel random pool. In addition, 
> the random generator in a TPM is hardware isolated, and less likely to be
> vulnerable to side channel or memory manipulation errors. The TPM
> RNG is typically FIPS certified.  The use of the TPM RNG was a deliberate
> design choice in trusted keys.

Hmm... so is RDRAND opcode FIPS certified.

Kernel has the random number generator for two reasons:

1. To protect against bugs in hwrng's.
2. To protect against deliberate backdoors in hwrng's.

How TPM RNG is guaranteed to protect against both 1 and 2?

If I would agree what you say, that'd be argument against using kernel
random number generator *anywhere* in the kernel. Even with the entropy
issues it is least worst thing to use for key generations for better
or worse.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Ken Goldman]

The TPM library specification states that the TPM must comply with NIST 
SP800-90 A.

https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/

shows that the TPMs get third party certification, Common Criteria EAL 4+.

While it's theoretically possible that an attacker could compromise
both the TPM vendors and the evaluation agencies, we do have EAL 4+ 
assurance against both 1 and 2.

On 10/6/2019 8:05 PM, Jarkko Sakkinen wrote:
> 
> Kernel has the random number generator for two reasons:
> 
> 1. To protect against bugs in hwrng's.
> 2. To protect against deliberate backdoors in hwrng's.
> 
> How TPM RNG is guaranteed to protect against both 1 and 2?

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> The TPM library specification states that the TPM must comply with NIST
> SP800-90 A.
> 
> https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> 
> shows that the TPMs get third party certification, Common Criteria EAL 4+.
> 
> While it's theoretically possible that an attacker could compromise
> both the TPM vendors and the evaluation agencies, we do have EAL 4+
> assurance against both 1 and 2.

Certifications do not equal to trust.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > The TPM library specification states that the TPM must comply with NIST
> > SP800-90 A.
> > 
> > https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> > 
> > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > 
> > While it's theoretically possible that an attacker could compromise
> > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > assurance against both 1 and 2.
> 
> Certifications do not equal to trust.

And for trusted keys the least trust solution is to do generation
with the kernel assets and sealing with TPM. With TEE the least
trust solution is equivalent.

Are you proposing that the kernel random number generation should
be removed? That would be my conclusion of this discussion if I
would agree any of this (I don't).

/Jarkko

RE: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Pascal Van Leeuwen]

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> Jarkko Sakkinen
> Sent: Wednesday, October 9, 2019 1:54 AM
> To: Ken Goldman <kgold@linux.ibm.com>
> Cc: Safford, David (GE Global Research, US) <david.safford@ge.com>; Mimi Zohar
> <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org; stable@vger.kernel.org; open
> list:ASYMMETRIC KEYS <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> 
> On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > The TPM library specification states that the TPM must comply with NIST
> > > SP800-90 A.
> > >
> > > https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> > >
> > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > >
> > > While it's theoretically possible that an attacker could compromise
> > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > assurance against both 1 and 2.
> >
> > Certifications do not equal to trust.
> 
So having an implementation reviewed by a capable third party of 
your choosing (as that's how certification usually works) is less
trustworthy than having random individuals hacking away at it?
(and trust me, _most_ people are not going to review that by 
themselves - very few people on this planet are capable to do so)

> And for trusted keys the least trust solution is to do generation
> with the kernel assets and sealing with TPM. With TEE the least
> trust solution is equivalent.
> 
> Are you proposing that the kernel random number generation should
> be removed? That would be my conclusion of this discussion if I
> would agree any of this (I don't).
> 
Life is not that black and white.

If certification is _not_ a requirement you can use the kernel random
number generator, especially if you don't trust, say, the TPM one.
If you _do_ require certification - and in many industries this is
_mandatory_, you simply _must_ follow the certification rules (which
may impose restrictions how the random number generation is done),
and this should not be made impossible for such _existing_ use cases.

Having said all that, generating _true_ entropy (and, importantly,
ensuring it cannot be manipulated) is a very complicated subject and
considering how all encryption security ultimately  depends on the
quality of the random numbers used for key material, I would not 
trust any implementation that has not been certified or otherwise 
carefully scrutinized by people _proven_ to have the expertise.

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 09, 2019 at 02:53:39AM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > The TPM library specification states that the TPM must comply with NIST
> > > SP800-90 A.
> > > 
> > > https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> > > 
> > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > > 
> > > While it's theoretically possible that an attacker could compromise
> > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > assurance against both 1 and 2.
> > 
> > Certifications do not equal to trust.
> 
> And for trusted keys the least trust solution is to do generation
> with the kernel assets and sealing with TPM. With TEE the least
> trust solution is equivalent.
> 
> Are you proposing that the kernel random number generation should
> be removed? That would be my conclusion of this discussion if I
> would agree any of this (I don't).

The whole point of rng in kernel has been to use multiple entropy
sources in order to disclose the trust issue.

Even with weaker entropy than TPM RNG it is still a better choice for
*non-TPM* keys because of better trustworthiness. Using only TPM RNG is
a design flaw that has existed probably because when trusted keys were
introduced TPM was more niche than it is today.

Please remember that a trusted key is not a TPM key. The reality
distortion field is strong here it seems.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 09, 2019 at 10:33:15AM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 09, 2019 at 02:53:39AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > > The TPM library specification states that the TPM must comply with NIST
> > > > SP800-90 A.
> > > > 
> > > > https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> > > > 
> > > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > > > 
> > > > While it's theoretically possible that an attacker could compromise
> > > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > > assurance against both 1 and 2.
> > > 
> > > Certifications do not equal to trust.
> > 
> > And for trusted keys the least trust solution is to do generation
> > with the kernel assets and sealing with TPM. With TEE the least
> > trust solution is equivalent.
> > 
> > Are you proposing that the kernel random number generation should
> > be removed? That would be my conclusion of this discussion if I
> > would agree any of this (I don't).
> 
> The whole point of rng in kernel has been to use multiple entropy
> sources in order to disclose the trust issue.
> 
> Even with weaker entropy than TPM RNG it is still a better choice for
> *non-TPM* keys because of better trustworthiness. Using only TPM RNG is
> a design flaw that has existed probably because when trusted keys were
> introduced TPM was more niche than it is today.
> 
> Please remember that a trusted key is not a TPM key. The reality
> distortion field is strong here it seems.

And why not use RDRAND on x86 instead of TPM RNG here? It is also FIPS
compliant and has less latency than TPM RNG. :-) If we go with this
route, lets pick the HRNG that performs best.

/Jarkko

RE: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Pascal Van Leeuwen]

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> Jarkko Sakkinen
> Sent: Wednesday, October 9, 2019 9:42 AM
> To: Ken Goldman <kgold@linux.ibm.com>
> Cc: Safford, David (GE Global Research, US) <david.safford@ge.com>; Mimi Zohar
> <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org; stable@vger.kernel.org; open
> list:ASYMMETRIC KEYS <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> 
> On Wed, Oct 09, 2019 at 10:33:15AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 09, 2019 at 02:53:39AM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > > > The TPM library specification states that the TPM must comply with NIST
> > > > > SP800-90 A.
> > > > >
> > > > > https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> > > > >
> > > > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > > > >
> > > > > While it's theoretically possible that an attacker could compromise
> > > > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > > > assurance against both 1 and 2.
> > > >
> > > > Certifications do not equal to trust.
> > >
> > > And for trusted keys the least trust solution is to do generation
> > > with the kernel assets and sealing with TPM. With TEE the least
> > > trust solution is equivalent.
> > >
> > > Are you proposing that the kernel random number generation should
> > > be removed? That would be my conclusion of this discussion if I
> > > would agree any of this (I don't).
> >
> > The whole point of rng in kernel has been to use multiple entropy
> > sources in order to disclose the trust issue.
> >
> > Even with weaker entropy than TPM RNG it is still a better choice for
> > *non-TPM* keys because of better trustworthiness. Using only TPM RNG is
> > a design flaw that has existed probably because when trusted keys were
> > introduced TPM was more niche than it is today.
> >
> > Please remember that a trusted key is not a TPM key. The reality
> > distortion field is strong here it seems.
> 
> And why not use RDRAND on x86 instead of TPM RNG here? It is also FIPS
> compliant and has less latency than TPM RNG. :-) If we go with this
> route, lets pick the HRNG that performs best.
> 
There's certification and certification. Not all certificates are
created equally. But if it matches your specific requirements, why not.
There's a _lot_ of HW out there that's not x86 though ...

And: is RDRAND certified for _all_ x86 processors? Or just Intel?
Or perhaps even only _specific (server) models_ of CPU's?
I also know for a fact that some older AMD processors had a broken
RDRAND implementation ...

So the choice really should be up to the application or user.

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 09, 2019 at 08:09:29AM +0000, Pascal Van Leeuwen wrote:
> There's certification and certification. Not all certificates are
> created equally. But if it matches your specific requirements, why not.
> There's a _lot_ of HW out there that's not x86 though ...
> 
> And: is RDRAND certified for _all_ x86 processors? Or just Intel?
> Or perhaps even only _specific (server) models_ of CPU's?
> I also know for a fact that some older AMD processors had a broken
> RDRAND implementation ...
> 
> So the choice really should be up to the application or user.

I'm not seriously suggesting to move rdrand here. I'm trying find the
logic how to move forward with trusted keys with multiple backends (not
only TPM).

AKA we have a kernel rng in existence but no clear policy when it should
be used and when not. This leads to throwing a dice with the design
choices. Even it TPM RNG is the right choice in tpm_asym.c, the choice
was not based on anything (otherwise it would have been documented).

/Jarkko

RE: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Pascal Van Leeuwen]

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> Jarkko Sakkinen
> Sent: Wednesday, October 9, 2019 9:33 AM
> To: Ken Goldman <kgold@linux.ibm.com>
> Cc: Safford, David (GE Global Research, US) <david.safford@ge.com>; Mimi Zohar
> <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org; stable@vger.kernel.org; open
> list:ASYMMETRIC KEYS <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> 
> On Wed, Oct 09, 2019 at 02:53:39AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > > The TPM library specification states that the TPM must comply with NIST
> > > > SP800-90 A.
> > > >
> > > > https://trustedcomputinggroup.org/membership/certification/tpm-certified-products/
> > > >
> > > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > > >
> > > > While it's theoretically possible that an attacker could compromise
> > > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > > assurance against both 1 and 2.
> > >
> > > Certifications do not equal to trust.
> >
> > And for trusted keys the least trust solution is to do generation
> > with the kernel assets and sealing with TPM. With TEE the least
> > trust solution is equivalent.
> >
> > Are you proposing that the kernel random number generation should
> > be removed? That would be my conclusion of this discussion if I
> > would agree any of this (I don't).
> 
> The whole point of rng in kernel has been to use multiple entropy
> sources in order to disclose the trust issue.
> 
I do understand that, and combining multiple entropy sources, if
you have them available to get _more_ entropy is a good idea, at 
least in theory. But ...

How do I know the mixing of entropy happens properly? Especially
if I'm not capable of judging this by myself. 
And how do I know the SW entropy pool and/or code cannot be influenced
_somehow_? (either directly or indirectly by influencing one of the
contributors). More code and/or HW involved means more attack vectors
and complication of the review process.

The point is, if you want to certify such an application, you would
have to have _all_ contributors _plus_ the kernel rng code certified.
And you would have to have it _recertified_ every time a _single_
component - including the kernel code itself! - changes.

> Even with weaker entropy than TPM RNG it is still a better choice for
> *non-TPM* keys because of better trustworthiness.
>
"Even with weaker entropy"? Now that's just silly. If you _know_ and
_trust_ the TPM to have _better_ entropy, then obviously that is the
better choice. I guess the key word being the trust you don't have.

> Using only TPM RNG is
> a design flaw that has existed probably because when trusted keys were
> introduced TPM was more niche than it is today.
>
For non-TPM keys, possibly. Assuming the kernel RNG indeed adds 
(or at least does not weaken) entropy. And assuming I _can_ trust
the kernel RNG implementation. Question is: why would I trust that
more than the TPM implementation? Sure, I could look at the code,
but would I truly and fully understand it? (so maybe _I_ would,
but would Joe Random User?)

> Please remember that a trusted key is not a TPM key. The reality
> distortion field is strong here it seems.
> 
Agree. But you should not mess with the possibility to generate
keys based on _just_ the TPM RNG _where that is required_ (and
perhaps _only_ where that is required, if possible)

> /Jarkko

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

[PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Safford, David (GE Global Research, US)]

> From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Sent: Tuesday, October 8, 2019 7:54 PM
> To: Ken Goldman <kgold@linux.ibm.com>
> Cc: Safford, David (GE Global Research, US) <david.safford@ge.com>; Mimi
> Zohar <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org;
> stable@vger.kernel.org; open list:ASYMMETRIC KEYS
> <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> 
> On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > The TPM library specification states that the TPM must comply with
> > > NIST
> > > SP800-90 A.
> > >
> > > https://trustedcomputinggroup.org/membership/certification/tpm-certi
> > > fied-products/
> > >
> > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > >
> > > While it's theoretically possible that an attacker could compromise
> > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > assurance against both 1 and 2.
> >
> > Certifications do not equal to trust.
> 
> And for trusted keys the least trust solution is to do generation with the kernel
> assets and sealing with TPM. With TEE the least trust solution is equivalent.
> 
> Are you proposing that the kernel random number generation should be
> removed? That would be my conclusion of this discussion if I would agree any
> of this (I don't).
> 
> /Jarkko

No one is suggesting that.

You are suggesting changing the documented behavior of trusted keys, and
that would cause problems for some of our use cases. While certification
may not in your mind be equal to trust, it is equal to compliance with 
mandatory regulations.

Perhaps rather than arguing past each other, we should look into 
providing users the ability to choose, as an argument to keyctl?

dave

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 09, 2019 at 12:11:06PM +0000, Safford, David (GE Global Research, US) wrote:
> 
> > From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Sent: Tuesday, October 8, 2019 7:54 PM
> > To: Ken Goldman <kgold@linux.ibm.com>
> > Cc: Safford, David (GE Global Research, US) <david.safford@ge.com>; Mimi
> > Zohar <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org;
> > stable@vger.kernel.org; open list:ASYMMETRIC KEYS
> > <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> > crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> > Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> > 
> > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > > The TPM library specification states that the TPM must comply with
> > > > NIST
> > > > SP800-90 A.
> > > >
> > > > https://trustedcomputinggroup.org/membership/certification/tpm-certi
> > > > fied-products/
> > > >
> > > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > > >
> > > > While it's theoretically possible that an attacker could compromise
> > > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > > assurance against both 1 and 2.
> > >
> > > Certifications do not equal to trust.
> > 
> > And for trusted keys the least trust solution is to do generation with the kernel
> > assets and sealing with TPM. With TEE the least trust solution is equivalent.
> > 
> > Are you proposing that the kernel random number generation should be
> > removed? That would be my conclusion of this discussion if I would agree any
> > of this (I don't).
> > 
> > /Jarkko
> 
> No one is suggesting that.
> 
> You are suggesting changing the documented behavior of trusted keys, and
> that would cause problems for some of our use cases. While certification
> may not in your mind be equal to trust, it is equal to compliance with 
> mandatory regulations.
> 
> Perhaps rather than arguing past each other, we should look into 
> providing users the ability to choose, as an argument to keyctl?
> 
> dave

I'm taking my words back in the regression part as regression would need
really a failing system. Definitely the fixes tag should be removed from
my patch.

What is anyway the role of the kernel rng? Why does it exist and when
exactly it should be used? This exactly where the whole review process
throughout the "chain of command" failed misserably with tpm_asym.c.

The commit message for tpm_asym.c does not document the design choice in
any possible way and still was merged to the mainline.

Before knowning the answer to the "existential" question we are
somewhat paralyzed on moving forward with trusted keys (e.g. paralyzed
to merge TEE backend).

Your proposal might make sense but I don't really want to say anything
since I'm completely cluesless of the role of the kernel rng. Looks like
everyone who participated to the review process of tpm_asym.c, is too.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Mon, Oct 14, 2019 at 10:00:33PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 09, 2019 at 12:11:06PM +0000, Safford, David (GE Global Research, US) wrote:
> > 
> > > From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Sent: Tuesday, October 8, 2019 7:54 PM
> > > To: Ken Goldman <kgold@linux.ibm.com>
> > > Cc: Safford, David (GE Global Research, US) <david.safford@ge.com>; Mimi
> > > Zohar <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org;
> > > stable@vger.kernel.org; open list:ASYMMETRIC KEYS
> > > <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> > > crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> > > Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
> > > 
> > > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > > > The TPM library specification states that the TPM must comply with
> > > > > NIST
> > > > > SP800-90 A.
> > > > >
> > > > > https://trustedcomputinggroup.org/membership/certification/tpm-certi
> > > > > fied-products/
> > > > >
> > > > > shows that the TPMs get third party certification, Common Criteria EAL 4+.
> > > > >
> > > > > While it's theoretically possible that an attacker could compromise
> > > > > both the TPM vendors and the evaluation agencies, we do have EAL 4+
> > > > > assurance against both 1 and 2.
> > > >
> > > > Certifications do not equal to trust.
> > > 
> > > And for trusted keys the least trust solution is to do generation with the kernel
> > > assets and sealing with TPM. With TEE the least trust solution is equivalent.
> > > 
> > > Are you proposing that the kernel random number generation should be
> > > removed? That would be my conclusion of this discussion if I would agree any
> > > of this (I don't).
> > > 
> > > /Jarkko
> > 
> > No one is suggesting that.
> > 
> > You are suggesting changing the documented behavior of trusted keys, and
> > that would cause problems for some of our use cases. While certification
> > may not in your mind be equal to trust, it is equal to compliance with 
> > mandatory regulations.
> > 
> > Perhaps rather than arguing past each other, we should look into 
> > providing users the ability to choose, as an argument to keyctl?
> > 
> > dave
> 
> I'm taking my words back in the regression part as regression would need
> really a failing system. Definitely the fixes tag should be removed from
> my patch.
> 
> What is anyway the role of the kernel rng? Why does it exist and when
> exactly it should be used? This exactly where the whole review process
> throughout the "chain of command" failed misserably with tpm_asym.c.
> 
> The commit message for tpm_asym.c does not document the design choice in
> any possible way and still was merged to the mainline.
> 
> Before knowning the answer to the "existential" question we are
> somewhat paralyzed on moving forward with trusted keys (e.g. paralyzed
> to merge TEE backend).
> 
> Your proposal might make sense but I don't really want to say anything
> since I'm completely cluesless of the role of the kernel rng. Looks like
> everyone who participated to the review process of tpm_asym.c, is too.

As a ABI backwards compatibility workaround I'd agree most likely agree
with you. As a guideline for new features there should be a framework on
how to decide what to do.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Mon, 2019-10-14 at 22:00 +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 09, 2019 at 12:11:06PM +0000, Safford, David (GE Global
> Research, US) wrote:
> > 
> > > From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Sent: Tuesday, October 8, 2019 7:54 PM
> > > To: Ken Goldman <kgold@linux.ibm.com>
> > > Cc: Safford, David (GE Global Research, US) <david.safford@ge.com
> > > >; Mimi
> > > Zohar <zohar@linux.ibm.com>; linux-integrity@vger.kernel.org;
> > > stable@vger.kernel.org; open list:ASYMMETRIC KEYS
> > > <keyrings@vger.kernel.org>; open list:CRYPTO API <linux-
> > > crypto@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> > > Subject: EXT: Re: [PATCH] KEYS: asym_tpm: Switch to
> > > get_random_bytes()
> > > 
> > > On Wed, Oct 09, 2019 at 02:49:35AM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Oct 07, 2019 at 06:13:01PM -0400, Ken Goldman wrote:
> > > > > The TPM library specification states that the TPM must comply
> > > > > with NIST SP800-90 A.
> > > > > 
> > > > > https://trustedcomputinggroup.org/membership/certification/tp
> > > > > m-certified-products/
> > > > > 
> > > > > shows that the TPMs get third party certification, Common
> > > > > Criteria EAL 4+.
> > > > > 
> > > > > While it's theoretically possible that an attacker could
> > > > > compromise both the TPM vendors and the evaluation agencies,
> > > > > we do have EAL 4+ assurance against both 1 and 2.
> > > > 
> > > > Certifications do not equal to trust.
> > > 
> > > And for trusted keys the least trust solution is to do generation
> > > with the kernel assets and sealing with TPM. With TEE the least
> > > trust solution is equivalent.
> > > 
> > > Are you proposing that the kernel random number generation should
> > > be removed? That would be my conclusion of this discussion if I
> > > would agree any of this (I don't).
> > > 
> > > /Jarkko
> > 
> > No one is suggesting that.
> > 
> > You are suggesting changing the documented behavior of trusted
> > keys, and that would cause problems for some of our use cases.
> > While certification may not in your mind be equal to trust, it is
> > equal to compliance with mandatory regulations.
> > 
> > Perhaps rather than arguing past each other, we should look into 
> > providing users the ability to choose, as an argument to keyctl?
> > 
> > dave
> 
> I'm taking my words back in the regression part as regression would
> need really a failing system. Definitely the fixes tag should be
> removed from my patch.
> 
> What is anyway the role of the kernel rng? Why does it exist and when
> exactly it should be used? This exactly where the whole review
> process throughout the "chain of command" failed misserably with
> tpm_asym.c.
> 
> The commit message for tpm_asym.c does not document the design choice
> in any possible way and still was merged to the mainline.
> 
> Before knowning the answer to the "existential" question we are
> somewhat paralyzed on moving forward with trusted keys (e.g.
> paralyzed to merge TEE backend).
> 
> Your proposal might make sense but I don't really want to say
> anything since I'm completely cluesless of the role of the kernel
> rng. Looks like everyone who participated to the review process of
> tpm_asym.c, is too.

The job of the in-kernel rng is simply to produce a mixed entropy pool
from which we can draw random numbers.  The idea is that quite a few
attackers have identified the rng as being a weak point in the security
architecture of the kernel, so if we mix entropy from all the sources
we have, you have to compromise most of them to gain some predictive
power over the rng sequence.

The point is not how certified the TPM RNG is, the point is that it's a
single source and if we rely on it solely for some applications, like
trusted keys, then it gives the attackers a single known point to go
after.  This may be impossible for script kiddies, but it won't be for
nation states ... are you going to exclusively trust the random number
you got from your chinese certified TPM?

Remember also that the attack doesn't have to be to the TPM only, it
could be the pathway by which we get the random number, which involves
components outside of the TPM certification.

James

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Mon, Oct 14, 2019 at 12:29:57PM -0700, James Bottomley wrote:
> The job of the in-kernel rng is simply to produce a mixed entropy pool
> from which we can draw random numbers.  The idea is that quite a few
> attackers have identified the rng as being a weak point in the security
> architecture of the kernel, so if we mix entropy from all the sources
> we have, you have to compromise most of them to gain some predictive
> power over the rng sequence.

The documentation says that krng is suitable for key generation.
Should the documentation changed to state that it is unsuitable?

> The point is not how certified the TPM RNG is, the point is that it's a
> single source and if we rely on it solely for some applications, like
> trusted keys, then it gives the attackers a single known point to go
> after.  This may be impossible for script kiddies, but it won't be for
> nation states ... are you going to exclusively trust the random number
> you got from your chinese certified TPM?

I'd suggest approach where TPM RNG result is xored with krng result.

> Remember also that the attack doesn't have to be to the TPM only, it
> could be the pathway by which we get the random number, which involves
> components outside of the TPM certification.

Yeah, I do get this.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Wed, 2019-10-16 at 14:00 +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 14, 2019 at 12:29:57PM -0700, James Bottomley wrote:
> > The job of the in-kernel rng is simply to produce a mixed entropy
> > pool from which we can draw random numbers.  The idea is that quite
> > a few attackers have identified the rng as being a weak point in
> > the security architecture of the kernel, so if we mix entropy from
> > all the sources we have, you have to compromise most of them to
> > gain some predictive power over the rng sequence.
> 
> The documentation says that krng is suitable for key generation.
> Should the documentation changed to state that it is unsuitable?

How do you get that from the argument above?  The krng is about the
best we have in terms of unpredictable key generation, so of course it
is suitable ... provided you give the entropy enough time to have
sufficient entropy.  It's also not foolproof ... Bernstein did a
speculation about how you could compromise all our input sources for
entropy.  However the more sources we have the more difficult the
compromise becomes.

> > The point is not how certified the TPM RNG is, the point is that
> > it's a single source and if we rely on it solely for some
> > applications, like trusted keys, then it gives the attackers a
> > single known point to go after.  This may be impossible for script
> > kiddies, but it won't be for nation states ... are you going to
> > exclusively trust the random number you got from your chinese
> > certified TPM?
> 
> I'd suggest approach where TPM RNG result is xored with krng result.

reversible ciphers are generally frowned upon in random number
generation, that's why the krng uses chacha20.  In general I think we
shouldn't try to code our own mixing and instead should get the krng to
do it for us using whatever the algorithm du jour that the crypto guys
have blessed is.  That's why I proposed adding the TPM output to the
krng as entropy input and then taking the output of the krng.

James

> > Remember also that the attack doesn't have to be to the TPM only,
> > it could be the pathway by which we get the random number, which
> > involves components outside of the TPM certification.
> 
> Yeah, I do get this.
> 
> /Jarkko
> 

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> reversible ciphers are generally frowned upon in random number
> generation, that's why the krng uses chacha20.  In general I think we
> shouldn't try to code our own mixing and instead should get the krng to
> do it for us using whatever the algorithm du jour that the crypto guys
> have blessed is.  That's why I proposed adding the TPM output to the
> krng as entropy input and then taking the output of the krng.

It is already registered as hwrng. What else? Was the issue that
it is only used as seed when the rng is init'd first? I haven't
at this point gone to the internals of krng.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> > reversible ciphers are generally frowned upon in random number
> > generation, that's why the krng uses chacha20.  In general I think
> > we shouldn't try to code our own mixing and instead should get the
> > krng to do it for us using whatever the algorithm du jour that the
> > crypto guys have blessed is.  That's why I proposed adding the TPM
> > output to the krng as entropy input and then taking the output of
> > the krng.
> 
> It is already registered as hwrng. What else?

It only contributes entropy once at start of OS.

>  Was the issue that it is only used as seed when the rng is init'd
> first? I haven't at this point gone to the internals of krng.

Basically it was similar to your xor patch except I got the kernel rng
to do the mixing, so it would use the chacha20 cipher at the moment
until they decide that's unsafe and change it to something else:

https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@HansenPartnership.com/

It uses add_hwgenerator_randomness() to do the mixing.  It also has an
unmixed source so that read of the TPM hwrng device works as expected.

James

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Sumit Garg]

On Thu, 17 Oct 2019 at 00:40, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> > > reversible ciphers are generally frowned upon in random number
> > > generation, that's why the krng uses chacha20.  In general I think
> > > we shouldn't try to code our own mixing and instead should get the
> > > krng to do it for us using whatever the algorithm du jour that the
> > > crypto guys have blessed is.  That's why I proposed adding the TPM
> > > output to the krng as entropy input and then taking the output of
> > > the krng.
> >
> > It is already registered as hwrng. What else?
>
> It only contributes entropy once at start of OS.
>

Why not just configure quality parameter of TPM hwrng as follows? It
would automatically initiate a kthread during hwrng_init() to feed
entropy from TPM to kernel random numbers pool (see:
drivers/char/hw_random/core.c +142).

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 3d6d394..fcc3817 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -548,6 +548,7 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
                 "tpm-rng-%d", chip->dev_num);
        chip->hwrng.name = chip->hwrng_name;
        chip->hwrng.read = tpm_hwrng_read;
+       chip->hwrng.quality = 1024; /* Here we assume TPM provides
full entropy */
        return hwrng_register(&chip->hwrng);

 }

> >  Was the issue that it is only used as seed when the rng is init'd
> > first? I haven't at this point gone to the internals of krng.
>
> Basically it was similar to your xor patch except I got the kernel rng
> to do the mixing, so it would use the chacha20 cipher at the moment
> until they decide that's unsafe and change it to something else:
>
> https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@HansenPartnership.com/
>
> It uses add_hwgenerator_randomness() to do the mixing.  It also has an
> unmixed source so that read of the TPM hwrng device works as expected.

Above suggestion is something similar to yours but utilizing the
framework already provided via hwrng core.

-Sumit

>
> James
>
>
>
>
>

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Thu, 2019-10-17 at 18:22 +0530, Sumit Garg wrote:
> On Thu, 17 Oct 2019 at 00:40, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> > > > reversible ciphers are generally frowned upon in random number
> > > > generation, that's why the krng uses chacha20.  In general I
> > > > think we shouldn't try to code our own mixing and instead
> > > > should get the krng to do it for us using whatever the
> > > > algorithm du jour that the crypto guys have blessed is.  That's
> > > > why I proposed adding the TPM output to the krng as entropy
> > > > input and then taking the output of the krng.
> > > 
> > > It is already registered as hwrng. What else?
> > 
> > It only contributes entropy once at start of OS.
> > 
> 
> Why not just configure quality parameter of TPM hwrng as follows? It
> would automatically initiate a kthread during hwrng_init() to feed
> entropy from TPM to kernel random numbers pool (see:
> drivers/char/hw_random/core.c +142).

The question was asked before by Jerry.  The main reason is we still
can't guarantee that at 1024 the hwrng will choose the TPM as the best
source (the problem being it only chooses one) and the mixing is done
periodically in a timer thread so it's still vulnerable to an entropy
exhaustion attack.  I think it's better to source the random number in
the TPM when asked but mix it with whatever entropy we have in the pool
using the crypto people's mixing algorithm.  This definitely avoids
exhaustion and provides some protection against single source rng
compromises.

James


> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> chip.c
> index 3d6d394..fcc3817 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -548,6 +548,7 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
>                  "tpm-rng-%d", chip->dev_num);
>         chip->hwrng.name = chip->hwrng_name;
>         chip->hwrng.read = tpm_hwrng_read;
> +       chip->hwrng.quality = 1024; /* Here we assume TPM provides
> full entropy */
>         return hwrng_register(&chip->hwrng);
> 
>  }
> 
> > >  Was the issue that it is only used as seed when the rng is
> > > init'd
> > > first? I haven't at this point gone to the internals of krng.
> > 
> > Basically it was similar to your xor patch except I got the kernel
> > rng
> > to do the mixing, so it would use the chacha20 cipher at the moment
> > until they decide that's unsafe and change it to something else:
> > 
> > https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@Hanse
> > nPartnership.com/
> > 
> > It uses add_hwgenerator_randomness() to do the mixing.  It also has
> > an
> > unmixed source so that read of the TPM hwrng device works as
> > expected.
> 
> Above suggestion is something similar to yours but utilizing the
> framework already provided via hwrng core.
> 
> -Sumit
> 
> > 
> > James
> > 
> > 
> > 
> > 
> > 
> 
> 

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Wed, Oct 16, 2019 at 03:10:29PM -0400, James Bottomley wrote:
> On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> > > reversible ciphers are generally frowned upon in random number
> > > generation, that's why the krng uses chacha20.  In general I think
> > > we shouldn't try to code our own mixing and instead should get the
> > > krng to do it for us using whatever the algorithm du jour that the
> > > crypto guys have blessed is.  That's why I proposed adding the TPM
> > > output to the krng as entropy input and then taking the output of
> > > the krng.
> > 
> > It is already registered as hwrng. What else?
> 
> It only contributes entropy once at start of OS.

Ok.

> >  Was the issue that it is only used as seed when the rng is init'd
> > first? I haven't at this point gone to the internals of krng.
> 
> Basically it was similar to your xor patch except I got the kernel rng
> to do the mixing, so it would use the chacha20 cipher at the moment
> until they decide that's unsafe and change it to something else:
> 
> https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@HansenPartnership.com/
> 
> It uses add_hwgenerator_randomness() to do the mixing.  It also has an
> unmixed source so that read of the TPM hwrng device works as expected.

Thinking that could this potentially racy? I.e. between the calls
something else could eat the entropy added?

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 17, 2019 at 09:04:40PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 03:10:29PM -0400, James Bottomley wrote:
> > On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> > > > reversible ciphers are generally frowned upon in random number
> > > > generation, that's why the krng uses chacha20.  In general I think
> > > > we shouldn't try to code our own mixing and instead should get the
> > > > krng to do it for us using whatever the algorithm du jour that the
> > > > crypto guys have blessed is.  That's why I proposed adding the TPM
> > > > output to the krng as entropy input and then taking the output of
> > > > the krng.
> > > 
> > > It is already registered as hwrng. What else?
> > 
> > It only contributes entropy once at start of OS.
> 
> Ok.
> 
> > >  Was the issue that it is only used as seed when the rng is init'd
> > > first? I haven't at this point gone to the internals of krng.
> > 
> > Basically it was similar to your xor patch except I got the kernel rng
> > to do the mixing, so it would use the chacha20 cipher at the moment
> > until they decide that's unsafe and change it to something else:
> > 
> > https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@HansenPartnership.com/
> > 
> > It uses add_hwgenerator_randomness() to do the mixing.  It also has an
> > unmixed source so that read of the TPM hwrng device works as expected.
> 
> Thinking that could this potentially racy? I.e. between the calls
> something else could eat the entropy added?

Also, what is wrong just taking one value from krng and mixing
it with a value from TPM RNG where needed? That would be non-racy
too.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Mon, Oct 21, 2019 at 02:39:39PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 17, 2019 at 09:04:40PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 16, 2019 at 03:10:29PM -0400, James Bottomley wrote:
> > > On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley wrote:
> > > > > reversible ciphers are generally frowned upon in random number
> > > > > generation, that's why the krng uses chacha20.  In general I think
> > > > > we shouldn't try to code our own mixing and instead should get the
> > > > > krng to do it for us using whatever the algorithm du jour that the
> > > > > crypto guys have blessed is.  That's why I proposed adding the TPM
> > > > > output to the krng as entropy input and then taking the output of
> > > > > the krng.
> > > > 
> > > > It is already registered as hwrng. What else?
> > > 
> > > It only contributes entropy once at start of OS.
> > 
> > Ok.
> > 
> > > >  Was the issue that it is only used as seed when the rng is init'd
> > > > first? I haven't at this point gone to the internals of krng.
> > > 
> > > Basically it was similar to your xor patch except I got the kernel rng
> > > to do the mixing, so it would use the chacha20 cipher at the moment
> > > until they decide that's unsafe and change it to something else:
> > > 
> > > https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@HansenPartnership.com/
> > > 
> > > It uses add_hwgenerator_randomness() to do the mixing.  It also has an
> > > unmixed source so that read of the TPM hwrng device works as expected.
> > 
> > Thinking that could this potentially racy? I.e. between the calls
> > something else could eat the entropy added?
> 
> Also, what is wrong just taking one value from krng and mixing
> it with a value from TPM RNG where needed? That would be non-racy
> too.

I guess we can move forward with this?

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[James Bottomley]

On Tue, 2019-10-29 at 10:42 +0200, Jarkko Sakkinen wrote:
> On Mon, Oct 21, 2019 at 02:39:39PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 17, 2019 at 09:04:40PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 16, 2019 at 03:10:29PM -0400, James Bottomley wrote:
> > > > On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > > > > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley
> > > > > wrote:
> > > > > > reversible ciphers are generally frowned upon in random
> > > > > > number
> > > > > > generation, that's why the krng uses chacha20.  In general
> > > > > > I think
> > > > > > we shouldn't try to code our own mixing and instead should
> > > > > > get the
> > > > > > krng to do it for us using whatever the algorithm du jour
> > > > > > that the
> > > > > > crypto guys have blessed is.  That's why I proposed adding
> > > > > > the TPM
> > > > > > output to the krng as entropy input and then taking the
> > > > > > output of
> > > > > > the krng.
> > > > > 
> > > > > It is already registered as hwrng. What else?
> > > > 
> > > > It only contributes entropy once at start of OS.
> > > 
> > > Ok.
> > > 
> > > > >  Was the issue that it is only used as seed when the rng is
> > > > > init'd
> > > > > first? I haven't at this point gone to the internals of krng.
> > > > 
> > > > Basically it was similar to your xor patch except I got the
> > > > kernel rng
> > > > to do the mixing, so it would use the chacha20 cipher at the
> > > > moment
> > > > until they decide that's unsafe and change it to something
> > > > else:
> > > > 
> > > > https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@H
> > > > ansenPartnership.com/
> > > > 
> > > > It uses add_hwgenerator_randomness() to do the mixing.  It also
> > > > has an
> > > > unmixed source so that read of the TPM hwrng device works as
> > > > expected.
> > > 
> > > Thinking that could this potentially racy? I.e. between the calls
> > > something else could eat the entropy added?
> > 
> > Also, what is wrong just taking one value from krng and mixing
> > it with a value from TPM RNG where needed? That would be non-racy
> > too.
> 
> I guess we can move forward with this?

Sure I suppose; can we can figure out how to get the mixing function du
jour exposed?

James

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Tue, Oct 29, 2019 at 07:58:16AM -0700, James Bottomley wrote:
> On Tue, 2019-10-29 at 10:42 +0200, Jarkko Sakkinen wrote:
> > On Mon, Oct 21, 2019 at 02:39:39PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Oct 17, 2019 at 09:04:40PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Oct 16, 2019 at 03:10:29PM -0400, James Bottomley wrote:
> > > > > On Wed, 2019-10-16 at 19:25 +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, Oct 16, 2019 at 08:34:12AM -0400, James Bottomley
> > > > > > wrote:
> > > > > > > reversible ciphers are generally frowned upon in random
> > > > > > > number
> > > > > > > generation, that's why the krng uses chacha20.  In general
> > > > > > > I think
> > > > > > > we shouldn't try to code our own mixing and instead should
> > > > > > > get the
> > > > > > > krng to do it for us using whatever the algorithm du jour
> > > > > > > that the
> > > > > > > crypto guys have blessed is.  That's why I proposed adding
> > > > > > > the TPM
> > > > > > > output to the krng as entropy input and then taking the
> > > > > > > output of
> > > > > > > the krng.
> > > > > > 
> > > > > > It is already registered as hwrng. What else?
> > > > > 
> > > > > It only contributes entropy once at start of OS.
> > > > 
> > > > Ok.
> > > > 
> > > > > >  Was the issue that it is only used as seed when the rng is
> > > > > > init'd
> > > > > > first? I haven't at this point gone to the internals of krng.
> > > > > 
> > > > > Basically it was similar to your xor patch except I got the
> > > > > kernel rng
> > > > > to do the mixing, so it would use the chacha20 cipher at the
> > > > > moment
> > > > > until they decide that's unsafe and change it to something
> > > > > else:
> > > > > 
> > > > > https://lore.kernel.org/linux-crypto/1570227068.17537.4.camel@H
> > > > > ansenPartnership.com/
> > > > > 
> > > > > It uses add_hwgenerator_randomness() to do the mixing.  It also
> > > > > has an
> > > > > unmixed source so that read of the TPM hwrng device works as
> > > > > expected.
> > > > 
> > > > Thinking that could this potentially racy? I.e. between the calls
> > > > something else could eat the entropy added?
> > > 
> > > Also, what is wrong just taking one value from krng and mixing
> > > it with a value from TPM RNG where needed? That would be non-racy
> > > too.
> > 
> > I guess we can move forward with this?
> 
> Sure I suppose; can we can figure out how to get the mixing function du
> jour exposed?

Maybe it is best to reflect the whole issue in the context of the
Sumit's 2nd patch set, which adds ARM TEE support in order to move
forward.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Janne Karhunen]

On Wed, Oct 16, 2019 at 6:35 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:

> > The documentation says that krng is suitable for key generation.
> > Should the documentation changed to state that it is unsuitable?
>
> How do you get that from the argument above?  The krng is about the
> best we have in terms of unpredictable key generation, so of course it
> is suitable ... provided you give the entropy enough time to have
> sufficient entropy.

Yes, so it can be both the safest and the least safe option available.
By default it's the worst one, but use it wisely and it can be the
best source. Hence I was proposing that kconfig option + boot time
printout to make this clear for everyone..


--
Janne

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> That isn't a valid justification for changing the original definition
> of trusted keys.  Just as the kernel supports different methods of
> implementing the same function on different architectures, trusted
> keys will need to support different methods of generating a random
> number.   

This is completely incorrect deduction. The random number generator
inside the kernel is there to gather entropy from different sources.
You would exploit trusted keys to potential weaknesses of a single
entropy source by doing that.

Of course in TEE platform, TEE can be one of the entropy sources but
there is no reason to weaken the security by using it as the only
sources.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Jarkko Sakkinen]

On Thu, Oct 03, 2019 at 09:02:01PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 03, 2019 at 09:02:32AM -0400, Mimi Zohar wrote:
> > That isn't a valid justification for changing the original definition
> > of trusted keys.  Just as the kernel supports different methods of
> > implementing the same function on different architectures, trusted
> > keys will need to support different methods of generating a random
> > number.   
> 
> This is completely incorrect deduction. The random number generator
> inside the kernel is there to gather entropy from different sources.
> You would exploit trusted keys to potential weaknesses of a single
> entropy source by doing that.
> 
> Of course in TEE platform, TEE can be one of the entropy sources but
> there is no reason to weaken the security by using it as the only
> sources.

I.e. where you go wrong is that you are inter mixing requirements
for the payload and for sealing. They are disjoint assets. The rules
for the payload should not be dependent on how you seal your trusted
key.

/Jarkko

Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()

[Janne Karhunen]

On Thu, Oct 3, 2019 at 2:41 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:

> > At what point during boot is the kernel random pool available?  Does
> > this imply that you're planning on changing trusted keys as well?
>
> Well trusted keys *must* be changed to use it. It is not a choice
> because using a proprietary random number generator instead of defacto
> one in the kernel can be categorized as a *regression*.
>
> Also, TEE trusted keys cannot use the TPM option.
>
> If it was not initialized early enough we would need fix that too.

Note that especially IMA and fs encryptions are pretty annoying in
this sense. You probably want to keep your keys device specific and
you really need the keys around the time when the filesystems mount
for the first time. This is very early on..


--
Janne