I'm not claiming this script is now safe. It would certainly benefit
from additional review. I do think (and hope) that I did not make things
worse, at least.
It might be better to look at vipw(8) or visudo(8), which both are
written in C, for prior art on how to do this kind of thing securely.
Security changes:
- Exit on errors and if referencing unset variables.
- Set PATH so that we don't run unintended commands from the PATH that
is in the caller's environment.
- Set umask to prevent other users from having write access to the
temporary files.
- Use /var/tmp instead of /tmp, as /tmp is not shared between users on
all systems. (So trying to install a file from /tmp as root would not
find the file, if the user running vidoas is not root.)
XXX: Using /var/tmp does not guarantee this either, but is more likely
to work.
- Create a temporary file for editing and use ln(1) to acquire the lock.
This addresses a race condition between checking for the lock file and
creating it.
- Use "install -r" to avoid a truncated doas.conf from existing as would
happen with cp (or install without the "-r" option).
XXX: "install -r" is not portable.
- Use "install -m" to set the mode of the installed doas.conf file.
Changes to user experience:
- Don't check for executability of ${EDITOR} as it is not required to be
an absolute path to the executable.
- Don't install an unchanged doas.conf file.
- Don't install an empty doas.conf file.
- The above two checks result in a no-op in the case that ${EDITOR}
could not be run.
- Present the user with a choice of fixing errors or canceling changes.
- Output diagnostic messages to stderr (just like other tools do, e.g.
doas, ln, and cp).
TODO:
- Avoid using hard-coded paths (/usr/local/bin and /usr/local/etc).
They should be replaced with @PREFIX@/bin and @SYSCONFDIR@ before
installing.
Calling setusercontext(3) makes per-user temporary storage work (see
per_user_tmp in security(7) and rc.conf(5)).
May as well also use reallocarray(3) from libc instead of the bundled
compat code.
version of the doas.conf file. Then allows the user to edit it.
The new configuration file is checked for syntax and then, if it passes,
is installed on the system. If the syntax check fails the user is asked
to fix any errors.
repeated calls to getpwuid() can over-write the original struct passwd
strucuture. This can lead to the original user's environment data
being overwritten by the target user's, even when "keepenv" is
specified in the doas.conf file.
We now do a deep copy of the original and target users' struct passwd
information to avoid over-writting the original on platforms where libc
uses a static area for all calls.
- Adjust the Makefile and the README for macOS / Darwin specific build instructions
- Add bsd-closefrom.c as a more portable version of closefrom(2), which was
obtained from the portable version of OpenSSH 8.1
- amalleo25
Provided cleaner fix for crash when user/command has
no valid match in the doas.conf file.
- amalleo25
Removed option to match UID with -u flag. Provided
usernames must now match a username, not UID. This was
ambigious if a user had a numeric username.
- Jesse
Added flag to display all warnings during compiling.
Added status checks when parsing user/group IDs for Linux.
Make sure Linux drops original user's groups when running as another user.
Seeing this being used on even more system like Illumos with this ugly
and security critical bug open makes me cringe every time I check if it
was finally fixed.
I reported it directly to the maintainer in 2017. I reported it to
pkgsrc-security@netbsd.org without a response.
and PATH from the original user to the target user. This could cause
files in the wrogn path or home directory to be read (or written to),
which resulted in potential security problems.
This has been changed so that only DISPLAY and TERM are passed to the
new environment. This is fine for running command line programs. When
GUI programs need to be run, "keepenv" can be added to the user's
doas.conf entry. This results in variables like HOME being copied
to the target user, allowing GUI programs to run.
Many thanks to Sander Bos for reporting this issue and explaining
how it can be exploited.
This commit also adds the ability to pass a customized PATH to
target users. The new PATH can be set at compile time in the
Makefile. The default path is provided in the Makefile and commented
out.
