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 <arvidj...@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
