General thoughts: Just a reminder: this patch tries to fix a BUG in code, which was present from the introduction of this functionality and acknowledged for more than 9 month now. The goal of this patch is to fix it without introducing too much change. We are spending too much time "improving" things and forgetting that basic functionality is broken.
Probably, most of us have experience working in software companies and the basic flow of any software development is: - fix bugs - improve - goto step 1; So fixing bugs and improvements should be in separate steps. I do not think it is an excuse not to fix bugs for 9 month in the hope of improving code readability along the way. That should be a separate task. Unfortunately, I do not have time to continuously rebase the PR for each 1 month delay - GRUB is not my primary and even second responsibility at work. I'm just trying to be nice and contribute back what I found broken. But I'm just a regular user and I have to cut this time from my family to do this. Actually, IMHO, this whole file needs to be redone, so comments regarding "this or that unclear" applies to everything there. As I said multiple times, I almost did not introduce new code, but just refactored the existing one fix the issue. I'm not aware of any decisions made prior to this patch. Many times in this mailing list there were requests to be considerate that GRUB maintainers are very busy and cannot always act/respond quickly. But, guess what: this goes the other way around - GRUB contributors can be busy as well. If you make me rebase the patch each month with new not very relevant comments I will give up essentially. If you think the code needs improvements, you are free to ask me for a follow-up with general directions/requirements for what you think is wrong. But, please, make a little effort to fix the work already done. The patch is not that big, but to prepare it I had to spend time to read and understand the whole PGP RFC (and their data formats and encodings are not that simple I would say). And after 9 month I do not even remember the specifics. General thoughts on improving GRUB development: - let's be tolerant to each other - for God's sake, let's move to a modern code hosting with convenient modern workflows (PRs, reviews etc). Yes, previously you said that you want to be able to review patches on the phone, but I do not imagine you guys having not being able to do this with a reasonable smartphone. Because fighting with git send-mail and subverting my email 2-factor auth just to send a patch makes me (and probably many others) very sad (and insecure BTW) - there should be no comments, like (taken from real-life) "I do not think this is a right name for this function". The right form would be "I do not think this is a right name for this function, how about <PUT YOUR PROPOSAL HERE>". We are not mind-readers and do not have full visibility into GRUB project. - ... Sorry for the noise, but this is something which came up working on this... Specific patch comments below. On Sat, Dec 10, 2016 at 5:59 PM, Andrei Borzenkov <arvidj...@gmail.com> wrote: > 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 This and more: this specific packet can be in the middle of "packet collection", but its length should be >=8. We do not want to potentially read data from subsequent packets (no overflow, but getting invalid data). >> + 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. > I remember I had justification for it, but do not remember what it is, because I wrote this 9 month ago. >> + 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. > 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. > We agreed previously that we should keep "what is correct" (your comment from 30 March 2016). Currently the function allocates the buffer with grub_zalloc (READBUF_SIZE). I changed buffer allocation for my code, but I have no idea about the assumptions of the code which follows. So, if previously, that code used buffer allocalted with grub_zalloc I do the same for compatibility. >> + 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
