On Mon, 13 Nov 2000 20:23:07 +0000 (GMT),
Chris Evans <[email protected]> wrote:
>Either the kernel or modprobe needs to treat the module name with
>_extreme_ distrust, and I see no evidence of that.
Agreed. modprobe was not designed to run suid. Calling modprobe from
request_module has the same effect as running suid. dev_load() can
take the interface name and pass it to modprobe unchanged and modprobe
does not verify its input, it trusts root/kernel.
>Without this distrust, there is a huge amount of code a malicious user can
>play with. Looking at meta_expand() and split_line() in modprobe, there
>are buffer overflows all over the place (lucky an interface name can only
>be 15 characters!). Worse, user-defined input can be passed to sprawling
>interfaces such as glob() and wordexp().
The buffer overflows are not as bad as they look, most of them are on
malloc strings which have calculated sizes. There are some strcat
calls that are suspect and I have fixed those in my tree, and replaced
the system() calls with safe equivalents. But that does not fix the
other problems.
(1) Some user defined input is passed directly through the kernel to
modprobe running as root.
(2) Current modprobe has no way of telling that it was invoked from the
kernel instead of by root so it cannot apply different verification
to its parameters for kmod input.
(3) modprobe applies filename expansion to the user supplied input as
well as the paths from modules.conf. The former is tainted, the
latter is not but they are joined together in modprobe. This is
fine for root, terrible for kmod.
The only secure fix I can see is to add SAFEMODE=1 to modprobe's
environment and change exec_modprobe.
static char * envp[] = { "HOME=/", "TERM=linux", "PATH=/sbin:/usr/sbin:/bin:/usr/bin", "SAFEMODE=1", NULL };
In safemode, modprobe will treat its last argument as a module name,
even if it starts with '-'. Also in safemode, no filename expansion
will be applied to the module name, filename expansion will still be
applied to paths in /etc/modules.conf.
To cater for older kernels (including 2.2), modprobe will treat an
environment of exactly this, and no more
"HOME=/", "TERM=linux", "PATH=/sbin:/usr/sbin:/bin:/usr/bin"
as requiring safemode. Why add SAFEMODE at all? In case we want to
change the environment in future.
Chris Evans raised another point.
(4) Passing user defined input directly through the kernel to modprobe
does more than expose modprobe to suspect input. If a user can
find a program that the kernel trusts and whose input is passed
straight through then they can load any module. Controlled loading
with kernel generated names like net-pf-10, eth0 is fine,
uncontrolled loading of any module name from user input is not.
This is a pure kernel problem.
>The only secure fix I can see is to add SAFEMODE=1 to modprobe's
>environment and change exec_modprobe.
SAFEMODE may mean other things to other programs, so that
an ordinary user might set that environment variable for some
other reason, and then get weird behavior if he or she has occasion
to su to root. In general, you only want to use environment variables
if either it is a user interface issue to keep the commands short
(not an issue here, since nobody is typing in the command that
requrest_module generates) or there is some well established
convention that will handled in a particular way by subordinate
child processes (e.g., PATH=....).
It would be much better to just add a command line option
to modprobe that request_module() would cause it treat the following
argument as the module to load (you do not ever have to force
argument processing to stop at that point, since module will be
fully contained in the next argument, even if it contains space or
linefeed).
Another possible approach would be to create a separate
/sbin/safe_modprobe. modprobe already behaves differently
based on whether argv[0] ends in "modprobe", "insmod", "depmod",
or "rmmod". So this would be in keeping with that convention.
It would also be trivial to retrofit old systems. Just have
some system boot script do:
echo /sbin/safe_modprobe > /proc/sys/kernel/modprobe
The issue of the kernel doing request_module() on arbitrary
strings is not just a security problem. It is also a namespace
collision problem, which this security concern will give us the
opportunity to fix. I have just been glad that no company has
shipped a networking device called, say, "ext2". The non-constant
module names that are loaded by request_module should have names like:
fs-msdos
fs-ext2
netif-eth0
netif-wvlan0
etc.
That way, a malicious user cannot cause a denial of service
by identifying one module with a loading bug (our kernels have 774 modules)
and doing, "ifconfig <modulename>".
The extra work of doing the snprintf() into a buffer
before invoking request_module will resolve the buffer overrun
issues too.
I would be happy to assist in coding this up. The 50 lines of
text that I have written in this email probably only translate into
20 lines of code.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
On Tue, 14 Nov 2000 12:31:41 -0800,
"Adam J. Richter" <[email protected]> wrote:
>>The only secure fix I can see is to add SAFEMODE=1 to modprobe's
>>environment and change exec_modprobe.
>
> SAFEMODE may mean other things to other programs, so that
MOD_SAFEMODE.
> It would be much better to just add a command line option
>to modprobe that request_module() would cause it treat the following
Changing the command line is not an option. Kernel 2.2 still runs with
modutils 2.1.121, changing the request_module command line would break
people using modutils 2.1.121 and force them to upgrade, AC would kill
me. I needed a mechanism that would work with modutils 2.3 but have no
effect on modutils 2.1.121, remember that 2.1.121 does not have this
security exposure. It also had to work on 2.2 kernels because many
people are using moditils 2.3 on 2.2 kernels. SGI ship a 2.2 kernel
with devfs for their big machines, that needs modutils 2.3.
> Another possible approach would be to create a separate
>/sbin/safe_modprobe. modprobe already behaves differently
>based on whether argv[0] ends in "modprobe", "insmod", "depmod",
>or "rmmod". So this would be in keeping with that convention.
>It would also be trivial to retrofit old systems. Just have
>some system boot script do:
>
> echo /sbin/safe_modprobe > /proc/sys/kernel/modprobe
I thought about that but it assumes that users will add that line to
their scripts - not guaranteed. The fix needed a change that would
automatically detect that safe mode was required and not rely on manual
intervention. Especially with 30+ Linux distributions out there.
> request_module has the same effect as running suid. dev_load() can
> take the interface name and pass it to modprobe unchanged and modprobe
> does not verify its input, it trusts root/kernel.
Then dev_load is being called the wrong way. In older kernels we explicitly
only did a dev_load with user passed names providing suser() was true.
Alan
Hello!
> > request_module has the same effect as running suid. dev_load() can
> > take the interface name and pass it to modprobe unchanged and modprobe
> > does not verify its input, it trusts root/kernel.
>
> Then dev_load is being called the wrong way. In older kernels we explicitly
> only did a dev_load with user passed names providing suser() was true.
It checks CAP_SYS_MODULE nowadays.
Which does not look good by the way, it is function of request_module(),
rather than of caller.
Alexey
> It checks CAP_SYS_MODULE nowadays.
>
> Which does not look good by the way, it is function of request_module(),
> rather than of caller.
Only the caller knows if the data is tainted. Thus only the caller can decide
Hello!
> Only the caller knows if the data is tainted. Thus only the caller can decide
Sorry? What data? What to decide?
Device name of &|&|&|&|&|& is absolutely legal, nicely loking name.
dev.c _wants_ to load such device and it is problem of kmod,
if it is not able to make this.
It is the first. And the second: each user is allowed to refer to this device.
And it is problem of module to allow to load corresponding module or not
to allow this.
It means that test for CAP_SYS_MODULE is illegal, moving pure policy
issue to improper place.
Alexey
> It is the first. And the second: each user is allowed to refer to this device.
> And it is problem of module to allow to load corresponding module or not
> to allow this.
Not so.
> It means that test for CAP_SYS_MODULE is illegal, moving pure policy
> issue to improper place.
Definitely not so
What matters is whether the user is requesting a module or the kernel is
choosing to load a module. In the former case where the user can influence the
module name then you need to check CAP_SYS_MODULE in the latter you do not
care.
Alan
Hello!
> > It means that test for CAP_SYS_MODULE is illegal, moving pure policy
> > issue to improper place.
>
> Definitely not so
>
> What matters is whether the user is requesting a module or the kernel is
> choosing to load a module. In the former case where the user can influence the
> module name then you need to check CAP_SYS_MODULE in the latter you do not
> care.
Alan, I honestly peered to this paragraph of text for 10 minutes. 8)8)
It is funny, but I managed to compile it only as:
"dev_load(i.e. you) need not take of care of this".
I.e. exactly the thing which I said. 8)
Alexey
Hi,
as modprobe (insmod) seems to have POSIX args handling, we should perhaps add "--"
to the modprobe cmdline, in order to stop further args processing, and to avoid
mixing a textual argument with an option.
BTW, it should perhaps be generalized.
Xav
--- linux-2.4-test10/kernel/kmod.c Tue Sep 26 01:18:55 2000
+++ linux/kernel/kmod.c Thu Nov 16 19:57:45 2000
@@ -133,7 +133,7 @@
static int exec_modprobe(void * module_name)
{
static char * envp[] = { "HOME=/", "TERM=linux", "PATH=/sbin:/usr/sbin:/bin:/usr/bin", NULL };
- char *argv[] = { modprobe_path, "-s", "-k", (char*)module_name, NULL };
+ char *argv[] = { modprobe_path, "-s", "-k", "--", (char*)module_name, NULL };
int ret;
ret = exec_usermodehelper(modprobe_path, argv, envp);
On Thu, 16 Nov 2000 16:04:23 +0000 (GMT),
Alan Cox <[email protected]> wrote:
>> request_module has the same effect as running suid. dev_load() can
>> take the interface name and pass it to modprobe unchanged and modprobe
>> does not verify its input, it trusts root/kernel.
>
>Then dev_load is being called the wrong way. In older kernels we explicitly
>only did a dev_load with user passed names providing suser() was true.
ping6 -I module_name. ping6 is setuid, it passes the interface name to
the kernel while it holds root privileges, suser() == true. It is
not reasonable to expect setuid programs to know that Linux does
something special with some parameters when no other O/S has that
"feature".
> >Then dev_load is being called the wrong way. In older kernels we explicitly
> >only did a dev_load with user passed names providing suser() was true.
>
> ping6 -I module_name. ping6 is setuid, it passes the interface name to
> the kernel while it holds root privileges, suser() == true. It is
> not reasonable to expect setuid programs to know that Linux does
> something special with some parameters when no other O/S has that
> "feature".
ping6 shouldnt be running with CAP_SYS_MODULE in the first place