From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1cFlxK-0003Nn-Gk for mharc-grub-devel@gnu.org; Sat, 10 Dec 2016 13:01:06 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cFlxF-0003Mb-K7 for grub-devel@gnu.org; Sat, 10 Dec 2016 13:01:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cFlxB-0005gJ-UB for grub-devel@gnu.org; Sat, 10 Dec 2016 13:01:01 -0500 Received: from mail-lf0-f66.google.com ([209.85.215.66]:35405) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cFlxB-0005fz-Ig for grub-devel@gnu.org; Sat, 10 Dec 2016 13:00:57 -0500 Received: by mail-lf0-f66.google.com with SMTP id p100so2673212lfg.2 for ; Sat, 10 Dec 2016 10:00:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=juCIrxgT5sDEuymdZtBHDISV4QZEQfQ85Ue4O7/z5aM=; b=wtoUH9vOJqVqPR9hvcpt9xEyfZzidbW09hKByV19m/mBvP0ZFpZQmI3avYcdj0zIkf /R535jknKKEKdVhqrJ4mubNdGLGsH4I7RTUeo8Cx395iC0efeEa/fL5jErvefDB6BH8t SKfMRfm6gFlpTkHCKT4+bjLgt7c0dGqVrifbE8cB5nbf9TJiqnkJjoW6IneujOxM+lNC GLIDPV9abxwEjeKInVOtGh4evCdiTA/syhvTcZWX5Ekat8YRUcBekHJkL469Ri//8bBG ry9jo+52lCYc4GANpSuUy9NOjdzESdX+GgcczB3EtdQ/MY0TmgZABm1TbQ9i7QVsWbvM 534Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=juCIrxgT5sDEuymdZtBHDISV4QZEQfQ85Ue4O7/z5aM=; b=D1ZJ9qEBAEyiun+bG5WUkQ0jgMkIRSrtr0jrfkUNB4BlrlTMgyZsUpThxxYb648/Di Sfa9/yhmdruqDnzYXLgvm2iIPzxIcRQPISXsbEIINk5lhMaKXet7HLeNSI5bXaQuesxy QSo7SoWM0wDowLw/C9jsWmXIaqrGT+E/3r0H0tR2iLBrwx8Z4SKKTtEOqDw9Ph179eUp mYoikubftoyjeRgbW5m0WJJxSd1mFOwn+x62vzoastnqXvT4PA1vgfUZQlcktQi2Kg1C N+bZHEzaFeZVwHksBhEJzJC7Wqrj407qq7DFLgjP0fy7kgr+jJa2VfIzL5LnC4c/okiQ c2bA== X-Gm-Message-State: AKaTC01hyIovjDe+LHNvp6IGcuNrwLjRcD3TD9vt40frMOOvqNY3sMFJV6tfX38788aonw== X-Received: by 10.25.234.214 with SMTP id y83mr28403896lfi.153.1481392795981; Sat, 10 Dec 2016 09:59:55 -0800 (PST) Received: from [192.168.1.44] (ppp109-252-90-110.pppoe.spdop.ru. [109.252.90.110]) by smtp.gmail.com with ESMTPSA id q19sm7574703lfi.1.2016.12.10.09.59.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 10 Dec 2016 09:59:55 -0800 (PST) Subject: Re: [PATCH v2] verify: search keyid in hashed signature subpackets To: The development of GNU GRUB References: <1480697919-23371-1-git-send-email-ignat@cloudflare.com> Cc: Daniel Kiper , Ignat Korchagin From: Andrei Borzenkov Message-ID: Date: Sat, 10 Dec 2016 20:59:54 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1480697919-23371-1-git-send-email-ignat@cloudflare.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.85.215.66 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Dec 2016 18:01:05 -0000 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); >