From: Jonathan Gray <jsg@jsg.id.au>
Subject: Re: Check permissions of iked psk files
To: Tobias Heider <tobias.heider@stusta.de>
Cc: Josh Rickmar <jrick@zettaport.com>, tech@openbsd.org
Date: Thu, 25 Apr 2024 18:07:14 +1000

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

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