Charles Duffy <char...@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 <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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
