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

[Andrei Borzenkov <arvidjaar@gmail.com>]

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);
>