Re: [PATCH] verify: search keyid in hashed signature subpackets (repost)

[Daniel Kiper]

On Mon, Nov 21, 2016 at 11:25:30PM +0100, Ignat Korchagin wrote:
> On Mon, Nov 21, 2016 at 3:45 PM, Daniel Kiper <address@hidden> 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 <address@hidden>
> >> ---
> >>  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.

OK, however, I am against leaving unreadable code if we touch it anyway.

> 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

So, say this thing in comment before grub_subpacket_keyid_search() function.
This will improve situation a bit.

> >> +        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.

Right, I realized that after posting my email. Hence, let's
leave grub_malloc() as is here and below.

Daniel