On Tue, Oct 27, 2026 at 07:56:40PM +0100, Omar Polo wrote:
> Bonjour :)
> 
> Marc Espie <marc.espie.openbsd@gmail.com> 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 <err.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > +#include <stdbool.h>
> 
> 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 <unistd.h>
> >  #include <sha2.h>
> >  #include <string.h>
> > @@ -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. */

?