Index | Thread | Search

From:
Marc Espie <marc.espie.openbsd@gmail.com>
Subject:
1st patch: make signify -z more stringent
To:
tech@openbsd.org
Cc:
tb@openbsd.org
Date:
Tue, 27 Oct 2026 13:12:42 +0100

Download raw body.

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


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>
 #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);
+			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);