From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1jisMx-0003pS-3j for mharc-grub-devel@gnu.org; Wed, 10 Jun 2020 00:29:43 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34448) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jisMt-0003kp-OI for grub-devel@gnu.org; Wed, 10 Jun 2020 00:29:39 -0400 Received: from mail-pf1-x442.google.com ([2607:f8b0:4864:20::442]:34103) by eggs.gnu.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jisMs-0008Th-Al for grub-devel@gnu.org; Wed, 10 Jun 2020 00:29:39 -0400 Received: by mail-pf1-x442.google.com with SMTP id z64so557131pfb.1 for ; Tue, 09 Jun 2020 21:29:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:subject:in-reply-to:references:date:message-id:mime-version; bh=u1CRaaqSFgvUS9KwhMocAMBvK7l+EJdYQmu5caQmPlw=; b=JD+UxIT+4pAhEE/iBSHMPXs7rUL1/0jnadbgirhkDOgORvUyrAnK8aPxl2wJUvM8fW /O4eiw8NKcCfEd8642ml8OkqWxVLkwuZ29SWW0WTDOvlxL+pClom6NAXQ7yU4KPjYzRV lMkymHJMv+mvuPeEH+77esM2HkQxlgdW/7+Mo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=u1CRaaqSFgvUS9KwhMocAMBvK7l+EJdYQmu5caQmPlw=; b=pYtuKrhPVGO0fk0Agl/c9hhJ8EloUkMgwwcgcLbbBilBPMrq/kaxVfnvK0J6TM62lA Va0HF7kBE9iFGUpZW5wQl/R48HBwGFGwymmdnVDqqBKNlDpgACmAT/YMt99I9bPbwboA Ia4s0s3HUoD64Qs/QI9z6m6ddFt+vYpTkCNba9d1EWty/eRWQz7iTl5YMCoBJj/LnMLY mQ2dmly6/zORIXAVf+xuzkzKlV1/sm+0dcb01RF3Joi6lU4ESsHE8sRlSoKLGYOirGTo oNaWAN2Kq4BpJxtOhVnFqGK7f5Mf8/GruPs0Nch3B0gkVIwz+G8kdKlFoBZBd46WFIRZ G9gg== X-Gm-Message-State: AOAM5309K4ezkSwlVEaEp7TxfOMPzLDakD1bBqDZy9d4Az6N9sHXCL1M Ir2TNxSquOPKbYztJ3cuPgj0Ee4DbKY= X-Google-Smtp-Source: ABdhPJxPFEwzOmZiJQSIVGAwxm2hngs82CGACaQi5a+LNMpcgEFT3V69XxtRwL/xPeRyZ9KTIpZz7g== X-Received: by 2002:a05:6a00:1589:: with SMTP id u9mr1051451pfk.201.1591763374697; Tue, 09 Jun 2020 21:29:34 -0700 (PDT) Received: from localhost (2001-44b8-1113-6700-45bd-9987-79fb-133f.static.ipv6.internode.on.net. [2001:44b8:1113:6700:45bd:9987:79fb:133f]) by smtp.gmail.com with ESMTPSA id u21sm11141097pfn.123.2020.06.09.21.29.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Jun 2020 21:29:33 -0700 (PDT) From: Daniel Axtens To: Charles Duffy , grub-devel@gnu.org Subject: Re: [PATCH] pgp: Recognize issuer subpackets in either hashed or unhashed sections In-Reply-To: References: <20200530211727.GA9870@shiny5l> Date: Wed, 10 Jun 2020 14:29:30 +1000 Message-ID: <87d067tufp.fsf@dja-thinkpad.axtens.net> MIME-Version: 1.0 Content-Type: text/plain Received-SPF: pass client-ip=2607:f8b0:4864:20::442; envelope-from=dja@axtens.net; helo=mail-pf1-x442.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Jun 2020 04:29:40 -0000 Charles Duffy 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 > > > Thanks/apologies/&c., > -- Charles > > On Sat, May 30, 2020 at 4:20 PM Charles Duffy 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 >> Signed-off-by: Daniel Axtens >> Signed-off-by: Charles Duffy >> --- >> 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