From: Tobias Heider <tobias.heider@stusta.de>
Subject: Re: Check permissions of iked psk files
To: Josh Rickmar <jrick@zettaport.com>, tech@openbsd.org
Date: Thu, 25 Apr 2024 14:16:38 +0200

On Thu, Apr 25, 2024 at 06:07:14PM +1000, Jonathan Gray wrote:
> On Sat, Apr 13, 2024 at 02:05:56PM +0200, Tobias Heider wrote:
> > On Fri, Apr 12, 2024 at 07:45:14PM -0400, Josh Rickmar wrote:
> > > On Fri, Apr 12, 2024 at 07:39:58PM -0400, Josh Rickmar wrote:
> > > > The same permission checks performed on /etc/iked.conf (which afaict
> > > > are only done due the possibility of inline preshared key strings)
> > > > should be performed on psk files.
> > > > 
> > > > ok?
> > > > 
> > > 
> > > Reordered to perform the fstat first (although check_file_secrecy also
> > > performs a fstat, this result in a better error message).
> > 
> > I think I'd just drop the fstat() in parsekeyfile(). The error message
> > doesn't make a huge difference.  In any case ok tobhe@
> 
> with fstat removed, sb is used uninitialised multiple times

ok tobhe@

> 
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/sbin/iked/parse.y,v
> diff -u -p -U12 -r1.145 parse.y
> --- parse.y	13 Apr 2024 15:58:10 -0000	1.145
> +++ parse.y	25 Apr 2024 08:03:50 -0000
> @@ -1943,24 +1943,26 @@ parsekey(unsigned char *hexkey, size_t l
>  
>  int
>  parsekeyfile(char *filename, struct iked_auth *auth)
>  {
>  	struct stat	 sb;
>  	int		 fd, ret;
>  	unsigned char	*hex;
>  
>  	if ((fd = open(filename, O_RDONLY)) == -1)
>  		err(1, "open %s", filename);
>  	if (check_file_secrecy(fd, filename) == -1)
>  		exit(1);
> +	if (fstat(fd, &sb) == -1)
> +		err(1, "parsekeyfile: stat %s", filename);
>  	if ((sb.st_size > KEYSIZE_LIMIT) || (sb.st_size == 0))
>  		errx(1, "%s: key too %s", filename, sb.st_size ? "large" :
>  		    "small");
>  	if ((hex = calloc(sb.st_size, sizeof(unsigned char))) == NULL)
>  		err(1, "parsekeyfile: calloc");
>  	if (read(fd, hex, sb.st_size) < sb.st_size)
>  		err(1, "parsekeyfile: read");
>  	close(fd);
>  	ret = parsekey(hex, sb.st_size, auth);
>  	free(hex);
>  	return (ret);
>  }
>