Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Wed, Oct 16, 2019 at 6:35 PM James Bottomley 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()
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.ca...@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()
On Thu, 17 Oct 2019 at 00:40, 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. > 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(>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.ca...@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()
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 > > random bytes first extracts bytes from the TPM RNG and > > places it into the kernel entropy pool and then asks for > > 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()
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
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
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()
[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()
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
