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

[Ignat Korchagin]

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 <address@hidden> 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
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel