From dfccc06a4a3c812b2f8cc0409ee9290d7a24d67b Mon Sep 17 00:00:00 2001 From: deraadt Date: Fri, 3 Aug 2018 16:02:53 +0000 Subject: [PATCH] unveil _PATH_UTMP at startup. Time for a commentary: There is a TOCTOU between unveil() and open() which should always be considered, since a path is being supplied twice to the kernel. First unveil()s define which paths remain in scope, then secondly open()s try to access paths in scope. The unveil() generates a vnode reservation against the final path resolution (including symbolic link collapse). Before the open() occurs, root could replace the path with symbolic traversal pointing elsewhere. Then open() will traverse a path which fails to discover the reserved vnode, and thus fail with ENOENT. The TOCTOU sequence doesn't succeed against the new path, it *always fails*. (Unless the symlink resolves to another unveil'd vnode object, but that is not new behaviour). So once a process is running with veiled filesystem view, we can consider such a symlink change action as PERMANENTLY visible to this process and correctly contained to the scoped view, rather than the previous behaviour of being TRANSIENT and global in view. So this is not a real race, security implications will be narrow, and generally the old symlink-race case is the less secure. When we add this unveil+open TOCTOU scenario to a program, we should consider who can perform such a symlink snap, and whether behaviour change to the program is more disruptive than the risks prevented through filesystem hiding. How does a program behave if a file disappears due to active interference? Are users (and scripts) used to operating in a racey best-effort way, and is the additional strictness strangling their freedom to run shitty stuff? A few general rules for base programs can avoid problems in this area: don't en masse unveil argv[], then process argv[] in a second phase. Don't unveil args which get placed into TZ, TERM, and some other environment variables, unless you completely understand what libc is doing. --- usr.bin/users/users.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/usr.bin/users/users.c b/usr.bin/users/users.c index 8a8d13828de..07d43771a3a 100644 --- a/usr.bin/users/users.c +++ b/usr.bin/users/users.c @@ -1,4 +1,4 @@ -/* $OpenBSD: users.c,v 1.13 2015/10/09 01:37:09 deraadt Exp $ */ +/* $OpenBSD: users.c,v 1.14 2018/08/03 16:02:53 deraadt Exp $ */ /* $NetBSD: users.c,v 1.5 1994/12/20 15:58:19 jtc Exp $ */ /* @@ -53,6 +53,8 @@ main(int argc, char *argv[]) struct utmp utmp; int ch; + if (unveil(_PATH_UTMP, "r") == -1) + err(1, "unveil"); if (pledge("stdio rpath", NULL) == -1) err(1, "pledge"); -- 2.20.1