]> git.vanrenterghem.biz Git - git.ikiwiki.info.git/blob - doc/bugs/ikiwiki-mass-rebuild_has_probably_never_worked_portably.mdwn
Portably and safely dropping privileges is far harder than it ought to be
[git.ikiwiki.info.git] / doc / bugs / ikiwiki-mass-rebuild_has_probably_never_worked_portably.mdwn
1 As best as I can recall, running ikiwiki-mass-rebuild as root has never worked for me on NetBSD or Mac OS X. On both platforms, it gives me a shell as each user in the system wikilist. This is due to non-portable arguments to su(1).
3 The following patch works much better on the aforementioned platforms, as well as CentOS 6:
5     diff --git ikiwiki-mass-rebuild ikiwiki-mass-rebuild
6     index ce4e084e8..2ff33b493 100755
7     --- ikiwiki-mass-rebuild
8     +++ ikiwiki-mass-rebuild
9     @@ -32,7 +32,7 @@ sub processuser {
10         my $user=shift;
11         return if $user=~/^-/ || $users{$user};
12         $users{$user}=1;
13     -   my $ret=system("su", $user, "-s", "/bin/sh", "-c", "--", "$0 --nonglobal @ARGV");
14     +   my $ret=system("su", "-m", $user, "-c", "/bin/sh -c -- '$0 --nonglobal @ARGV'");
15         if ($ret != 0) {
16                 print STDERR "warning: processing for $user failed with code $ret\n";
17         }
19 The `-m` may be overzealous. I have some sites running as users with `/sbin/nologin` for a shell, and this allows running a command as those users, though without some typical environment variables. This is probably wrong. Maybe I should be doing something else to limit shell access for those users, and the su arg should instead be `-`.
21 --[[schmonz]]
23 > To get some real-world and very cross-platform testing, I've committed
24 > a conservative version of this patch, with `-` in place of `-m`, to
25 > pkgsrc's ikiwiki package (rev 3.20180311nb1), and will report back. In
26 > the meanwhile, would this change cause any obvious regressions on
27 > Debian? --[[schmonz]]
29 >> su(1) does several things for us, not all of them completely obvious:
30 >>
31 >> * raise or drop privileges
32 >> * avoid inheriting the controlling tty
33 >> * alter the environment
34 >> * run a PAM stack which can do more or less anything
35 >> * execute the given command
36 >>
37 >> Because it's a privileged program, and POSIX/SUS don't specify the
38 >> behaviour of privileged operations, its behaviour is determined
39 >> by tradition rather than standards.
40 >>
41 >> Dropping privileges (in this case) is uncontroversial: clearly we want
42 >> to do that.
43 >>
44 >> Not inheriting the controlling tty is necessary to prevent tty hijacking
45 >> when dropping privileges (CVE-2011-1408, [[!debbug 628843]]). See
46 >> ikiwiki-mass-rebuild's git history. It might also be possible to do this
47 >> with `POSIX::setsid`, but I don't know whether that fully protects us
48 >> on all platforms, and I would hope that every platform's `su` does the
49 >> right things for that platform.
50 >>
51 >> Altering the environment is less clear. I'm taking the su(1) from Debian
52 >> as a reference because that's what Joey would have developed against,
53 >> and it has several modes for how much it does to the environment:
54 >>
55 >> * with `-m` (or equivalently `-p` or `--preserve-environment`):
56 >>   reset only `PATH` and `IFS`; inherit everything else. I'm fairly
57 >>   sure we don't want this, because we don't want ikiwiki to run with
58 >>   root's `HOME`.
59 >> * without `-m` or `-`: reset `HOME`, `SHELL`, `USER`, `LOGNAME`,
60 >>   `PATH` and `IFS`; inherit everything else.
61 >> * with `-` (or equivalently `-l` or `--login`) but not `-m`:
62 >>   reset `HOME`, etc.; inherit `TERM`, `COLORTERM`, `DISPLAY` and
63 >>   `XAUTHORITY`; clear everything else.
64 >>
65 >> Before Joey switched ikiwiki-mass-rebuild from dropping privileges
66 >> itself to using `su` to fix CVE-2011-1408, it would reset `HOME`,
67 >> inherit `PATH` (!) and clear everything else. Using plain `su`
68 >> without `-` and without clearing the environment is increasingly
69 >> discredited, because it isn't 1980 any more and a lot of programs
70 >> respect environment variables whose correct values are user-specific,
71 >> such as `XDG_RUNTIME_DIR` and `DBUS_SESSION_BUS_ADDRESS`. So I think
72 >> using `su -` would be reasonable and perhaps preferable.
73 >>
74 >> Running the PAM stack is essentially unavoidable when we're
75 >> altering privileges like this, and it's what PAM is there for,
76 >> so we should do it. I think some `su` implementations (although not
77 >> the one in Debian) run different PAM stacks for `su` and `su -`.
78 >>
79 >> Finally, running the command. `su` has two design flaws in this area:
80 >>
81 >> * The command is a string to be parsed by the shell, not an argument
82 >>   vector; on Linux, this design flaw can be avoided by using
83 >>   `runuser -u USER ... -- COMMAND [ARGUMENT...]` from util-linux instead
84 >>   (essentially a non-setuid fork of util-linux su with more reasonable
85 >>   command-line handling), and on many Unix systems it can be avoided by
86 >>   using `sudo -u USER ... -- COMMAND [ARGUMENT...]`, but presumably neither
87 >>   is available as standard on all OSs because that would be far too
88 >>   helpful. runuser is also (still) vulnerable to `TIOCSTI` tty hijacking,
89 >>   because its developers think that ioctl has no legitimate uses and
90 >>   should be disabled or made a privileged operation in the Linux kernel,
91 >>   but the Linux kernel maintainers have rejected that solution and
92 >>   neither seems to be willing to back down.
93 >>
94 >>   We might be able to bypass this with this trick:
95 >>
96 >>       system('su', ..., '--', '-c', 'exec "$0" "$@"', $0, @ARGV);
97 >>
98 >>   using the fact that arguments to a Bourne/POSIX shell after `-c`
99 >>   are set as `$0`, `$1`, ... in the shell. But the second design flaw
100 >>   makes this unreliable.
101 >>
102 >> * `-c` is specified to run the given command with the user's
103 >>   login shell from `/etc/passwd` (which might be `nologin` or `csh`
104 >>   or anything else), not a standardized Bourne/POSIX shell, so you
105 >>   can't predict what (if anything) the given command will actually
106 >>   do, or even how to quote correctly. On Linux, giving `-s /bin/sh`
107 >>   works around this design flaw, but apparently that's not portable
108 >>   or we wouldn't be having this discussion.
109 >>
110 >> In principle ikiwiki-mass-rebuild was already wrong here, becase it
111 >> receives arbitrary arguments and passes them to ikiwiki, but will do
112 >> the wrong thing if they contain shell metacharacters (this is not a
113 >> security vulnerability, because it's the unprivileged shell that will
114 >> do the wrong thing; it's just wrong). Your proposed change makes it
115 >> differently wrong, which I suppose is not *necessarily* worse, but
116 >> I'd prefer it to be actually correct.
117 >>
118 >> It seems that by using `-m` you're relying on root having a
119 >> Bourne-compatible (POSIX) login shell, so that when `SHELL` is
120 >> inherited from root's environment, it will parse the argument of `-c`
121 >> according to `/bin/sh` rules. This is less reliable than Linux
122 >> `su -s /bin/sh` and has more side-effects, but the man page collection
123 >> on unix.com suggests that this meaning for `-s` is Linux-specific
124 >> and has not been copied by any other OSs, which is depressing because
125 >> that option seems to be the only way to achieve what we want.
126 >>
127 >> In conclusion, non-interactive `su` is a disaster area, but we use
128 >> it because traditional Unix terminal handling is also a disaster
129 >> area, and I don't see a good solution.
130 >> --[[smcv]]