* [PATCH] verify: search keyid in hashed signature subpackets @ 2016-03-29 19:02 Ignat Korchagin 2016-03-30 4:44 ` Andrei Borzenkov 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-03-29 19:02 UTC (permalink / raw) To: grub-devel Currently GRUB2 verify logic searches PGP keyid only in unhashed subpackets of PGP signature packet. As a result, signatures generated with GoLang openpgp package (https://godoc.org/golang.org/x/crypto/openpgp) could not be verified, because this package puts keyid in hashed subpackets and GRUB code never initializes the keyid variable, therefore is not able to find "verification key" with id 0x0. diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c index 166d0aa..dde37c4 100644 --- a/grub-core/commands/verify.c +++ b/grub-core/commands/verify.c @@ -532,33 +532,15 @@ hash->write (context, &v, sizeof (v)); hash->write (context, &v4, sizeof (v4)); - while (rem) - { - r = grub_file_read (sig, readbuf, - rem < READBUF_SIZE ? rem : READBUF_SIZE); - if (r < 0) - goto fail; - if (r == 0) - break; - hash->write (context, readbuf, r); - rem -= r; - } - hash->write (context, &v, sizeof (v)); - s = 0xff; - hash->write (context, &s, sizeof (s)); - hash->write (context, &headlen, sizeof (headlen)); - r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); - if (r != sizeof (unhashed_sub)) + if (rem > READBUF_SIZE) + goto fail; + r = grub_file_read (sig, readbuf, rem); + if (r != rem) goto fail; { grub_uint8_t *ptr; grub_uint32_t l; - rem = grub_be_to_cpu16 (unhashed_sub); - if (rem > READBUF_SIZE) - goto fail; - r = grub_file_read (sig, readbuf, rem); - if (r != rem) - goto fail; + for (ptr = readbuf; ptr < readbuf + rem; ptr += l) { if (*ptr < 192) @@ -581,6 +563,46 @@ keyid = grub_get_unaligned64 (ptr + 1); } } + hash->write (context, readbuf, r); + hash->write (context, &v, sizeof (v)); + s = 0xff; + hash->write (context, &s, sizeof (s)); + hash->write (context, &headlen, sizeof (headlen)); + r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); + if (r != sizeof (unhashed_sub)) + goto fail; + if (keyid == 0) + { + grub_uint8_t *ptr; + grub_uint32_t l; + rem = grub_be_to_cpu16 (unhashed_sub); + if (rem > READBUF_SIZE) + goto fail; + r = grub_file_read (sig, readbuf, rem); + if (r != rem) + goto fail; + for (ptr = readbuf; ptr < readbuf + rem; ptr += l) + { + if (*ptr < 192) + l = *ptr++; + else if (*ptr < 255) + { + if (ptr + 1 >= readbuf + rem) + break; + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; + ptr += 2; + } + else + { + if (ptr + 5 >= readbuf + rem) + break; + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); + ptr += 5; + } + if (*ptr == 0x10 && l >= 8) + keyid = grub_get_unaligned64 (ptr + 1); + } + } hash->final (context); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-03-29 19:02 [PATCH] verify: search keyid in hashed signature subpackets Ignat Korchagin @ 2016-03-30 4:44 ` Andrei Borzenkov 2016-03-30 8:47 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Andrei Borzenkov @ 2016-03-30 4:44 UTC (permalink / raw) To: The development of GNU GRUB 29.03.2016 22:02, Ignat Korchagin пишет: > Currently GRUB2 verify logic searches PGP keyid only in unhashed subpackets of PGP signature packet. As a result, signatures generated with GoLang openpgp package (https://godoc.org/golang.org/x/crypto/openpgp) could not be verified, because this package puts keyid in hashed subpackets and GRUB code never initializes the keyid variable, therefore is not able to find "verification key" with id 0x0. > > diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c > index 166d0aa..dde37c4 100644 > --- a/grub-core/commands/verify.c > +++ b/grub-core/commands/verify.c > @@ -532,33 +532,15 @@ > > hash->write (context, &v, sizeof (v)); > hash->write (context, &v4, sizeof (v4)); > - while (rem) > - { > - r = grub_file_read (sig, readbuf, > - rem < READBUF_SIZE ? rem : READBUF_SIZE); > - if (r < 0) > - goto fail; > - if (r == 0) > - break; > - hash->write (context, readbuf, r); > - rem -= r; > - } > - hash->write (context, &v, sizeof (v)); > - s = 0xff; > - hash->write (context, &s, sizeof (s)); > - hash->write (context, &headlen, sizeof (headlen)); > - r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); > - if (r != sizeof (unhashed_sub)) > + if (rem > READBUF_SIZE) > + goto fail; This changes behavior. It accepted hashed subpackets of arbitrary length before. If this was not appropriate, please explain why. > + r = grub_file_read (sig, readbuf, rem); > + if (r != rem) > goto fail; > { > grub_uint8_t *ptr; > grub_uint32_t l; > - rem = grub_be_to_cpu16 (unhashed_sub); > - if (rem > READBUF_SIZE) > - goto fail; > - r = grub_file_read (sig, readbuf, rem); > - if (r != rem) > - goto fail; > + > for (ptr = readbuf; ptr < readbuf + rem; ptr += l) > { > if (*ptr < 192) > @@ -581,6 +563,46 @@ > keyid = grub_get_unaligned64 (ptr + 1); > } > } > + hash->write (context, readbuf, r); > + hash->write (context, &v, sizeof (v)); > + s = 0xff; > + hash->write (context, &s, sizeof (s)); > + hash->write (context, &headlen, sizeof (headlen)); > + r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); > + if (r != sizeof (unhashed_sub)) > + goto fail; > + if (keyid == 0) > + { > + grub_uint8_t *ptr; > + grub_uint32_t l; > + rem = grub_be_to_cpu16 (unhashed_sub); > + if (rem > READBUF_SIZE) > + goto fail; > + r = grub_file_read (sig, readbuf, rem); > + if (r != rem) > + goto fail; > + for (ptr = readbuf; ptr < readbuf + rem; ptr += l) > + { > + if (*ptr < 192) > + l = *ptr++; > + else if (*ptr < 255) > + { > + if (ptr + 1 >= readbuf + rem) > + break; > + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; > + ptr += 2; > + } > + else > + { > + if (ptr + 5 >= readbuf + rem) > + break; > + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); > + ptr += 5; > + } > + if (*ptr == 0x10 && l >= 8) > + keyid = grub_get_unaligned64 (ptr + 1); > + } > + } > > hash->final (context); > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-03-30 4:44 ` Andrei Borzenkov @ 2016-03-30 8:47 ` Ignat Korchagin 2016-03-30 9:38 ` Andrei Borzenkov 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-03-30 8:47 UTC (permalink / raw) To: The development of GNU GRUB Well the code was copied from handling unhashed subpackets and has same assumptions. I do agree that it does not handle arbitrary length data. But if you consider it wrong, it should be changed for both hashed and unhashed packets. Currently, for example, if the length of unhashed subpackets will be longer than READBUF_SIZE, the signature check will fail as well. On Wed, Mar 30, 2016 at 5:44 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote: > > 29.03.2016 22:02, Ignat Korchagin пишет: > > Currently GRUB2 verify logic searches PGP keyid only in unhashed subpackets of PGP signature packet. As a result, signatures generated with GoLang openpgp package (https://godoc.org/golang.org/x/crypto/openpgp) could not be verified, because this package puts keyid in hashed subpackets and GRUB code never initializes the keyid variable, therefore is not able to find "verification key" with id 0x0. > > > > diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c > > index 166d0aa..dde37c4 100644 > > --- a/grub-core/commands/verify.c > > +++ b/grub-core/commands/verify.c > > @@ -532,33 +532,15 @@ > > > > hash->write (context, &v, sizeof (v)); > > hash->write (context, &v4, sizeof (v4)); > > - while (rem) > > - { > > - r = grub_file_read (sig, readbuf, > > - rem < READBUF_SIZE ? rem : READBUF_SIZE); > > - if (r < 0) > > - goto fail; > > - if (r == 0) > > - break; > > - hash->write (context, readbuf, r); > > - rem -= r; > > - } > > - hash->write (context, &v, sizeof (v)); > > - s = 0xff; > > - hash->write (context, &s, sizeof (s)); > > - hash->write (context, &headlen, sizeof (headlen)); > > - r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); > > - if (r != sizeof (unhashed_sub)) > > + if (rem > READBUF_SIZE) > > + goto fail; > > This changes behavior. It accepted hashed subpackets of arbitrary length > before. If this was not appropriate, please explain why. > > > + r = grub_file_read (sig, readbuf, rem); > > + if (r != rem) > > goto fail; > > { > > grub_uint8_t *ptr; > > grub_uint32_t l; > > - rem = grub_be_to_cpu16 (unhashed_sub); > > - if (rem > READBUF_SIZE) > > - goto fail; > > - r = grub_file_read (sig, readbuf, rem); > > - if (r != rem) > > - goto fail; > > + > > for (ptr = readbuf; ptr < readbuf + rem; ptr += l) > > { > > if (*ptr < 192) > > @@ -581,6 +563,46 @@ > > keyid = grub_get_unaligned64 (ptr + 1); > > } > > } > > + hash->write (context, readbuf, r); > > + hash->write (context, &v, sizeof (v)); > > + s = 0xff; > > + hash->write (context, &s, sizeof (s)); > > + hash->write (context, &headlen, sizeof (headlen)); > > + r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); > > + if (r != sizeof (unhashed_sub)) > > + goto fail; > > + if (keyid == 0) > > + { > > + grub_uint8_t *ptr; > > + grub_uint32_t l; > > + rem = grub_be_to_cpu16 (unhashed_sub); > > + if (rem > READBUF_SIZE) > > + goto fail; > > + r = grub_file_read (sig, readbuf, rem); > > + if (r != rem) > > + goto fail; > > + for (ptr = readbuf; ptr < readbuf + rem; ptr += l) > > + { > > + if (*ptr < 192) > > + l = *ptr++; > > + else if (*ptr < 255) > > + { > > + if (ptr + 1 >= readbuf + rem) > > + break; > > + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; > > + ptr += 2; > > + } > > + else > > + { > > + if (ptr + 5 >= readbuf + rem) > > + break; > > + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); > > + ptr += 5; > > + } > > + if (*ptr == 0x10 && l >= 8) > > + keyid = grub_get_unaligned64 (ptr + 1); > > + } > > + } > > > > hash->final (context); > > > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-03-30 8:47 ` Ignat Korchagin @ 2016-03-30 9:38 ` Andrei Borzenkov 2016-03-30 14:09 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Andrei Borzenkov @ 2016-03-30 9:38 UTC (permalink / raw) To: The development of GNU GRUB On Wed, Mar 30, 2016 at 11:47 AM, Ignat Korchagin <ignat@cloudflare.com> wrote: > Well the code was copied from handling unhashed subpackets and has > same assumptions. I do agree that it does not handle arbitrary length > data. But if you consider it wrong, it should be changed for both > hashed and unhashed packets. Currently, for example, if the length of > unhashed subpackets will be longer than READBUF_SIZE, the signature > check will fail as well. > Yes, looking at RFC 4880, unhashed subpackets can exceed 4K. Still it is no excuse for deliberately breaking what was correct. Given that max size is just 64K, may be increasing READBUF_SIZE is the simplest solution for now, as long as this is the only packet format version supported anyway. It is of course possible to implement running detection of keyid, but I'm not sure whether this is worse it. Please also factor out keyid check in separate function too to avoid code duplication. Could you also add regression test with signature that did not work for you? > On Wed, Mar 30, 2016 at 5:44 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote: >> >> 29.03.2016 22:02, Ignat Korchagin пишет: >> > Currently GRUB2 verify logic searches PGP keyid only in unhashed subpackets of PGP signature packet. As a result, signatures generated with GoLang openpgp package (https://godoc.org/golang.org/x/crypto/openpgp) could not be verified, because this package puts keyid in hashed subpackets and GRUB code never initializes the keyid variable, therefore is not able to find "verification key" with id 0x0. >> > >> > diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c >> > index 166d0aa..dde37c4 100644 >> > --- a/grub-core/commands/verify.c >> > +++ b/grub-core/commands/verify.c >> > @@ -532,33 +532,15 @@ >> > >> > hash->write (context, &v, sizeof (v)); >> > hash->write (context, &v4, sizeof (v4)); >> > - while (rem) >> > - { >> > - r = grub_file_read (sig, readbuf, >> > - rem < READBUF_SIZE ? rem : READBUF_SIZE); >> > - if (r < 0) >> > - goto fail; >> > - if (r == 0) >> > - break; >> > - hash->write (context, readbuf, r); >> > - rem -= r; >> > - } >> > - hash->write (context, &v, sizeof (v)); >> > - s = 0xff; >> > - hash->write (context, &s, sizeof (s)); >> > - hash->write (context, &headlen, sizeof (headlen)); >> > - r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); >> > - if (r != sizeof (unhashed_sub)) >> > + if (rem > READBUF_SIZE) >> > + goto fail; >> >> This changes behavior. It accepted hashed subpackets of arbitrary length >> before. If this was not appropriate, please explain why. >> >> > + r = grub_file_read (sig, readbuf, rem); >> > + if (r != rem) >> > goto fail; >> > { >> > grub_uint8_t *ptr; >> > grub_uint32_t l; >> > - rem = grub_be_to_cpu16 (unhashed_sub); >> > - if (rem > READBUF_SIZE) >> > - goto fail; >> > - r = grub_file_read (sig, readbuf, rem); >> > - if (r != rem) >> > - goto fail; >> > + >> > for (ptr = readbuf; ptr < readbuf + rem; ptr += l) >> > { >> > if (*ptr < 192) >> > @@ -581,6 +563,46 @@ >> > keyid = grub_get_unaligned64 (ptr + 1); >> > } >> > } >> > + hash->write (context, readbuf, r); >> > + hash->write (context, &v, sizeof (v)); >> > + s = 0xff; >> > + hash->write (context, &s, sizeof (s)); >> > + hash->write (context, &headlen, sizeof (headlen)); >> > + r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); >> > + if (r != sizeof (unhashed_sub)) >> > + goto fail; >> > + if (keyid == 0) >> > + { >> > + grub_uint8_t *ptr; >> > + grub_uint32_t l; >> > + rem = grub_be_to_cpu16 (unhashed_sub); >> > + if (rem > READBUF_SIZE) >> > + goto fail; >> > + r = grub_file_read (sig, readbuf, rem); >> > + if (r != rem) >> > + goto fail; >> > + for (ptr = readbuf; ptr < readbuf + rem; ptr += l) >> > + { >> > + if (*ptr < 192) >> > + l = *ptr++; >> > + else if (*ptr < 255) >> > + { >> > + if (ptr + 1 >= readbuf + rem) >> > + break; >> > + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; >> > + ptr += 2; >> > + } >> > + else >> > + { >> > + if (ptr + 5 >= readbuf + rem) >> > + break; >> > + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); >> > + ptr += 5; >> > + } >> > + if (*ptr == 0x10 && l >= 8) >> > + keyid = grub_get_unaligned64 (ptr + 1); >> > + } >> > + } >> > >> > hash->final (context); >> > >> > >> > >> > _______________________________________________ >> > Grub-devel mailing list >> > Grub-devel@gnu.org >> > https://lists.gnu.org/mailman/listinfo/grub-devel >> > >> >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-03-30 9:38 ` Andrei Borzenkov @ 2016-03-30 14:09 ` Ignat Korchagin 2016-04-09 4:27 ` Andrei Borzenkov 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-03-30 14:09 UTC (permalink / raw) To: The development of GNU GRUB Implemented as a separate function which should process arbitrary length data. As for tests, it seems that the easiest way is to add this signature to tests/file_filter. Not sure how should I send you the patch with binary data though. diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c index 166d0aa..cc8fa39 100644 --- a/grub-core/commands/verify.c +++ b/grub-core/commands/verify.c @@ -445,6 +445,88 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval, return ret; } +static grub_uint64_t +grub_subpacket_keyid_search (grub_file_t f, grub_ssize_t sub_len, + const gcry_md_spec_t *hash, void *context) +{ + grub_uint64_t keyid = 0; + grub_uint8_t *readbuf = NULL; + + while (sub_len > 0) + { + grub_uint8_t szid[5]; + grub_size_t sz_len; + grub_size_t l; + + grub_ssize_t r = grub_file_read (f, szid, 1); + if (r != 1) + return 0; + + if (szid[0] < 192) + { + l = szid[0]; + sz_len = 1; + } + else if (szid[0] < 255) + { + r = grub_file_read (f, szid + 1, 1); + if (r != 1) + return 0; + + l = (((szid[0] & ~192) << GRUB_CHAR_BIT) | szid[1]) + 192; + sz_len = 2; + } + else + { + r = grub_file_read (f, szid + 1, 4); + if (r != 4) + return 0; + + l = grub_be_to_cpu32 (grub_get_unaligned32 (szid + 1)); + sz_len = 5; + } + + readbuf = grub_zalloc (l); + if (!readbuf) + return 0; + + r = grub_file_read (f, readbuf, l); + if (r <= 0) + goto fail; + + while ((grub_size_t)r < l) + { + grub_ssize_t rr = grub_file_read (f, readbuf + r, l - (grub_size_t)r); + if (rr <= 0) + goto fail; + r += rr; + } + + if (*readbuf == 0x10 && l >= 8) + keyid = grub_get_unaligned64 (readbuf + 1); + + if (hash && context) + { + hash->write (context, szid, sz_len); + hash->write (context, readbuf, l); + } + + grub_free (readbuf); + readbuf = NULL; + + sub_len -= sz_len + l; + } + +fail: + if (readbuf) + { + grub_free (readbuf); + return 0; + } + + return keyid; +} + static grub_err_t grub_verify_signature_real (char *buf, grub_size_t size, grub_file_t f, grub_file_t sig, @@ -532,17 +614,7 @@ grub_verify_signature_real (char *buf, grub_size_t size, hash->write (context, &v, sizeof (v)); hash->write (context, &v4, sizeof (v4)); - while (rem) - { - r = grub_file_read (sig, readbuf, - rem < READBUF_SIZE ? rem : READBUF_SIZE); - if (r < 0) - goto fail; - if (r == 0) - break; - hash->write (context, readbuf, r); - rem -= r; - } + keyid = grub_subpacket_keyid_search (sig, rem, hash, context); hash->write (context, &v, sizeof (v)); s = 0xff; hash->write (context, &s, sizeof (s)); @@ -550,37 +622,11 @@ grub_verify_signature_real (char *buf, grub_size_t size, r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); if (r != sizeof (unhashed_sub)) goto fail; - { - grub_uint8_t *ptr; - grub_uint32_t l; - rem = grub_be_to_cpu16 (unhashed_sub); - if (rem > READBUF_SIZE) - goto fail; - r = grub_file_read (sig, readbuf, rem); - if (r != rem) - goto fail; - for (ptr = readbuf; ptr < readbuf + rem; ptr += l) - { - if (*ptr < 192) - l = *ptr++; - else if (*ptr < 255) - { - if (ptr + 1 >= readbuf + rem) - break; - l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; - ptr += 2; - } - else - { - if (ptr + 5 >= readbuf + rem) - break; - l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); - ptr += 5; - } - if (*ptr == 0x10 && l >= 8) - keyid = grub_get_unaligned64 (ptr + 1); - } - } + rem = grub_be_to_cpu16 (unhashed_sub); + if (keyid == 0) + keyid = grub_subpacket_keyid_search (sig, rem, NULL, NULL); + else + grub_subpacket_keyid_search (sig, rem, NULL, NULL); hash->final (context); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-03-30 14:09 ` Ignat Korchagin @ 2016-04-09 4:27 ` Andrei Borzenkov 2016-04-10 18:34 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Andrei Borzenkov @ 2016-04-09 4:27 UTC (permalink / raw) To: grub-devel 30.03.2016 17:09, Ignat Korchagin пишет: > Implemented as a separate function which should process arbitrary length data. TBH I still think that simply setting READBUF_SIZE to 64K is the simplest solution. > As for tests, it seems that the easiest way is to add this signature to tests/file_filter. Not sure how should I send you the patch with binary data though. > Just sign files and send new signatures and keys, I will commit them. > diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c > index 166d0aa..cc8fa39 100644 > --- a/grub-core/commands/verify.c > +++ b/grub-core/commands/verify.c > @@ -445,6 +445,88 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval, > return ret; > } > > +static grub_uint64_t > +grub_subpacket_keyid_search (grub_file_t f, grub_ssize_t sub_len, > + const gcry_md_spec_t *hash, void *context) This does more than just searching for keyid, it also hashes content, so name is misleading. > +{ > + grub_uint64_t keyid = 0; > + grub_uint8_t *readbuf = NULL; > + > + while (sub_len > 0) > + { > + grub_uint8_t szid[5]; > + grub_size_t sz_len; > + grub_size_t l; > + > + grub_ssize_t r = grub_file_read (f, szid, 1); > + if (r != 1) > + return 0; > + > + if (szid[0] < 192) > + { > + l = szid[0]; > + sz_len = 1; > + } > + else if (szid[0] < 255) > + { > + r = grub_file_read (f, szid + 1, 1); > + if (r != 1) > + return 0; > + > + l = (((szid[0] & ~192) << GRUB_CHAR_BIT) | szid[1]) + 192; > + sz_len = 2; > + } > + else > + { > + r = grub_file_read (f, szid + 1, 4); > + if (r != 4) > + return 0; > + > + l = grub_be_to_cpu32 (grub_get_unaligned32 (szid + 1)); > + sz_len = 5; > + } > + > + readbuf = grub_zalloc (l); So you allocate full subpacket length anyway. Why not set READBUF_SIZE to max size then from the very start? > + if (!readbuf) > + return 0; > + > + r = grub_file_read (f, readbuf, l); > + if (r <= 0) > + goto fail; > + > + while ((grub_size_t)r < l) > + { > + grub_ssize_t rr = grub_file_read (f, readbuf + r, l - (grub_size_t)r); > + if (rr <= 0) > + goto fail; > + r += rr; > + } > + > + if (*readbuf == 0x10 && l >= 8) > + keyid = grub_get_unaligned64 (readbuf + 1); > + > + if (hash && context) > + { > + hash->write (context, szid, sz_len); > + hash->write (context, readbuf, l); > + } > + > + grub_free (readbuf); > + readbuf = NULL; > + > + sub_len -= sz_len + l; > + } > + > +fail: > + if (readbuf) > + { > + grub_free (readbuf); > + return 0; > + } > + > + return keyid; > +} > + > static grub_err_t > grub_verify_signature_real (char *buf, grub_size_t size, > grub_file_t f, grub_file_t sig, > @@ -532,17 +614,7 @@ grub_verify_signature_real (char *buf, grub_size_t size, > > hash->write (context, &v, sizeof (v)); > hash->write (context, &v4, sizeof (v4)); > - while (rem) > - { > - r = grub_file_read (sig, readbuf, > - rem < READBUF_SIZE ? rem : READBUF_SIZE); > - if (r < 0) > - goto fail; > - if (r == 0) > - break; > - hash->write (context, readbuf, r); > - rem -= r; > - } > + keyid = grub_subpacket_keyid_search (sig, rem, hash, context); > hash->write (context, &v, sizeof (v)); > s = 0xff; > hash->write (context, &s, sizeof (s)); > @@ -550,37 +622,11 @@ grub_verify_signature_real (char *buf, grub_size_t size, > r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); > if (r != sizeof (unhashed_sub)) > goto fail; > - { > - grub_uint8_t *ptr; > - grub_uint32_t l; > - rem = grub_be_to_cpu16 (unhashed_sub); > - if (rem > READBUF_SIZE) > - goto fail; > - r = grub_file_read (sig, readbuf, rem); > - if (r != rem) > - goto fail; > - for (ptr = readbuf; ptr < readbuf + rem; ptr += l) > - { > - if (*ptr < 192) > - l = *ptr++; > - else if (*ptr < 255) > - { > - if (ptr + 1 >= readbuf + rem) > - break; > - l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; > - ptr += 2; > - } > - else > - { > - if (ptr + 5 >= readbuf + rem) > - break; > - l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); > - ptr += 5; > - } > - if (*ptr == 0x10 && l >= 8) > - keyid = grub_get_unaligned64 (ptr + 1); > - } > - } > + rem = grub_be_to_cpu16 (unhashed_sub); > + if (keyid == 0) > + keyid = grub_subpacket_keyid_search (sig, rem, NULL, NULL); > + else > + grub_subpacket_keyid_search (sig, rem, NULL, NULL); > > hash->final (context); > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-04-09 4:27 ` Andrei Borzenkov @ 2016-04-10 18:34 ` Ignat Korchagin 2016-04-19 14:28 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-04-10 18:34 UTC (permalink / raw) To: The development of GNU GRUB > TBH I still think that simply setting READBUF_SIZE to 64K is the simplest solution. I would agree, but I was just a little concerned about allocating large buffer. I'm not sure whether GRUB is considered to be able to run on very resource constrained environments and it seemed that per-subpacket allocation is better. But if you are sure it is OK for 64K buffer I can rewrite the code to allocate it once before using it. > Just sign files and send new signatures and keys, I will commit them. Will send with updated patch. > This does more than just searching for keyid, it also hashes content, so name is misleading. Any suggestions? > So you allocate full subpacket length anyway. Why not set READBUF_SIZE to max size then from the very start? See the comment about large buffer. This does per-subpacket allocation, which should be less then overall subpacket length. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-04-10 18:34 ` Ignat Korchagin @ 2016-04-19 14:28 ` Ignat Korchagin 2016-04-21 16:54 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-04-19 14:28 UTC (permalink / raw) To: The development of GNU GRUB Was preparing test data for the above patch. I wanted to reuse files and keys in tests/file_filter. There are two files: keys and keys.pub. I assumed first one is private key, while the other one is public. However, it seems that keys is public as well. Is the test private key for this public key available? On Sun, Apr 10, 2016 at 7:34 PM, Ignat Korchagin <ignat@cloudflare.com> wrote: >> TBH I still think that simply setting READBUF_SIZE to 64K is the simplest solution. > > I would agree, but I was just a little concerned about allocating > large buffer. I'm not sure whether GRUB is considered to be able to > run on very resource constrained environments and it seemed that > per-subpacket allocation is better. But if you are sure it is OK for > 64K buffer I can rewrite the code to allocate it once before using it. > >> Just sign files and send new signatures and keys, I will commit them. > > Will send with updated patch. > >> This does more than just searching for keyid, it also hashes content, so name is misleading. > > Any suggestions? > >> So you allocate full subpacket length anyway. Why not set READBUF_SIZE to max size then from the very start? > > See the comment about large buffer. This does per-subpacket > allocation, which should be less then overall subpacket length. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-04-19 14:28 ` Ignat Korchagin @ 2016-04-21 16:54 ` Ignat Korchagin 2016-04-28 21:32 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-04-21 16:54 UTC (permalink / raw) To: The development of GNU GRUB Best of both worlds: I left the READBUF_SIZE as is for the rest of the code, but rather reallocate the buffer with appropriate length for subpackets specifically. Hashing is outside of the keyid search function, so the name is good this time. Still did not get your response about PGP private test key. If it is not available, I can regenerate it and resign all test files for future use. diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c index 4268eaa..91e1da6 100644 --- a/grub-core/commands/verify.c +++ b/grub-core/commands/verify.c @@ -445,6 +445,38 @@ return ret; } +static grub_uint64_t +grub_subpacket_keyid_search (const grub_uint8_t * sub, grub_ssize_t sub_len) +{ + const grub_uint8_t *ptr; + grub_uint32_t l; + grub_uint64_t keyid = 0; + + for (ptr = sub; ptr < sub + sub_len; ptr += l) + { + if (*ptr < 192) + l = *ptr++; + else if (*ptr < 255) + { + if (ptr + 1 >= sub + sub_len) + break; + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; + ptr += 2; + } + else + { + if (ptr + 5 >= sub + sub_len) + break; + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); + ptr += 5; + } + if (*ptr == 0x10 && l >= 8) + keyid = grub_get_unaligned64 (ptr + 1); + } + + return keyid; +} + static grub_err_t grub_verify_signature_real (char *buf, grub_size_t size, grub_file_t f, grub_file_t sig, @@ -529,20 +561,31 @@ break; hash->write (context, readbuf, r); } + grub_free (readbuf); + + readbuf = grub_malloc (rem); + if (!readbuf) + goto fail; hash->write (context, &v, sizeof (v)); hash->write (context, &v4, sizeof (v4)); - while (rem) + + r = 0; + while (r < rem) { - r = grub_file_read (sig, readbuf, - rem < READBUF_SIZE ? rem : READBUF_SIZE); - if (r < 0) + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r); + if (rr < 0) goto fail; - if (r == 0) + if (rr == 0) break; - hash->write (context, readbuf, r); - rem -= r; + r += rr; } + if (r != rem) + goto fail; + hash->write (context, readbuf, rem); + keyid = grub_subpacket_keyid_search (readbuf, rem); + grub_free (readbuf); + hash->write (context, &v, sizeof (v)); s = 0xff; hash->write (context, &s, sizeof (s)); @@ -550,40 +593,34 @@ r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); if (r != sizeof (unhashed_sub)) goto fail; - { - grub_uint8_t *ptr; - grub_uint32_t l; - rem = grub_be_to_cpu16 (unhashed_sub); - if (rem > READBUF_SIZE) - goto fail; - r = grub_file_read (sig, readbuf, rem); - if (r != rem) - goto fail; - for (ptr = readbuf; ptr < readbuf + rem; ptr += l) - { - if (*ptr < 192) - l = *ptr++; - else if (*ptr < 255) - { - if (ptr + 1 >= readbuf + rem) - break; - l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; - ptr += 2; - } - else - { - if (ptr + 5 >= readbuf + rem) - break; - l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); - ptr += 5; - } - if (*ptr == 0x10 && l >= 8) - keyid = grub_get_unaligned64 (ptr + 1); - } - } + rem = grub_be_to_cpu16 (unhashed_sub); + readbuf = grub_malloc (rem); + if (!readbuf) + goto fail; + + r = 0; + while (r < rem) + { + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r); + if (rr < 0) + goto fail; + if (rr == 0) + break; + r += rr; + } + if (r != rem) + goto fail; + + if (keyid == 0) + keyid = grub_subpacket_keyid_search (readbuf, rem); + grub_free (readbuf); hash->final (context); + readbuf = grub_zalloc (READBUF_SIZE); + if (!readbuf) + goto fail; + grub_dprintf ("crypt", "alive\n"); hval = hash->read (context); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-04-21 16:54 ` Ignat Korchagin @ 2016-04-28 21:32 ` Ignat Korchagin 2016-11-10 13:50 ` Daniel Kiper 0 siblings, 1 reply; 14+ messages in thread From: Ignat Korchagin @ 2016-04-28 21:32 UTC (permalink / raw) To: The development of GNU GRUB, Andrei Borzenkov Is this going to 2.02? I think it should, because it is a bug. On Thu, Apr 21, 2016 at 5:54 PM, Ignat Korchagin <ignat@cloudflare.com> wrote: > Best of both worlds: I left the READBUF_SIZE as is for the rest of the code, but rather reallocate the buffer with appropriate length for subpackets specifically. Hashing is outside of the keyid search function, so the name is good this time. > > Still did not get your response about PGP private test key. If it is not available, I can regenerate it and resign all test files for future use. > > diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c > index 4268eaa..91e1da6 100644 > --- a/grub-core/commands/verify.c > +++ b/grub-core/commands/verify.c > @@ -445,6 +445,38 @@ > return ret; > } > > +static grub_uint64_t > +grub_subpacket_keyid_search (const grub_uint8_t * sub, grub_ssize_t sub_len) > +{ > + const grub_uint8_t *ptr; > + grub_uint32_t l; > + grub_uint64_t keyid = 0; > + > + for (ptr = sub; ptr < sub + sub_len; ptr += l) > + { > + if (*ptr < 192) > + l = *ptr++; > + else if (*ptr < 255) > + { > + if (ptr + 1 >= sub + sub_len) > + break; > + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; > + ptr += 2; > + } > + else > + { > + if (ptr + 5 >= sub + sub_len) > + break; > + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); > + ptr += 5; > + } > + if (*ptr == 0x10 && l >= 8) > + keyid = grub_get_unaligned64 (ptr + 1); > + } > + > + return keyid; > +} > + > static grub_err_t > grub_verify_signature_real (char *buf, grub_size_t size, > grub_file_t f, grub_file_t sig, > @@ -529,20 +561,31 @@ > break; > hash->write (context, readbuf, r); > } > + grub_free (readbuf); > + > + readbuf = grub_malloc (rem); > + if (!readbuf) > + goto fail; > > hash->write (context, &v, sizeof (v)); > hash->write (context, &v4, sizeof (v4)); > - while (rem) > + > + r = 0; > + while (r < rem) > { > - r = grub_file_read (sig, readbuf, > - rem < READBUF_SIZE ? rem : READBUF_SIZE); > - if (r < 0) > + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r); > + if (rr < 0) > goto fail; > - if (r == 0) > + if (rr == 0) > break; > - hash->write (context, readbuf, r); > - rem -= r; > + r += rr; > } > + if (r != rem) > + goto fail; > + hash->write (context, readbuf, rem); > + keyid = grub_subpacket_keyid_search (readbuf, rem); > + grub_free (readbuf); > + > hash->write (context, &v, sizeof (v)); > s = 0xff; > hash->write (context, &s, sizeof (s)); > @@ -550,40 +593,34 @@ > r = grub_file_read (sig, &unhashed_sub, sizeof (unhashed_sub)); > if (r != sizeof (unhashed_sub)) > goto fail; > - { > - grub_uint8_t *ptr; > - grub_uint32_t l; > - rem = grub_be_to_cpu16 (unhashed_sub); > - if (rem > READBUF_SIZE) > - goto fail; > - r = grub_file_read (sig, readbuf, rem); > - if (r != rem) > - goto fail; > - for (ptr = readbuf; ptr < readbuf + rem; ptr += l) > - { > - if (*ptr < 192) > - l = *ptr++; > - else if (*ptr < 255) > - { > - if (ptr + 1 >= readbuf + rem) > - break; > - l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; > - ptr += 2; > - } > - else > - { > - if (ptr + 5 >= readbuf + rem) > - break; > - l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1)); > - ptr += 5; > - } > - if (*ptr == 0x10 && l >= 8) > - keyid = grub_get_unaligned64 (ptr + 1); > - } > - } > + rem = grub_be_to_cpu16 (unhashed_sub); > + readbuf = grub_malloc (rem); > + if (!readbuf) > + goto fail; > + > + r = 0; > + while (r < rem) > + { > + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r); > + if (rr < 0) > + goto fail; > + if (rr == 0) > + break; > + r += rr; > + } > + if (r != rem) > + goto fail; > + > + if (keyid == 0) > + keyid = grub_subpacket_keyid_search (readbuf, rem); > + grub_free (readbuf); > > hash->final (context); > > + readbuf = grub_zalloc (READBUF_SIZE); > + if (!readbuf) > + goto fail; > + > grub_dprintf ("crypt", "alive\n"); > > hval = hash->read (context); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-04-28 21:32 ` Ignat Korchagin @ 2016-11-10 13:50 ` Daniel Kiper 2016-11-13 9:18 ` Andrei Borzenkov 0 siblings, 1 reply; 14+ messages in thread From: Daniel Kiper @ 2016-11-10 13:50 UTC (permalink / raw) To: arvidjaar; +Cc: ignat, grub-devel On Thu, Apr 28, 2016 at 10:32:13PM +0100, Ignat Korchagin wrote: > Is this going to 2.02? I think it should, because it is a bug. Andrei, could you take care of it? Ignat, please repost this (taking into acount Andrei comments) as separate email with proper SOB, etc. Daniel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-11-10 13:50 ` Daniel Kiper @ 2016-11-13 9:18 ` Andrei Borzenkov 2016-11-15 12:42 ` Daniel Kiper 0 siblings, 1 reply; 14+ messages in thread From: Andrei Borzenkov @ 2016-11-13 9:18 UTC (permalink / raw) To: Daniel Kiper; +Cc: ignat, grub-devel 10.11.2016 16:50, Daniel Kiper пишет: > On Thu, Apr 28, 2016 at 10:32:13PM +0100, Ignat Korchagin wrote: >> Is this going to 2.02? I think it should, because it is a bug. > > Andrei, could you take care of it? > Yes, I will. I'd also appreciate refreshed patch against current tree. > Ignat, please repost this (taking into acount Andrei comments) > as separate email with proper SOB, etc. > > Daniel > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-11-13 9:18 ` Andrei Borzenkov @ 2016-11-15 12:42 ` Daniel Kiper 2016-11-15 12:43 ` Ignat Korchagin 0 siblings, 1 reply; 14+ messages in thread From: Daniel Kiper @ 2016-11-15 12:42 UTC (permalink / raw) To: Andrei Borzenkov; +Cc: Daniel Kiper, ignat, grub-devel On Sun, Nov 13, 2016 at 12:18:05PM +0300, Andrei Borzenkov wrote: > 10.11.2016 16:50, Daniel Kiper ??????????: > > On Thu, Apr 28, 2016 at 10:32:13PM +0100, Ignat Korchagin wrote: > >> Is this going to 2.02? I think it should, because it is a bug. > > > > Andrei, could you take care of it? > > > > Yes, I will. I'd also appreciate refreshed patch against current tree. Great! Thanks a lot! Daniel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] verify: search keyid in hashed signature subpackets 2016-11-15 12:42 ` Daniel Kiper @ 2016-11-15 12:43 ` Ignat Korchagin 0 siblings, 0 replies; 14+ messages in thread From: Ignat Korchagin @ 2016-11-15 12:43 UTC (permalink / raw) To: Daniel Kiper; +Cc: Andrei Borzenkov, The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 565 bytes --] A little busy now, but will try to send the patch this week. On Tue, Nov 15, 2016 at 12:42 PM, Daniel Kiper <dkiper@net-space.pl> wrote: > On Sun, Nov 13, 2016 at 12:18:05PM +0300, Andrei Borzenkov wrote: > > 10.11.2016 16:50, Daniel Kiper ??????????: > > > On Thu, Apr 28, 2016 at 10:32:13PM +0100, Ignat Korchagin wrote: > > >> Is this going to 2.02? I think it should, because it is a bug. > > > > > > Andrei, could you take care of it? > > > > > > > Yes, I will. I'd also appreciate refreshed patch against current tree. > > Great! Thanks a lot! > > Daniel > [-- Attachment #2: Type: text/html, Size: 1023 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-11-15 12:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-29 19:02 [PATCH] verify: search keyid in hashed signature subpackets Ignat Korchagin 2016-03-30 4:44 ` Andrei Borzenkov 2016-03-30 8:47 ` Ignat Korchagin 2016-03-30 9:38 ` Andrei Borzenkov 2016-03-30 14:09 ` Ignat Korchagin 2016-04-09 4:27 ` Andrei Borzenkov 2016-04-10 18:34 ` Ignat Korchagin 2016-04-19 14:28 ` Ignat Korchagin 2016-04-21 16:54 ` Ignat Korchagin 2016-04-28 21:32 ` Ignat Korchagin 2016-11-10 13:50 ` Daniel Kiper 2016-11-13 9:18 ` Andrei Borzenkov 2016-11-15 12:42 ` Daniel Kiper 2016-11-15 12:43 ` Ignat Korchagin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.