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

[Andrei Borzenkov <arvidjaar@gmail.com>]

30.03.2016 17:09, Ignat Korchagin пишет:
> Implemented as a separate function which should process arbitrary length data.

TBH I still think that simply setting READBUF_SIZE to 64K is the
simplest solution.

> As for tests, it seems that the easiest way is to add this signature
to tests/file_filter. Not sure how should I send you the patch with
binary data though.
>

Just sign files and send new signatures and keys, I will commit them.

> diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c
> index 166d0aa..cc8fa39 100644
> --- a/grub-core/commands/verify.c
> +++ b/grub-core/commands/verify.c
> @@ -445,6 +445,88 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval,
>    return ret;
>  }
>  
> +static grub_uint64_t
> +grub_subpacket_keyid_search (grub_file_t f, grub_ssize_t sub_len,
> +			     const gcry_md_spec_t *hash, void *context)

This does more than just searching for keyid, it also hashes content, so
name is misleading.

> +{
> +  grub_uint64_t keyid = 0;
> +  grub_uint8_t *readbuf = NULL;
> +
> +  while (sub_len > 0)
> +    {
> +      grub_uint8_t szid[5];
> +      grub_size_t sz_len;
> +      grub_size_t l;
> +
> +      grub_ssize_t r = grub_file_read (f, szid, 1);
> +      if (r != 1)
> +        return 0;
> +
> +      if (szid[0] < 192)
> +	     {
> +          l = szid[0];
> +          sz_len = 1;
> +        }
> +      else if (szid[0] < 255)
> +        {
> +          r = grub_file_read (f, szid + 1, 1);
> +          if (r != 1)
> +            return 0;
> +
> +          l = (((szid[0] & ~192) << GRUB_CHAR_BIT) | szid[1]) + 192;
> +          sz_len = 2;
> +        }
> +      else
> +        {
> +          r = grub_file_read (f, szid + 1, 4);
> +          if (r != 4)
> +            return 0;
> +
> +          l = grub_be_to_cpu32 (grub_get_unaligned32 (szid + 1));
> +          sz_len = 5;
> +        }
> +
> +      readbuf = grub_zalloc (l);

So you allocate full subpacket length anyway. Why not set READBUF_SIZE
to max size then from the very start?

> +      if (!readbuf)
> +        return 0;
> +
> +      r = grub_file_read (f, readbuf, l);
> +      if (r <= 0)
> +        goto fail;
> +
> +      while ((grub_size_t)r < l)
> +        {
> +          grub_ssize_t rr = grub_file_read (f, readbuf + r, l - (grub_size_t)r);
> +          if (rr <= 0)
> +            goto fail;
> +          r += rr;
> +        }
> +
> +      if (*readbuf == 0x10 && l >= 8)
> +        keyid = grub_get_unaligned64 (readbuf + 1);
> +
> +      if (hash && context)
> +        {
> +          hash->write (context, szid, sz_len);
> +          hash->write (context, readbuf, l);
> +        }
> +
> +      grub_free (readbuf);
> +      readbuf = NULL;
> +
> +      sub_len -= sz_len + l;
> +    }
> +
> +fail:
> +  if (readbuf)
> +    {
> +      grub_free (readbuf);
> +      return 0;
> +    }
> +
> +  return keyid;
> +}
> +
>  static grub_err_t
>  grub_verify_signature_real (char *buf, grub_size_t size,
>  			    grub_file_t f, grub_file_t sig,
> @@ -532,17 +614,7 @@ grub_verify_signature_real (char *buf, grub_size_t size,
>  
>      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;
> -      }
> +    keyid = grub_subpacket_keyid_search (sig, rem, hash, context);
>      hash->write (context, &v, sizeof (v));
>      s = 0xff;
>      hash->write (context, &s, sizeof (s));
> @@ -550,37 +622,11 @@ 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);
> +    if (keyid == 0)
> +      keyid = grub_subpacket_keyid_search (sig, rem, NULL, NULL);
> +    else
> +      grub_subpacket_keyid_search (sig, rem, NULL, NULL);
>  
>      hash->final (context);
>  
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>