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

[Andrei Borzenkov]

On Wed, Mar 30, 2016 at 11:47 AM, Ignat Korchagin <address@hidden> 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 <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
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel