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

[Ignat Korchagin]

General thoughts:
Just a reminder: this patch tries to fix a BUG in code, which was
present from the introduction of this functionality and acknowledged
for more than 9 month now. The goal of this patch is to fix it without
introducing too much change. We are spending too much time "improving"
things and forgetting that basic functionality is broken.

Probably, most of us have experience working in software companies and
the basic flow of any software development is:
  - fix bugs
  - improve
  - goto step 1;
So fixing bugs and improvements should be in separate steps. I do not
think it is an excuse not to fix bugs for 9 month in the hope of
improving code readability along the way. That should be a separate
task. Unfortunately, I do not have time to continuously rebase the PR
for each 1 month delay - GRUB is not my primary and even second
responsibility at work. I'm just trying to be nice and contribute back
what I found broken. But I'm just a regular user and I have to cut
this time from my family to do this.

Actually, IMHO, this whole file needs to be redone, so comments
regarding "this or that unclear" applies to everything there. As I
said multiple times, I almost did not introduce new code, but just
refactored the existing one fix the issue. I'm not aware of any
decisions made prior to this patch.

Many times in this mailing list there were requests to be considerate
that GRUB maintainers are very busy and cannot always act/respond
quickly. But, guess what: this goes the other way around - GRUB
contributors can be busy as well. If you make me rebase the patch each
month with new not very relevant comments I will give up essentially.
If you think the code needs improvements, you are free to ask me for a
follow-up with general directions/requirements for what you think is
wrong. But, please, make a little effort to fix the work already done.
The patch is not that big, but to prepare it I had to spend time to
read and understand the whole PGP RFC (and their data formats and
encodings are not that simple I would say). And after 9 month I do not
even remember the specifics.

General thoughts on improving GRUB development:
  - let's be tolerant to each other
  - for God's sake, let's move to a modern code hosting with
convenient modern workflows (PRs, reviews etc). Yes, previously you
said that you want to be able to review patches on the phone, but I do
not imagine you guys having not being able to do this with a
reasonable smartphone. Because fighting with git send-mail and
subverting my email 2-factor auth just to send a patch makes me (and
probably many others) very sad (and insecure BTW)
  - there should be no comments, like (taken from real-life) "I do not
think this is a right name for this function". The right form would be
"I do not think this is a right name for this function, how about <PUT
YOUR PROPOSAL HERE>". We are not mind-readers and do not have full
visibility into GRUB project.
  - ...

Sorry for the noise, but this is something which came up working on this...
Specific patch comments below.

On Sat, Dec 10, 2016 at 5:59 PM, Andrei Borzenkov <address@hidden> wrote:
> 02.12.2016 19:58, Ignat Korchagin пишет:
>> According to RFC 4880 5.2.3 only "Signature Creation Time" subpacket
>> "MUST" be present in the hashed area. All other subpacket types may be 
>> present
>> either in hashed or unhashed areas. Currently GRUB assumes, that the "Issuer"
>> subpacket is in unhashed area (by default put there by gpg tool), but other
>> PGP implementations (like https://godoc.org/golang.org/x/crypto/openpgp)
>> may put it in the hashed area.
>> ---
>>  grub-core/commands/verify.c | 122 
>> ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 83 insertions(+), 39 deletions(-)
>>
>> diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c
>> index 67cb1c7..79b3826 100644
>> --- a/grub-core/commands/verify.c
>> +++ b/grub-core/commands/verify.c
>> @@ -33,6 +33,9 @@
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> +/* RFC 4880 5.2.3.1 */
>> +#define OPENPGP_SIGNATURE_SUBPACKET_TYPE 16
>> +
>>  struct grub_verified
>>  {
>>    grub_file_t file;
>> @@ -445,6 +448,42 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval,
>>    return ret;
>>  }
>>
>> +/*
>> + * Parsing algorithm from RFC 4880 5.2.3.1
>> + */
>> +
>> +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)
>> +        l = *ptr++;
>> +      else if (*ptr < 255)
>> +        {
>> +          if (ptr + 1 >= sub + sub_len)
>> +            break;
>> +          l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
>> +          ptr += 2;
>> +        }
>> +      else
>> +        {
>> +          if (ptr + 5 >= sub + sub_len)
>> +            break;
>> +          l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
>> +          ptr += 5;
>> +        }
>> +      if (*ptr == OPENPGP_SIGNATURE_SUBPACKET_TYPE && l >= 8)
>
> Overflow check? ptr + 8 < ptr + sub_len

This and more: this specific packet can be in the middle of "packet
collection", but its length should be >=8. We do not want to
potentially read data from subsequent packets (no overflow, but
getting invalid data).
>> +        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 +568,31 @@ grub_verify_signature_real (char *buf, grub_size_t 
>> size,
>>           break;
>>         hash->write (context, readbuf, r);
>>       }
>> +    grub_free (readbuf);
>> +
>> +    readbuf = grub_malloc (rem);
>> +    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;
>
> I think this loop is overcomplicated. In all other places we assume that
> short read from grub_file_read means error.
>

I remember I had justification for it, but do not remember what it is,
because I wrote this 9 month ago.
>> +    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 +600,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);
>> +    if (!readbuf)
>> +      goto fail;
>> +
>> +    r = 0;
>> +    while (r < rem)
>> +      {
>> +        grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r);
>> +        if (rr < 0)
>> +          goto fail;
>> +        if (rr == 0)
>> +          break;
>> +        r += rr;
>> +      }
>> +    if (r != rem)
>> +      goto fail;
>> +
>
> Ditto.
>

Ditto.
>> +    if (keyid == 0)
>> +      keyid = grub_subpacket_keyid_search (readbuf, rem);
>> +    grub_free (readbuf);
>>
>>      hash->final (context);
>>
>> +    readbuf = grub_zalloc (READBUF_SIZE);
>
> No need to use grub_zalloc here, we did not zero buffer before as well.
>

We agreed previously that we should keep "what is correct" (your
comment from 30 March 2016). Currently the function allocates the
buffer with grub_zalloc (READBUF_SIZE). I changed buffer allocation
for my code, but I have no idea about the assumptions of the code
which follows. So, if previously, that code used buffer allocalted
with grub_zalloc I do the same for compatibility.
>> +    if (!readbuf)
>> +      goto fail;
>> +
>>      grub_dprintf ("crypt", "alive\n");
>>
>>      hval = hash->read (context);
>>
>