02.12.2016 19:58, Ignat Korchagin пишет:
> According to RFC 4880 5.2.3 only "Signature Creation Time" subpacket
> "MUST" be present in the hashed area. All other subpacket types may be present
> either in hashed or unhashed areas. Currently GRUB assumes, that the "Issuer"
> subpacket is in unhashed area (by default put there by gpg tool), but other
> PGP implementations (like https://godoc.org/golang.org/x/crypto/openpgp)
> may put it in the hashed area.
> ---
> grub-core/commands/verify.c | 122
> ++++++++++++++++++++++++++++++--------------
> 1 file changed, 83 insertions(+), 39 deletions(-)
>
> diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c
> index 67cb1c7..79b3826 100644
> --- a/grub-core/commands/verify.c
> +++ b/grub-core/commands/verify.c
> @@ -33,6 +33,9 @@
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> +/* RFC 4880 5.2.3.1 */
> +#define OPENPGP_SIGNATURE_SUBPACKET_TYPE 16
> +
> struct grub_verified
> {
> grub_file_t file;
> @@ -445,6 +448,42 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval,
> return ret;
> }
>
> +/*
> + * Parsing algorithm from RFC 4880 5.2.3.1
> + */
> +
> +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)
> + {
> + if (*ptr < 192)
> + l = *ptr++;
> + else if (*ptr < 255)
> + {
> + if (ptr + 1 >= sub + sub_len)
> + break;
> + l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
> + ptr += 2;
> + }
> + else
> + {
> + if (ptr + 5 >= sub + sub_len)
> + break;
> + l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
> + ptr += 5;
> + }
> + if (*ptr == OPENPGP_SIGNATURE_SUBPACKET_TYPE && l >= 8)
Overflow check? ptr + 8 < ptr + sub_len
> + keyid = grub_get_unaligned64 (ptr + 1);
> + }
> +
> + return keyid;
> +}
> +
> static grub_err_t
> grub_verify_signature_real (char *buf, grub_size_t size,
> grub_file_t f, grub_file_t sig,
> @@ -529,20 +568,31 @@ grub_verify_signature_real (char *buf, grub_size_t size,
> break;
> hash->write (context, readbuf, r);
> }
> + grub_free (readbuf);
> +
> + readbuf = grub_malloc (rem);
> + if (!readbuf)
> + goto fail;
>
> hash->write (context, &v, sizeof (v));
> hash->write (context, &v4, sizeof (v4));
> - while (rem)
> +
> + r = 0;
> + while (r < rem)
> {
> - r = grub_file_read (sig, readbuf,
> - rem < READBUF_SIZE ? rem : READBUF_SIZE);
> - if (r < 0)
> - goto fail;
> - if (r == 0)
> + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r);
> + if (rr < 0)
> + goto fail;
> + if (rr == 0)
> break;
> - hash->write (context, readbuf, r);
> - rem -= r;
> + r += rr;
> }
> + if (r != rem)
> + goto fail;
I think this loop is overcomplicated. In all other places we assume that
short read from grub_file_read means error.
> + hash->write (context, readbuf, rem);
> + keyid = grub_subpacket_keyid_search (readbuf, rem);
> + grub_free (readbuf);
> +
> hash->write (context, &v, sizeof (v));
> s = 0xff;
> hash->write (context, &s, sizeof (s));
> @@ -550,40 +600,34 @@ grub_verify_signature_real (char *buf, grub_size_t size,
> r = grub_file_read (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 (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);
> + readbuf = grub_malloc (rem);
> + if (!readbuf)
> + goto fail;
> +
> + r = 0;
> + while (r < rem)
> + {
> + grub_ssize_t rr = grub_file_read (sig, readbuf + r, rem - r);
> + if (rr < 0)
> + goto fail;
> + if (rr == 0)
> + break;
> + r += rr;
> + }
> + if (r != rem)
> + goto fail;
> +
Ditto.
> + if (keyid == 0)
> + keyid = grub_subpacket_keyid_search (readbuf, rem);
> + grub_free (readbuf);
>
> hash->final (context);
>
> + readbuf = grub_zalloc (READBUF_SIZE);
No need to use grub_zalloc here, we did not zero buffer before as well.
> + if (!readbuf)
> + goto fail;
> +
> grub_dprintf ("crypt", "alive\n");
>
> hval = hash->read (context);
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
