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: [email protected]
Fixes: 0c36264aa1d5 ("KEYS: asym_tpm: Add loadkey2 and flushspecific [ver #2]")
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
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
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: [email protected]
>Fixes: 0c36264aa1d5 ("KEYS: asym_tpm: Add loadkey2 and flushspecific [ver #2]")
>Signed-off-by: Jarkko Sakkinen <[email protected]>
>---
> 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?
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: [email protected]
> > Fixes: 0c36264aa1d5 ("KEYS: asym_tpm: Add loadkey2 and flushspecific [ver #2]")
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > 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
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
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
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
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
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
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
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
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
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
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
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
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?