From: Todd C. Miller <millert@openbsd.org>
Subject: Re: xinstall: do not overrun filename buffer
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: tech@openbsd.org
Date: Wed, 16 Oct 2024 13:30:16 -0600

On Wed, 16 Oct 2024 21:15:24 +0200, Christian Weisgerber wrote:

> There was a weird failure during a ports build:
>
> install: /usr/obj/ports/unknown-horizons-2019.1/fake-amd64/usr/local/share/un
> known-horizons/content/gfx/atlas//INS@FkRVpXXcoW: Bad address
>
> I was going to write it off as a spurious failure, but tb@ took a look
> at xinstall and noticed that the wrong buffer length was passed to
> strlcat().  Or rather, the conversion to strl*() accidentally used
> the wrong variable.

We should probably check the return value of strlcpy() and strlcat()
too.  Something like this:

Index: usr.bin/xinstall/xinstall.c
===================================================================
RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v
diff -u -p -u -r1.77 xinstall.c
--- usr.bin/xinstall/xinstall.c	4 Dec 2022 23:50:50 -0000	1.77
+++ usr.bin/xinstall/xinstall.c	16 Oct 2024 19:28:46 -0000
@@ -621,13 +621,19 @@ create_tempfile(char *path, char *temp, 
 {
 	char *p;
 
-	strlcpy(temp, path, tsize);
+	if (strlcpy(temp, path, tsize) >= tsize) {
+		errno = ENAMETOOLONG;
+		return(-1);
+	}
 	if ((p = strrchr(temp, '/')) != NULL)
 		p++;
 	else
 		p = temp;
 	*p = '\0';
-	strlcat(p, "INS@XXXXXXXXXX", tsize);
+	if (strlcat(temp, "INS@XXXXXXXXXX", tsize) >= tsize) {
+		errno = ENAMETOOLONG;
+		return(-1);
+	}
 
 	return(mkstemp(temp));
 }