vidoas: Address security concerns and improve some behaviour

[suominen]

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.

[suominen]

I should have added another XXX about ln(1) not being portable, either: On some systems "ln" behaves like "ln -f" by default (I'm thinking Solaris, so possibly Illumos; I don't have ready access to either, so I cannot check). It is imperative that ln does not try to remove the lock file, but instead fails if the file already exists.

[slicer69]

I looked over the changes and I like what you are doing. This approach looks much more robust and probably more secure, especially on systems that have multiple people with admin access.

My one concern here is that the patched vidoas script does not appear to be portable. In particular, I don't think it will run on FreeBSD or on GNU/Linux platforms. I believe the key issues here are the lack of "--" flag in doas commands which run programs that have parameters. For example "doas install -r -m" will fail on some platforms while "doas -- install -r -m" should work everywhere.

I also noticed the "install" command which is used in a few places has parameters which are not documented on Linux and FreeBSD so I think those will fail. The "-r" flag, for example, is missing from "install" on these platforms.

Would you mind updating the pull request and re-submitting? I'll give it another test run then and merge it if everything works on those two platforms.

[suominen]

The need to use -- sounds like a bug in getopt(3). Which platform has this issue?

I see that the code in this version of doas has been modified by unconditionally prepending + to optstring in the getopt() call. This is a GNU extension in glibc and the code should really account for that. However, this should mean that even with glibc the use of -- is unnecessary. That's why I think it would be better to track down the misbehaving getopt(3) implementation instead of cargo culting a workaround. Also, I think it is dangerous if doas(1) doesn't behave the same on all platforms. I think any port should preserve the behaviour closely.

I suggest you read through the first comment in this pull request (it is also the commit log entry for the change): it highlights two other portability issues in addition to the -r flag of install(1). Unfortunately I don't see any easy shell script replacement for install -r that doesn't come with a race condition of its own. (On FreeBSD and OpenBSD one would use install -S, of course.)

I still think the best route to implement vidoas(8) is to write it in C, using e.g. vipw(8) as a starting point.

[slicer69]

The issue with the double-dash (--) in front of commands was a GNU issue. Recent versions of doas do fix the issue. I'm not sure if the behaviour is different on any other platforms. My concern with the script wasn't due to current behaviour of doas, just compatibility with previous versions running on GNU userland.

Follow up on your related thought, as far as I know, this port of doas does now work the same across all platforms. The prefix -- is just for backward compatibility.

[slicer69]

I have merged in your improved vidoas script with minor change of taking out the "-r" flag for the install call as this flag, as you noted, isn't portable outside of NetBSD. It can be patched back in as needed by packagers.

[suominen]

Please be aware that the default behaviour of install(1) is to truncate and overwrite the existing file. This means there is a window of time when doas(1) will read a partial configuration file and act on it.

The correct solution is to make the new configuration available using an atomic operation such as rename(2). This is what install -r (or install -S) achieves. Using mv(1) is possible but tricky, as you need to make sure the source and target are in the same file system so that a rename(2) call can succeed. Otherwise mv(1) will fall back to being no different from cp(1) or install(1) without the -r (or -S) option and thus creating the same opportunity for doas(1) acting on a partially written configuration file.

[slicer69]

Having the doas.conf file truncated is okay because with no matching rules doas will reject authentication. A partial file will cause doas to correctly fail if someone tries to authenticate while the doas.conf file is being updated.

[suominen]

A partial file will cause doas to correctly fail if someone tries to authenticate while the doas.conf file is being updated.

I don't think that holds true.

The last matching rule determines the action taken. This means that a matching permit rule can be further restricted by a following deny rule. Consider the following configuration:

permit alice
deny alice as root

If doas(1) reads a partial configuration file with the permit already in it but the deny not yet written, the results are not what was intended.

Each rule line also has optional elements. For example, the as target part is optional and defaults to "all users." So if the intended configuration is permit alice as bob but doas(1) reads a partial configuration of permit alice then Alice would be allowed to run as any user (including root).

[slicer69]

This seems pretty unlikely to happen. I mean we're talking about a situation where a file isn't just truncated (ie zero bytes), but is partially written to the disk when it is read, and full lines with valid syntax are written, and the operating system is not caching the file it just wrote to the temporary location. And a malicious user is in the doas.conf file with permission to switch to any other user (except root), and knows exactly when the write happens.

This configuration shouldn't be used in the first place as it's poorly thought out, but even if it is, this is still a highly unlikely scenario,

At this point using vidoas is still at least as safe, if not safer, than using an editor like vi on the doas.conf file directly because that would face the same issue.