From: Marc Espie Subject: Re: 1st patch: make signify -z more stringent To: Omar Polo Cc: tech@openbsd.org, tb@openbsd.org Date: Tue, 27 Oct 2026 20:14:14 +0100 On Tue, Oct 27, 2026 at 07:56:40PM +0100, Omar Polo wrote: > Bonjour :) > > Marc Espie wrote: > > I know that SHA512/256 is not (yet ?) vulnerable to size extension attacks, > > but just in case, it feels better to actually advertize the full size > > of the file, so that the last block will get check as well. > > > > There is a necessary transition period, obviously... > > > > Here's the patch that will check an extended header that also records > > the size of the last block, and error out if that doesn't match. > > > > > > Once that patch is fully deployed, the next patch would print > > the lastblocksize, and the 3rd patch would remove the conditional "checklast" > > and assume lastblocksize is always there. > > > > Of course, there is no currently known extension attacks on SHA512/256, but > > why risk it ? > > > > In case an extension attack is discovered, let me remind you that the package > > format is a MULTI_STREAM gzip (e.g., several gzips concatenated). > > > > It would be trivial for an attacker to append an "empty" gzip and use the > > comment field to put some kind of malicious payload to obtain the correct > > checksum (if there was an extension attack). > > > > I've also sent a small pkg_add patch, a few weeks ago, that would set > > Strict => 1 in the IO::Uncompress::Gunzip parameters, thus forbidding > > gzip headers to contain anything but ascii. > > > > I know all this is about non existent attacks for now, but come on. > > > > We can be prepared. > > make sense to me; just one nit and one question inline > > > Index: zsig.c > > =================================================================== > > RCS file: /vide/cvs/src/usr.bin/signify/zsig.c,v > > diff -u -p -r1.20 zsig.c > > --- zsig.c 20 May 2025 01:03:56 -0000 1.20 > > +++ zsig.c 30 May 2026 09:04:22 -0000 > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > this is the nit: we don't seem to use much stdbool in the codebase, i > don't know how much is cargo culting, personal preference, copy-pasting > etc, but i'd still stick to a plain int since signify doesn't seem to > use bare ints otherwise. Doesn't matter at all, since this part will eventually go away in a few weeks/months. I tend to prefer using bool for clarity of reading on new code. That's just personal preference. I'm fully aware there's zero functional difference. > > #include > > #include > > #include > > @@ -131,7 +132,7 @@ readgz_header(struct gzheader *h, int fd > > > > static void > > copy_blocks(int fdout, int fdin, const char *sha, const char *endsha, > > - size_t bufsize, uint8_t *bufend) > > + size_t bufsize, uint8_t *bufend, bool checklast, size_t lastblocksize) > > { > > uint8_t *buffer; > > uint8_t *residual; > > @@ -160,6 +161,14 @@ copy_blocks(int fdout, int fdin, const c > > if (more == 0) > > break; > > } > > + if (n != bufsize) { > > + if (endsha - sha > SHA512_256_DIGEST_STRING_LENGTH) > > + errx(1, "block has the wrong length " > > + "(%zu vs %zu)", n, bufsize); > > this took me a while to fully wrap my head around. why not > strengthening the check that there's a bit below: > > : if (endsha - sha < SHA512_256_DIGEST_STRING_LENGTH-1) > : errx(4, "signature truncated"); > > into a endsha - sha != SHA512_256_DIGEST_STRING_LENGTH ? > > that will also simplify this block to just one if (checklast && n != ...) I find the current code simpler to understand actually: if we end up with n != bufsize, it means it *has* to be the last block, so if endsha - sha > SHA521_... there is definitely a problem. (and the checklast test is a bit more explicit about it) I fail to see how your change makes it clearer. Maybe add an extra comment instead, like /* In that case, this *has* to be the lastblock, so if we * have more than one checksum to verify, we're in trouble. */ ?