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

[Ignat Korchagin]

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