2019-10-03 18:57:03

by Jarkko Sakkinen

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

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


2019-10-03 19:11:41

by Mimi Zohar

[permalink] [raw]
Subject: 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

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

> From: Mimi Zohar <[email protected]>
> Sent: Thursday, October 3, 2019 2:54 PM
> To: Jarkko Sakkinen <[email protected]>; 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

2019-10-04 18:29:06

by Jarkko Sakkinen

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

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

2019-10-04 18:31:09

by Jarkko Sakkinen

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

> > 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

2019-10-04 18:45:03

by James Bottomley

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

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

2019-10-04 20:08:41

by Jerry Snitselaar

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

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.

2019-10-04 22:11:47

by James Bottomley

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

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)
{

2019-10-07 10:33:55

by Janne Karhunen

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

On Thu, Oct 3, 2019 at 2:41 PM Jarkko Sakkinen
<[email protected]> 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

2019-10-07 18:10:05

by Mimi Zohar

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

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

2019-10-07 22:14:46

by Ken Goldman

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

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?

2019-10-08 23:54:27

by Jarkko Sakkinen

[permalink] [raw]
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.

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

2019-10-09 07:11:02

by Pascal Van Leeuwen

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

> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of
> Jarkko Sakkinen
> Sent: Wednesday, October 9, 2019 1:54 AM
> To: Ken Goldman <[email protected]>
> Cc: Safford, David (GE Global Research, US) <[email protected]>; Mimi Zohar
> <[email protected]>; [email protected]; [email protected]; open
> list:ASYMMETRIC KEYS <[email protected]>; open list:CRYPTO API <linux-
> [email protected]>; open list <[email protected]>
> 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
http://www.insidesecure.com

2019-10-14 23:45:11

by Jarkko Sakkinen

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

On Wed, Oct 09, 2019 at 12:11:06PM +0000, Safford, David (GE Global Research, US) wrote:
>
> > From: Jarkko Sakkinen <[email protected]>
> > Sent: Tuesday, October 8, 2019 7:54 PM
> > To: Ken Goldman <[email protected]>
> > Cc: Safford, David (GE Global Research, US) <[email protected]>; Mimi
> > Zohar <[email protected]>; [email protected];
> > [email protected]; open list:ASYMMETRIC KEYS
> > <[email protected]>; open list:CRYPTO API <linux-
> > [email protected]>; open list <[email protected]>
> > 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

2019-10-14 23:49:19

by Jarkko Sakkinen

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

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

2019-10-14 23:54:47

by Jarkko Sakkinen

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

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 <[email protected]>
> > > Sent: Tuesday, October 8, 2019 7:54 PM
> > > To: Ken Goldman <[email protected]>
> > > Cc: Safford, David (GE Global Research, US) <[email protected]>; Mimi
> > > Zohar <[email protected]>; [email protected];
> > > [email protected]; open list:ASYMMETRIC KEYS
> > > <[email protected]>; open list:CRYPTO API <linux-
> > > [email protected]>; open list <[email protected]>
> > > 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

2019-10-14 23:56:31

by James Bottomley

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

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 <[email protected]>
> > > Sent: Tuesday, October 8, 2019 7:54 PM
> > > To: Ken Goldman <[email protected]>
> > > Cc: Safford, David (GE Global Research, US) <[email protected]
> > > >; Mimi
> > > Zohar <[email protected]>; [email protected];
> > > [email protected]; open list:ASYMMETRIC KEYS
> > > <[email protected]>; open list:CRYPTO API <linux-
> > > [email protected]>; open list <[email protected]>
> > > 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

2019-10-16 14:48:33

by Jarkko Sakkinen

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

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

2019-10-17 12:28:09

by Jarkko Sakkinen

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

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

2019-10-18 15:54:42

by Sumit Garg

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

On Thu, 17 Oct 2019 at 00:40, James Bottomley
<[email protected]> 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/[email protected]/
>
> 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
>
>
>
>
>

2019-10-18 15:56:14

by James Bottomley

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

On Thu, 2019-10-17 at 18:22 +0530, Sumit Garg wrote:
> On Thu, 17 Oct 2019 at 00:40, James Bottomley
> <[email protected]> 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
> >
> >
> >
> >
> >
>
>

2019-10-18 23:47:55

by Janne Karhunen

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

On Wed, Oct 16, 2019 at 6:35 PM James Bottomley
<[email protected]> 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

2019-10-29 14:59:28

by James Bottomley

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

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