vidoas: Address security concerns and improve some behaviour

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.

vidoas

@@ -4,51 +4,104 @@
 # automatically checks it for syntax errors before installing
 # the new copy of doas.conf.
 
-tmpdoas=/tmp/doas.conf
-installeddoas=/usr/local/etc/doas.conf
-defaulteditor=/usr/bin/vi
-doasexec=/usr/local/bin/doas
+set -eu
 
-# Check to make sure we have an editor
-if [ ! -x "$EDITOR" ]
-then
-   echo "No default editor, assuming vi."
-   EDITOR=$defaulteditor
-fi
+PATH=/bin:/usr/bin:/usr/local/bin
+export PATH
 
-if [ ! -x "$EDITOR" ]
+PROG="${0##*/}"
+
+umask 077
+
+WRK_DIR=/var/tmp
+INSTALL_DIR=/usr/local/etc
+
+doas_conf_mode=0644
+
+doas_lock_file="${WRK_DIR}/doas.conf"
+installed_doas="${INSTALL_DIR}/doas.conf"
+
+die()
+{
+    echo "${PROG}: ${@}" 1>&2
+    exit 1
+}
+
+warn()
+{
+    echo "${PROG}: ${@}" 1>&2
+}
+
+get_intr()
+{
+    stty -a \
+    | sed -En '
+	/^(.* )?intr = / {
+	    s///
+	    s/;.*$//
+	    p
+	}
+    '
+}
+
+set_trap_rm()
+{
+    local file file_list
+    file_list=
+    for file
+    do
+	file_list="${file_list} '${file}'"
+    done
+    if [ -n "${file_list}" ]
+    then
+	trap "rm -f ${file_list}" 0 1 2 15
+    fi
+}
+
+tmp_doas="$(mktemp "${WRK_DIR}/doas.conf.XXXXXXXXXX")"
+set_trap_rm "${tmp_doas}"
+
+# Check to see if an existing configuration file is installed.
+if [ -f "${installed_doas}" ]
 then
-  echo "Could not find an editor."
-  exit 1
+    if [ -r "${installed_doas}" ]
+    then
+	cp "${installed_doas}" "${tmp_doas}"
+    else
+	die "Cannot read ${installed_doas}"
+    fi
 fi
 
 # Check to see if existing temporary doas.conf file exists.
-if [ -f "$tmpdoas" ]
+if ln "${tmp_doas}" "${doas_lock_file}"
 then
-   echo "Someone is already working on the doas.conf file."
-   exit 2
+    set_trap_rm "${tmp_doas}" "${doas_lock_file}"
+else
+    die "The doas.conf file is already locked"
 fi
 
-# Check to see if an existing configuration file is installed.
-if [ -f "$installeddoas" ]
-then
-   cp "$installeddoas" "$tmpdoas"
-   $EDITOR "$tmpdoas"
-fi
+"${EDITOR:-vi}" "${tmp_doas}" || true
 
-doas -C "$tmpdoas"
-status=$?
-while [ $status -ge 1 ]
+while ! doas -C "${tmp_doas}"
 do
-   echo "An error was found in the configuration file. Please fix doas.conf."
-   read status
-   $EDITOR "$tmpdoas"
-   doas -C "$tmpdoas"
-   status=$?
+    warn "Press enter to edit doas.conf again to fix it,"
+    warn "or interrupt ($(get_intr)) to cancel."
+    read status
+    "${EDITOR:-vi}" "${tmp_doas}" || true
 done
 
-echo "Parsing check of doas.conf passed. Installing new copy of doas.conf."
-$doasexec cp "$tmpdoas" "$installeddoas"
-rm -f "$tmpdoas"
-exit 0
-
+if [ -s "${tmp_doas}" ]
+then
+    if cmp -s "${tmp_doas}" "${installed_doas}"
+    then
+	warn "No changes made"
+	warn "${installed_doas} unchanged"
+    else
+	doas install -r -m "${doas_conf_mode}" \
+	    "${tmp_doas}" "${installed_doas}" \
+	&& warn "${installed_doas} updated"
+    fi
+else
+    warn "Not installing an empty doas.conf file"
+    warn "${installed_doas} unchanged"
+fi