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