| Message ID | 20221119134259.2969204-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | hwrng: u2fzero - account for high quality RNG | expand |
On Sat, 19 Nov 2022, Jason A. Donenfeld wrote: > The U2F zero apparently has a real TRNG in it with maximum quality, not > one with quality of "1", which was likely a misinterpretation of the > field as a boolean. So remove the assignment entirely, so that we get > the default quality setting. > > In the u2f-zero firmware, the 0x21 RNG command used by this driver is > handled as such [1]: > > case U2F_CUSTOM_GET_RNG: > if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, > NULL, 0, > appdata.tmp, > sizeof(appdata.tmp), &res) == 0 ) > { > memmove(msg->pkt.init.payload, res.buf, 32); > U2FHID_SET_LEN(msg, 32); > usb_write((uint8_t*)msg, 64); > } > else > { > U2FHID_SET_LEN(msg, 0); > usb_write((uint8_t*)msg, 64); > } > > This same call to `atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1, > ATECC_RNG_P2,...)` is then also used in the token's cryptographically > critical "u2f_new_keypair" function, as its rather straightforward > source of random bytes [2]: > > int8_t u2f_new_keypair(uint8_t * handle, uint8_t * appid, uint8_t * pubkey) > { > struct atecc_response res; > uint8_t private_key[36]; > int i; > > watchdog(); > > if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, > NULL, 0, > appdata.tmp, > sizeof(appdata.tmp), &res) != 0 ) > { > return -1; > } > > So it seems rather plain that the ATECC RNG is considered to provide > good random numbers. > > [1] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/custom.c > [2] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/u2f_atecc.c Good catch, thanks Jason. Applied.
On 19/11/2022 14:42, Jason A. Donenfeld wrote: > The U2F zero apparently has a real TRNG in it with maximum quality, not > one with quality of "1", which was likely a misinterpretation of the > field as a boolean. So remove the assignment entirely, so that we get > the default quality setting. > > In the u2f-zero firmware, the 0x21 RNG command used by this driver is > handled as such [1]: > So it seems rather plain that the ATECC RNG is considered to provide > good random numbers. Thanks — at the time when it was written, there was a general concern about whether we should trust the hardware RNG of this device or not, so the safer option was not to :) > Cc: Andrej Shadura <andrew.shadura@collabora.co.uk> > Cc: Jiri Kosina <jkosina@suse.cz> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Probably too late, but still: Acked-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
On Tue, Nov 22, 2022 at 11:22:37AM +0100, Jiri Kosina wrote: > On Sat, 19 Nov 2022, Jason A. Donenfeld wrote: > > > The U2F zero apparently has a real TRNG in it with maximum quality, not > > one with quality of "1", which was likely a misinterpretation of the > > field as a boolean. So remove the assignment entirely, so that we get > > the default quality setting. > > > > In the u2f-zero firmware, the 0x21 RNG command used by this driver is > > handled as such [1]: > > > > case U2F_CUSTOM_GET_RNG: > > if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, > > NULL, 0, > > appdata.tmp, > > sizeof(appdata.tmp), &res) == 0 ) > > { > > memmove(msg->pkt.init.payload, res.buf, 32); > > U2FHID_SET_LEN(msg, 32); > > usb_write((uint8_t*)msg, 64); > > } > > else > > { > > U2FHID_SET_LEN(msg, 0); > > usb_write((uint8_t*)msg, 64); > > } > > > > This same call to `atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1, > > ATECC_RNG_P2,...)` is then also used in the token's cryptographically > > critical "u2f_new_keypair" function, as its rather straightforward > > source of random bytes [2]: > > > > int8_t u2f_new_keypair(uint8_t * handle, uint8_t * appid, uint8_t * pubkey) > > { > > struct atecc_response res; > > uint8_t private_key[36]; > > int i; > > > > watchdog(); > > > > if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, > > NULL, 0, > > appdata.tmp, > > sizeof(appdata.tmp), &res) != 0 ) > > { > > return -1; > > } > > > > So it seems rather plain that the ATECC RNG is considered to provide > > good random numbers. > > > > [1] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/custom.c > > [2] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/u2f_atecc.c > > Good catch, thanks Jason. Applied. This should probably go via Herbert's tree, because it depends on some changed handling for the zero quality field. Jason
On Tue, 22 Nov 2022, Jason A. Donenfeld wrote: > > This should probably go via Herbert's tree, because it depends on some > > changed handling for the zero quality field. OK, I will drop it. At least Herbert can include Andrej's ack :)
On Sat, Nov 19, 2022 at 02:42:59PM +0100, Jason A. Donenfeld wrote: > The U2F zero apparently has a real TRNG in it with maximum quality, not > one with quality of "1", which was likely a misinterpretation of the > field as a boolean. So remove the assignment entirely, so that we get > the default quality setting. > > In the u2f-zero firmware, the 0x21 RNG command used by this driver is > handled as such [1]: > > case U2F_CUSTOM_GET_RNG: > if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, > NULL, 0, > appdata.tmp, > sizeof(appdata.tmp), &res) == 0 ) > { > memmove(msg->pkt.init.payload, res.buf, 32); > U2FHID_SET_LEN(msg, 32); > usb_write((uint8_t*)msg, 64); > } > else > { > U2FHID_SET_LEN(msg, 0); > usb_write((uint8_t*)msg, 64); > } > > This same call to `atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1, > ATECC_RNG_P2,...)` is then also used in the token's cryptographically > critical "u2f_new_keypair" function, as its rather straightforward > source of random bytes [2]: > > int8_t u2f_new_keypair(uint8_t * handle, uint8_t * appid, uint8_t * pubkey) > { > struct atecc_response res; > uint8_t private_key[36]; > int i; > > watchdog(); > > if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, > NULL, 0, > appdata.tmp, > sizeof(appdata.tmp), &res) != 0 ) > { > return -1; > } > > So it seems rather plain that the ATECC RNG is considered to provide > good random numbers. > > [1] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/custom.c > [2] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/u2f_atecc.c > > Cc: Andrej Shadura <andrew.shadura@collabora.co.uk> > Cc: Jiri Kosina <jkosina@suse.cz> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/hid/hid-u2fzero.c | 1 - > 1 file changed, 1 deletion(-) Patch applied. Thanks.
diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c index ad489caf53ad..744a91e6e78c 100644 --- a/drivers/hid/hid-u2fzero.c +++ b/drivers/hid/hid-u2fzero.c @@ -261,7 +261,6 @@ static int u2fzero_init_hwrng(struct u2fzero_device *dev, dev->hwrng.name = dev->rng_name; dev->hwrng.read = u2fzero_rng_read; - dev->hwrng.quality = 1; return devm_hwrng_register(&dev->hdev->dev, &dev->hwrng); }
The U2F zero apparently has a real TRNG in it with maximum quality, not one with quality of "1", which was likely a misinterpretation of the field as a boolean. So remove the assignment entirely, so that we get the default quality setting. In the u2f-zero firmware, the 0x21 RNG command used by this driver is handled as such [1]: case U2F_CUSTOM_GET_RNG: if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, NULL, 0, appdata.tmp, sizeof(appdata.tmp), &res) == 0 ) { memmove(msg->pkt.init.payload, res.buf, 32); U2FHID_SET_LEN(msg, 32); usb_write((uint8_t*)msg, 64); } else { U2FHID_SET_LEN(msg, 0); usb_write((uint8_t*)msg, 64); } This same call to `atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1, ATECC_RNG_P2,...)` is then also used in the token's cryptographically critical "u2f_new_keypair" function, as its rather straightforward source of random bytes [2]: int8_t u2f_new_keypair(uint8_t * handle, uint8_t * appid, uint8_t * pubkey) { struct atecc_response res; uint8_t private_key[36]; int i; watchdog(); if (atecc_send_recv(ATECC_CMD_RNG,ATECC_RNG_P1,ATECC_RNG_P2, NULL, 0, appdata.tmp, sizeof(appdata.tmp), &res) != 0 ) { return -1; } So it seems rather plain that the ATECC RNG is considered to provide good random numbers. [1] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/custom.c [2] https://github.com/conorpp/u2f-zero/blob/master/firmware/src/u2f_atecc.c Cc: Andrej Shadura <andrew.shadura@collabora.co.uk> Cc: Jiri Kosina <jkosina@suse.cz> Cc: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/hid/hid-u2fzero.c | 1 - 1 file changed, 1 deletion(-)