From: Theo Buehler <tb@theobuehler.org>
Subject: Re: xinstall: do not overrun filename buffer
To: "Todd C. Miller" <millert@openbsd.org>
Cc: Christian Weisgerber <naddy@mips.inka.de>, tech@openbsd.org
Date: Wed, 16 Oct 2024 21:45:19 +0200

On Wed, Oct 16, 2024 at 01:30:16PM -0600, Todd C. Miller wrote:
> 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:

Makes sense. ok tb

> 
> 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));
>  }
>