From: "Omar Polo" <op@omarpolo.com>
Subject: Re: 1st patch: make signify -z more stringent
To: Marc Espie <marc.espie.openbsd@gmail.com>
Cc: tech@openbsd.org, tb@openbsd.org
Date: Tue, 27 Oct 2026 20:25:14 +0100

Marc Espie <marc.espie.openbsd@gmail.com> wrote:
> On Tue, Oct 27, 2026 at 07:56:40PM +0100, Omar Polo wrote:
> > Bonjour :)
> > 
> > Marc Espie <marc.espie.openbsd@gmail.com> wrote:
> > > [...]
> > > 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.

sure.

> > >  #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.

to be fair, i had to do a bit of back-and-forward to understand that,
because we're first ensuring that endsha - sha > digest_string_len and
then that endsha - sha < digest_string_len - 1; so i spent some time
trying to understand when we could enter in one of the two cases and not
the other.  just one != could be simpler, assuming i'm not
misunderstanding this.

> (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. */
> 
> ?

maybe wouldn't hurt.

in any case, none of this was meant to be a blocker for the diff =)


Thanks,
Omar Polo