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