Group ID leak when creating timestamp directory

[sysvinit]

When timestamp files are enabled, doas will by default attempt to create the timestamp directory if it doesn't already exist:

OpenDoas/timestamp.c

Lines 265 to 272 in 9a25a6d

	if (stat(TIMESTAMP_DIR, &st) == -1) {
	if (errno != ENOENT)
	return -1;
	if (mkdir(TIMESTAMP_DIR, 0700) == -1)
	return -1;
	} else if (st.st_uid != 0 || st.st_mode != (S_IFDIR | 0700)) {
	return -1;
	}

The build files install doas with mode 4755, i.e. setuid root, so that the effective user ID is set to root when the binary is executed:

OpenDoas/configure

Line 135 in 9a25a6d

: ${BINMODE:=4755}

This means, however, that the process's real and effective group ID are set to that of the invoking user when doas is run. When the timestamp directory is then created, it is owned by the invoking user's primary group:

molly on flywheel ~/s/c/opendoas> # tested on Debian 10, with timestamps enabled and PAM disabled
molly on flywheel ~/s/c/opendoas> sudo chown root:root doas
molly on flywheel ~/s/c/opendoas> sudo chmod 4755 doas
molly on flywheel ~/s/c/opendoas> sudo rm -rf /run/doas
molly on flywheel ~/s/c/opendoas> ./doas true  # doas.conf contains "permit persist molly"
doas (molly@flywheel) password: 
molly on flywheel ~/s/c/opendoas> ls -ld /run/doas
drwx------ 2 root molly 60 Mar 27 11:29 /run/doas

This doesn't appear to be a big problem at first glance, though it may be unexpected behaviour, given that it discloses the group of the user who ran doas when the timestamp directory was created.

(Additionally, the timestamp files themselves inherit the invoking user's group ID. This appears to be intentional (though I'm not immediately sure why), as there is an explicit check against the process's group ID here:

OpenDoas/timestamp.c

Line 224 in 9a25a6d

if (st.st_uid != 0 || st.st_gid != getgid() || st.st_mode != (S_IFREG | 0000))

)

[Duncaen]

This was done to avoid the extra chown, but there is no real reason to do that as long as there is no toctu issue between mkdir and chown.
#47 (comment).

[sysvinit]

Ah, I hadn't seen that earlier issue, apologies. It does appear to be mostly a cosmetic thing, given the checks in the rest of the code, but I'll leave the discussions for the other issue.