Re: [PATCH] pgp: Recognize issuer subpackets in either hashed or unhashed sections

[Daniel Axtens]

Charles Duffy <charles@dyfis.net> writes:

> Looking at the mailing list archive, it appears that the patch may have
> been munged by line wrap (unclear as to why; used `mutt -H` to send `git
> format-patch`'s output).

I use git-send-email, that might give better results?

Anyway I think it might be worth adding a test to signature_test. I know
the test suite is a bit crufty so it may end up being too big an
undertaking, but I think it would be good to at least try.

Regards,
Daniel

>
> Regardless, if it doesn't apply for anyone, a byte-for-byte unmodified copy
> can also be found at
> https://raw.githubusercontent.com/charles-dyfis-net/grub-openpgp-compat-test/fa5029ee0c2a8be073b05245c247c2610b5f864d/0001-pgp-Recognize-issuer-subpackets-in-either-hashed-or-.patch
> <https://raw.githubusercontent.com/charles-dyfis-net/grub-openpgp-compat-test/master/0001-pgp-Recognize-issuer-subpackets-in-either-hashed-or-.patch>
>
> Thanks/apologies/&c.,
> -- Charles
>
> On Sat, May 30, 2020 at 4:20 PM Charles Duffy <charles@dyfis.net> wrote:
>
>> Currently, GRUB's OpenPGP signature parsing searches for the issuer
>> field (specifying the key to use) only in the unhashed portion of the
>> signature.
>>
>> RFC 4880 permits almost all fields (with the sole exception of signature
>> creation time, which MUST be recognized only in the hashed area) to be
>> present either in hashed or unhashed areas; and specifies that
>> implementations should use the unhashed area only for "advisory
>> information".
>>
>> While GnuPG's decision to consider issuer ID advisory is defensible
>> (after all, one could simply do an exhaustive search of known public
>> keys in its absence), it is not the only valid decision; in particular,
>> the Go x/crypto/openpgp library chooses to store issuer ID in the hashed
>> area.
>>
>> Without this patch, trying to verify a valid signature made by
>> x/crypto/openpgp results in `error: public key 00000000 not found.`,
>> because the `keyid` variable is unpopulated.
>>
>> This patch, originally written by Ignat Korchagin and ported to GRUB
>> 2.04 by Daniel Axtens, remedies this. I (Charles Duffy) have tried to
>> address review comments on the original requesting that named constants
>> be used to enhance readability.
>>
>> There are still outstanding compatibility issues parsing public keys
>> not serialized by GnuPG; these may be addressed in a later patch.
>>
>> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> Signed-off-by: Charles Duffy <charles@dyfis.net>
>> ---
>>  grub-core/commands/pgp.c | 137 +++++++++++++++++++++++++++------------
>>  1 file changed, 97 insertions(+), 40 deletions(-)
>>
>> diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
>> index bbf6871fe..b49b997c4 100644
>> --- a/grub-core/commands/pgp.c
>> +++ b/grub-core/commands/pgp.c
>> @@ -39,6 +39,17 @@ enum
>>      OPTION_SKIP_SIG = 0
>>    };
>>
>> +enum
>> +  {
>> +    OPENPGP_SIG_SUBPACKET_TYPE_ISSUER = 16 /* subpacket contains 8-octet
>> key id, see RFC4880 5.2.3.5 */
>> +  };
>> +
>> +enum
>> +  {
>> +    OPENPGP_SIG_SUBPACKET_LEN_2BYTE_LIMIT = 192,
>> +    OPENPGP_SIG_SUBPACKET_LEN_5BYTE_CONST = 255
>> +  };
>> +
>>  static const struct grub_arg_option options[] =
>>    {
>>      {"skip-sig", 's', 0,
>> @@ -448,6 +459,47 @@ struct grub_pubkey_context
>>    void *hash_context;
>>  };
>>
>> +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)
>> +    {
>> +      /* this algorithm is expressely given in RFC 4880 5.2.3.1 to parse
>> length
>> +       * specifications, which may be 1-byte, 2-byte or 5-bytes long */
>> +      if (*ptr < OPENPGP_SIG_SUBPACKET_LEN_2BYTE_LIMIT)
>> +       l = *ptr++;
>> +      /* 2-octet length field; the two high bits used to specify this
>> format
>> +       * are not part of the data, and the value as a whole is offset to
>> avoid
>> +       * having multiple ways to specify values that would fit in the
>> 1-byte
>> +       * form */
>> +      else if (*ptr < OPENPGP_SIG_SUBPACKET_LEN_5BYTE_CONST)
>> +       {
>> +         if (ptr + 1 >= sub + sub_len)
>> +           break;
>> +         l = (((ptr[0] - OPENPGP_SIG_SUBPACKET_LEN_2BYTE_LIMIT) <<
>> GRUB_CHAR_BIT) | ptr[1])
>> +                + OPENPGP_SIG_SUBPACKET_LEN_2BYTE_LIMIT;
>> +         ptr += 2;
>> +       }
>> +      /* 5-octet length field, 0xff constant followed by 4-byte value */
>> +      else
>> +       {
>> +         if (ptr + 5 >= sub + sub_len)
>> +           break;
>> +         l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
>> +         ptr += 5;
>> +       }
>> +      /* determine whether we found the packet we're looking for */
>> +      if (*ptr == OPENPGP_SIG_SUBPACKET_TYPE_ISSUER && l >= 8)
>> +       keyid = grub_get_unaligned64 (ptr + 1);
>> +    }
>> +
>> +  return keyid;
>> +}
>> +
>>  static grub_err_t
>>  grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t
>> sig)
>>  {
>> @@ -520,7 +572,7 @@ grub_verify_signature_real (struct grub_pubkey_context
>> *ctxt,
>>    gcry_mpi_t mpis[10];
>>    grub_uint8_t pk = ctxt->v4.pkeyalgo;
>>    grub_size_t i;
>> -  grub_uint8_t *readbuf = NULL;
>> +  grub_uint8_t *readbuf = NULL, *subpacket_buf = NULL;
>>    unsigned char *hval;
>>    grub_ssize_t rem = grub_be_to_cpu16 (ctxt->v4.hashed_sub);
>>    grub_uint32_t headlen = grub_cpu_to_be32 (rem + 6);
>> @@ -538,17 +590,29 @@ grub_verify_signature_real (struct
>> grub_pubkey_context *ctxt,
>>
>>    ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
>>    ctxt->hash->write (ctxt->hash_context, &ctxt->v4, sizeof (ctxt->v4));
>> -  while (rem)
>> +
>> +  subpacket_buf = grub_malloc (rem);
>> +  if (!subpacket_buf)
>> +    goto fail;
>> +
>> +  r = 0;
>> +  while (r < rem)
>>      {
>> -      r = grub_file_read (ctxt->sig, readbuf,
>> -                         rem < READBUF_SIZE ? rem : READBUF_SIZE);
>> -      if (r < 0)
>> -       goto fail;
>> -      if (r == 0)
>> +      grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem
>> - r);
>> +      if (rr < 0)
>> +        goto fail;
>> +      if (rr == 0)
>>         break;
>> -      ctxt->hash->write (ctxt->hash_context, readbuf, r);
>> -      rem -= r;
>> +      r += rr;
>>      }
>> +  if (r != rem)
>> +    goto fail;
>> +  ctxt->hash->write (ctxt->hash_context, subpacket_buf, rem);
>> +
>> +  keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
>> +  grub_free (subpacket_buf);
>> +  subpacket_buf = NULL;
>> +
>>    ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
>>    s = 0xff;
>>    ctxt->hash->write (ctxt->hash_context, &s, sizeof (s));
>> @@ -556,37 +620,27 @@ grub_verify_signature_real (struct
>> grub_pubkey_context *ctxt,
>>    r = grub_file_read (ctxt->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 (ctxt->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);
>> +  subpacket_buf = grub_malloc (rem);
>> +  if (!subpacket_buf)
>> +    goto fail;
>> +
>> +  r = 0;
>> +  while (r < rem)
>> +    {
>> +     grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + 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 (subpacket_buf, rem);
>>
>>    ctxt->hash->final (ctxt->hash_context);
>>
>> @@ -656,10 +710,13 @@ grub_verify_signature_real (struct
>> grub_pubkey_context *ctxt,
>>      goto fail;
>>
>>    grub_free (readbuf);
>> +  grub_free (subpacket_buf);
>>
>>    return GRUB_ERR_NONE;
>>
>>   fail:
>> +  if (subpacket_buf)
>> +    grub_free (subpacket_buf);
>>    grub_free (readbuf);
>>    if (!grub_errno)
>>      return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>> --
>> 2.25.4
>>
>>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel