On Mon, Nov 21, 2016 at 3:45 PM, Daniel Kiper <dki...@net-space.pl> wrote: > On Fri, Nov 18, 2016 at 12:00:08PM +0000, Ignat Korchagin wrote: >> Reposting this, as requested by Daniel and rebasing on current tree. >> >> 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. >> >> Signed-off-by: Ignat Korchagin <ig...@cloudflare.com> >> --- >> grub-core/commands/verify.c | 115 >> +++++++++++++++++++++++++++++--------------- >> 1 file changed, 76 insertions(+), 39 deletions(-) >> >> diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c >> index 67cb1c7..1b628b2 100644 >> --- a/grub-core/commands/verify.c >> +++ b/grub-core/commands/verify.c >> @@ -445,6 +445,38 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval, >> 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) > > Please define some constants and use them properly... This is original GRUB code here. It was just moved to a separate function. See below. Also, defining constants here is probably more confusing. I do not know for sure, but suspect this algorithm is taken directly from RFC 4880 5.2.3.1 and there are no names there > >> + l = *ptr++; >> + else if (*ptr < 255) > > Ditto. > >> + { >> + if (ptr + 1 >= sub + sub_len) >> + break; >> + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192; > > Ditto. > >> + ptr += 2; > > Ditto and below... > >> + } >> + 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 @@ grub_verify_signature_real (char *buf, grub_size_t >> size, >> break; >> hash->write (context, readbuf, r); >> } >> + grub_free (readbuf); >> + >> + readbuf = grub_malloc (rem); > > grub_zalloc()? This buffer is filled below by grub_file_read, so no need to waste cycles zeroing it. > >> + 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) >> - goto fail; >> - if (r == 0) >> + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r); >> + if (rr < 0) >> + goto fail; >> + 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 @@ 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); >> + readbuf = grub_malloc (rem); > > grub_zalloc()? Same as above > > Daniel
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
