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

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

[Ignat Korchagin]

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 <ignat@cloudflare.com>
---
 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)
+        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 == 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);
+    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);
+    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;
+
+    if (keyid == 0)
+      keyid = grub_subpacket_keyid_search (readbuf, rem);
+    grub_free (readbuf);
 
     hash->final (context);
 
+    readbuf = grub_zalloc (READBUF_SIZE);
+    if (!readbuf)
+      goto fail;
+
     grub_dprintf ("crypt", "alive\n");
 
     hval = hash->read (context);
-- 
2.1.4

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

[Daniel Kiper]

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 <ignat@cloudflare.com>
> ---
>  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...

> +        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()?

> +    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()?

Daniel

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

[Jon McCune]

[-- Attachment #1: Type: text/plain, Size: 914 bytes --]

On Mon, Nov 21, 2016 at 6:45 AM, Daniel Kiper <dkiper@net-space.pl> 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.
>

I think it would be wise to include a brief argument citing the OpenPGP RFC
that this change is compliant. Compatibility with an existing
implementation is valuable, but let's make sure the appropriate code is
being changed. (I haven't looked carefully myself.)

Thanks,
-Jon

[-- Attachment #2: Type: text/html, Size: 1413 bytes --]

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

[Ignat Korchagin]

On Mon, Nov 21, 2016 at 3:45 PM, Daniel Kiper <dkiper@net-space.pl> 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 <ignat@cloudflare.com>
>> ---
>>  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

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

[Ignat Korchagin]

On Mon, Nov 21, 2016 at 6:56 PM, Jon McCune <jonmccune@google.com> wrote:
> On Mon, Nov 21, 2016 at 6:45 AM, Daniel Kiper <dkiper@net-space.pl> 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.
>
>
> I think it would be wise to include a brief argument citing the OpenPGP RFC
> that this change is compliant. Compatibility with an existing implementation
> is valuable, but let's make sure the appropriate code is being changed. (I
> haven't looked carefully myself.)
>
> Thanks,
> -Jon
>
>

This change is compliant with RFC 4880. According to p 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.

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 <dkiper@net-space.pl> 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 <ignat@cloudflare.com>
> >> ---
> >>  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

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

[Daniel Kiper]

On Mon, Nov 21, 2016 at 11:31:26PM +0100, Ignat Korchagin wrote:
> On Mon, Nov 21, 2016 at 6:56 PM, Jon McCune <jonmccune@google.com> wrote:
> > On Mon, Nov 21, 2016 at 6:45 AM, Daniel Kiper <dkiper@net-space.pl> 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.
> >
> >
> > I think it would be wise to include a brief argument citing the OpenPGP RFC
> > that this change is compliant. Compatibility with an existing implementation
> > is valuable, but let's make sure the appropriate code is being changed. (I
> > haven't looked carefully myself.)
> >
> > Thanks,
> > -Jon
> >
> >
>
> This change is compliant with RFC 4880. According to p 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.

Please add this to commit message.

Daniel

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

[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)
+        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;
+    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;
+
+    if (keyid == 0)
+      keyid = grub_subpacket_keyid_search (readbuf, rem);
+    grub_free (readbuf);
 
     hash->final (context);
 
+    readbuf = grub_zalloc (READBUF_SIZE);
+    if (!readbuf)
+      goto fail;
+
     grub_dprintf ("crypt", "alive\n");
 
     hval = hash->read (context);
-- 
2.1.4

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

[Daniel Kiper]

On Fri, Dec 02, 2016 at 04:58:39PM +0000, Ignat Korchagin wrote:
> 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.

In general LGTM. One nitpick, may I add your SOB (Signed-off-by) here?
Otherwise if there are no objections in 2-3 days I will apply it.

Daniel

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

[Ignat Korchagin]

> One nitpick, may I add your SOB (Signed-off-by) here?
Sorry, somehow I missed it this time (probably forgot to specify the
git option).
Yes, please add it.

On Mon, Dec 5, 2016 at 3:24 PM, Daniel Kiper <dkiper@net-space.pl> wrote:
> On Fri, Dec 02, 2016 at 04:58:39PM +0000, Ignat Korchagin wrote:
>> 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.
>
> In general LGTM. One nitpick, may I add your SOB (Signed-off-by) here?
> Otherwise if there are no objections in 2-3 days I will apply it.
>
> Daniel

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

[Andrei Borzenkov]

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

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

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

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

> +    if (!readbuf)
> +      goto fail;
> +
>      grub_dprintf ("crypt", "alive\n");
>  
>      hval = hash->read (context);
> 

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 <arvidjaar@gmail.com> 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);
>>
>

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

[Daniel Kiper]

Hi Ignat,

On Sun, Dec 11, 2016 at 02:51:00PM +0000, Ignat Korchagin wrote:
> 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.

[...]

Thank you for your work. I understand your POV but I think that Andrei's questions
are valid too. I know that this sounds stupid but please be patient. We are trying
to rectify all GRUB2 maintenance issues. Unfortunately it takes time especially if
backlog is huge. Personally I can promise that I will do my best to get your fixes
into 2.02 release. However, we need your support and a bit of understanding too.

Daniel

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

[Ignat Korchagin]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

Hi,

> please be patient
I'm not in a hurry. Like probably everyone else I have a fork repo where
all changes are present. Just wanted to rely more on upstream in the future

> Unfortunately it takes time especially if backlog is huge
That is my point: if environment is more friendly, probably you would get
more help in working through backlog

But, anyway, back to the patch: I recovered some of the context of my code,
so here are the details

> I think this loop is overcomplicated. In all other places we assume that
> short read from grub_file_read means error.
This loop validates incorrect (or even bogus) signature format.
The format should be (simplified) |total len|subpack1|subpack2|....
Each subpacket has its own length specified as well
This loop tries to verify that the overall processed packet length match.
Since we we process arbitrary length here, I do not see a better approach

As for other concerns I commented in my previous reply to the patch.

Thank you.

On Mon, Dec 12, 2016 at 1:20 PM, Daniel Kiper <dkiper@net-space.pl> wrote:

> Hi Ignat,
>
> On Sun, Dec 11, 2016 at 02:51:00PM +0000, Ignat Korchagin wrote:
> > 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.
>
> [...]
>
> Thank you for your work. I understand your POV but I think that Andrei's
> questions
> are valid too. I know that this sounds stupid but please be patient. We
> are trying
> to rectify all GRUB2 maintenance issues. Unfortunately it takes time
> especially if
> backlog is huge. Personally I can promise that I will do my best to get
> your fixes
> into 2.02 release. However, we need your support and a bit of
> understanding too.
>
> Daniel
>

[-- Attachment #2: Type: text/html, Size: 3786 bytes --]