search

Duncaen / OpenDoas

A portable fork of the OpenBSD `doas` command
Other
610 stars 35 forks source link

Group ID leak when creating timestamp directory #64

Closed sysvinit closed 3 years ago

sysvinit commented 3 years ago

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

https://github.com/Duncaen/OpenDoas/blob/9a25a6d7b6be3ed4ffb822c5a3fa178057d18329/timestamp.c#L265-L272

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:

https://github.com/Duncaen/OpenDoas/blob/9a25a6d7b6be3ed4ffb822c5a3fa178057d18329/configure#L135

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: https://github.com/Duncaen/OpenDoas/blob/9a25a6d7b6be3ed4ffb822c5a3fa178057d18329/timestamp.c#L224)

Duncaen commented 3 years ago

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. https://github.com/Duncaen/OpenDoas/issues/47#issuecomment-769741401.

sysvinit commented 3 years ago

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.