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

* [PATCH] pgp: Recognize issuer subpackets in either hashed or unhashed sections
@ 2020-05-30 21:20 Charles Duffy
  2020-05-31 17:51 ` Charles Duffy
  0 siblings, 1 reply; 3+ messages in thread
From: Charles Duffy @ 2020-05-30 21:20 UTC (permalink / raw)
  To: grub-devel

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



^ permalink raw reply related	[flat|nested] 3+ messages in thread