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).
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 <char...@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 <ig...@cloudflare.com> > Signed-off-by: Daniel Axtens <d...@axtens.net> > Signed-off-by: Charles Duffy <char...@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
