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 19:56:40 +0100

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.

>  #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 != ...)

> +			else if (checklast && n != lastblocksize)
> +				errx(1, "block has the wrong length "
> +				    "(%zu vs %zu)", n, lastblocksize);
> +		}
>  		if (n == 0)
>  			break;
>  		SHA512_256Data(buffer, n, output);
> @@ -188,6 +197,8 @@ zverify(const char *pubkeyfile, const ch
>  	char *p;
>  	uint8_t *bufend;
>  	int fdin, fdout;
> +	bool checklast;
> +	size_t lastblocksize;
>  
>  	/* by default, verification will love pipes */
>  	if (!sigfile)
> @@ -207,6 +218,7 @@ zverify(const char *pubkeyfile, const ch
>  	    pubkeyfile, keytype);
>  
>  	bufsize = MYBUFSIZE;
> +	checklast = false;
>  
>  #define BEGINS_WITH(x, y) memcmp((x), (y), sizeof(y)-1) == 0
>  
> @@ -216,6 +228,11 @@ zverify(const char *pubkeyfile, const ch
>  	    sscanf(p, "blocksize=%zu\n", &bufsize) > 0) {
>  		while (*(p++) != '\n')
>  			continue;
> +		if (sscanf(p, "lastblocksize=%zu\n", &lastblocksize) > 0) {
> +			checklast = true;
> +			while (*(p++) != '\n')
> +				continue;
> +		}
>  	}
>  
>  	if (*p != '\n')
> @@ -225,7 +242,8 @@ zverify(const char *pubkeyfile, const ch
>  	writeall(fdout, fake, sizeof fake, msgfile);
>  	writeall(fdout, h.comment, len+1, msgfile);
>  	*(p++) = 0;
> -	copy_blocks(fdout, fdin, p, h.endcomment, bufsize, bufend);
> +	copy_blocks(fdout, fdin, p, h.endcomment, bufsize, bufend, checklast,
> +	    lastblocksize);
>  	free(h.buffer);
>  	close(fdout);
>  	close(fdin);