Hi netdev folks,
I'd like to discuss the ability to load any modules from /lib/modules/
by a process with CAP_NET_ADMIN. Since Linux 2.6.32 [1] there is such
possibility:
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: fffffffc00001000
CapEff: fffffffc00001000
CapBnd: fffffffc00001000
root@albatros:~# lsmod | grep xfs
root@albatros:~# ifconfig xfs
xfs: error fetching interface information: Device not found
root@albatros:~# lsmod | grep xfs
xfs 767011 0
exportfs 4226 2 xfs,nfsd
Ability of CAP_NET_ADMIN to load the driver to work with a particular
network device is rational; however, one may load any module not even
related to network this way. Hopefully, this is not equal to
CAP_SYS_MODULE since the module set is restricted to /lib/modules
(additionally may be disabled with /proc/sys/kernel/modules_disabled),
but the idea of non-netdev module loading is weird.
My proposal is changing request_module("%s", name) to something like
request_module("netdev-%s", name) inside of dev_load() and adding
aliases to related drivers. This would allow to load only netdev
modules via these ioctls. I'm not sure what modules should be patches -
at least real physical netdevices have names different from drivers'
names, so they don't need patching. I suppose the list is not big.
Any comments are welcome.
[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Thu, 2011-02-24 at 18:12 +0300, Vasiliy Kulikov wrote:
> Hi netdev folks,
>
> I'd like to discuss the ability to load any modules from /lib/modules/
> by a process with CAP_NET_ADMIN. Since Linux 2.6.32 [1] there is such
> possibility:
>
> root@albatros:~# grep Cap /proc/$$/status
> CapInh: 0000000000000000
> CapPrm: fffffffc00001000
> CapEff: fffffffc00001000
> CapBnd: fffffffc00001000
> root@albatros:~# lsmod | grep xfs
> root@albatros:~# ifconfig xfs
> xfs: error fetching interface information: Device not found
> root@albatros:~# lsmod | grep xfs
> xfs 767011 0
> exportfs 4226 2 xfs,nfsd
Eek!
> Ability of CAP_NET_ADMIN to load the driver to work with a particular
> network device is rational; however, one may load any module not even
> related to network this way. Hopefully, this is not equal to
> CAP_SYS_MODULE since the module set is restricted to /lib/modules
> (additionally may be disabled with /proc/sys/kernel/modules_disabled),
> but the idea of non-netdev module loading is weird.
>
> My proposal is changing request_module("%s", name) to something like
> request_module("netdev-%s", name) inside of dev_load() and adding
> aliases to related drivers.
AFAIK these interface-name aliases are usually defined by distribution
configuration files rather than within the modules themselves. And that
behaviour is pretty much obsolete now that we have hotplug and udev.
> This would allow to load only netdev
> modules via these ioctls. I'm not sure what modules should be patches -
> at least real physical netdevices have names different from drivers'
> names, so they don't need patching. I suppose the list is not big.
The only modules I can see that declare aliases like this are:
net/ipv4/ip_gre.c:MODULE_ALIAS("gre0");
net/ipv4/ipip.c:MODULE_ALIAS("tunl0");
net/ipv6/sit.c:MODULE_ALIAS("sit0");
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
On Thu, Feb 24, 2011 at 16:34 +0000, Ben Hutchings wrote:
> On Thu, 2011-02-24 at 18:12 +0300, Vasiliy Kulikov wrote:
> > My proposal is changing request_module("%s", name) to something like
> > request_module("netdev-%s", name) inside of dev_load() and adding
> > aliases to related drivers.
>
> AFAIK these interface-name aliases are usually defined by distribution
> configuration files rather than within the modules themselves. And that
> behaviour is pretty much obsolete now that we have hotplug and udev.
>
> > This would allow to load only netdev
> > modules via these ioctls. I'm not sure what modules should be patches -
> > at least real physical netdevices have names different from drivers'
> > names, so they don't need patching. I suppose the list is not big.
>
> The only modules I can see that declare aliases like this are:
>
> net/ipv4/ip_gre.c:MODULE_ALIAS("gre0");
> net/ipv4/ipip.c:MODULE_ALIAS("tunl0");
> net/ipv6/sit.c:MODULE_ALIAS("sit0");
Thank you! These are not handled via udev or anything, it is usefull to
load it via "ifconfig sit0 up", so they do require patching.
I didn't find any modules requiring autoloading except these three.
Good candidate should be "isatap0" as it is seen in ip help, but there
is no such alias.
$ ip tunnel add help 2>&1 | grep mode
[ mode { ipip | gre | sit | isatap } ] [ remote ADDR ] [ local ADDR ]
I'll send the patch for request_module() and three drivers.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
anybody load any module not related to networking.
This patch restricts an ability of autoloading modules to netdev modules
with explicit aliases. Currently there are only three users of the
feature: ipip, ip_gre and sit.
Before the patch:
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: fffffffc00001000
CapEff: fffffffc00001000
CapBnd: fffffffc00001000
root@albatros:~# lsmod | grep xfs
root@albatros:~# ifconfig xfs
xfs: error fetching interface information: Device not found
root@albatros:~# lsmod | grep xfs
xfs 767011 0
exportfs 4226 2 xfs,nfsd
After:
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: ffffffffffffffff
CapEff: ffffffffffffffff
CapBnd: ffffffffffffffff
root@albatros:~# lsmod | grep scsi_wait_scan
root@albatros:~# ifconfig scsi_wait_scan
scsi_wait_scan: error fetching interface information: Device not found
root@albatros:~# lsmod | grep scsi_wait_scan
root@albatros:~# modprobe scsi_wait_scan
root@albatros:~# lsmod | grep scsi_wait_scan
scsi_wait_scan 829 0
Reference: http://www.openwall.com/lists/oss-security/2011/02/24/17
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
include/linux/netdevice.h | 3 +++
net/core/dev.c | 2 +-
net/ipv4/ip_gre.c | 2 +-
net/ipv4/ipip.c | 2 +-
net/ipv6/sit.c | 2 +-
5 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d971346..71caf7a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2392,6 +2392,9 @@ extern int netdev_notice(const struct net_device *dev, const char *format, ...)
extern int netdev_info(const struct net_device *dev, const char *format, ...)
__attribute__ ((format (printf, 2, 3)));
+#define MODULE_ALIAS_NETDEV(device) \
+ MODULE_ALIAS("netdev-" device)
+
#if defined(DEBUG)
#define netdev_dbg(__dev, format, args...) \
netdev_printk(KERN_DEBUG, __dev, format, ##args)
diff --git a/net/core/dev.c b/net/core/dev.c
index 8ae6631..79b33d9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1120,7 +1120,7 @@ void dev_load(struct net *net, const char *name)
rcu_read_unlock();
if (!dev && capable(CAP_NET_ADMIN))
- request_module("%s", name);
+ request_module("netdev-%s", name);
}
EXPORT_SYMBOL(dev_load);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 6613edf..d1d0e2c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
MODULE_LICENSE("GPL");
MODULE_ALIAS_RTNL_LINK("gre");
MODULE_ALIAS_RTNL_LINK("gretap");
-MODULE_ALIAS("gre0");
+MODULE_ALIAS_NETDEV("gre0");
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 988f52f..a5f58e7 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -913,4 +913,4 @@ static void __exit ipip_fini(void)
module_init(ipip_init);
module_exit(ipip_fini);
MODULE_LICENSE("GPL");
-MODULE_ALIAS("tunl0");
+MODULE_ALIAS_NETDEV("tunl0");
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 8ce38f1..d2c16e1 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1290,4 +1290,4 @@ static int __init sit_init(void)
module_init(sit_init);
module_exit(sit_cleanup);
MODULE_LICENSE("GPL");
-MODULE_ALIAS("sit0");
+MODULE_ALIAS_NETDEV("sit0");
--
1.7.0.4
25.02.2011 15:30, Vasiliy Kulikov wrote:
> On Thu, Feb 24, 2011 at 16:34 +0000, Ben Hutchings wrote:
>> On Thu, 2011-02-24 at 18:12 +0300, Vasiliy Kulikov wrote:
>>> My proposal is changing request_module("%s", name) to something like
>>> request_module("netdev-%s", name) inside of dev_load() and adding
>>> aliases to related drivers.
It is not the kernel patching which we should worry about, kernel
part is trivial.
What is not trivial is to patch all the systems out there who
autoloads network drivers based on /etc/modprobe.d/network-aliases.conf
(some local file), ie, numerous working setups which already
uses this mechanism since stone age. And patching these is
not trivial at all, unfortunately.
Somewhat weird setups (one can load the modules explicitly, and
nowadays this all is handled by udev anyway), but this change
will break some working systems.
Maybe the cost (some pain for some users) isn't large enough
but the outcome is good, and I think it _is_ good, but it needs
some wider discussion first, imho.
I can't think of a way to handle this without breaking stuff.
Thanks!
/mjt
On Fri, Feb 25, 2011 at 18:29 +0300, Michael Tokarev wrote:
> 25.02.2011 15:30, Vasiliy Kulikov wrote:
> > On Thu, Feb 24, 2011 at 16:34 +0000, Ben Hutchings wrote:
> >> On Thu, 2011-02-24 at 18:12 +0300, Vasiliy Kulikov wrote:
> >>> My proposal is changing request_module("%s", name) to something like
> >>> request_module("netdev-%s", name) inside of dev_load() and adding
> >>> aliases to related drivers.
>
> It is not the kernel patching which we should worry about, kernel
> part is trivial.
>
> What is not trivial is to patch all the systems out there who
> autoloads network drivers based on /etc/modprobe.d/network-aliases.conf
> (some local file), ie, numerous working setups which already
> uses this mechanism since stone age. And patching these is
> not trivial at all, unfortunately.
>
> Somewhat weird setups (one can load the modules explicitly, and
> nowadays this all is handled by udev anyway), but this change
> will break some working systems.
>
> Maybe the cost (some pain for some users) isn't large enough
> but the outcome is good, and I think it _is_ good, but it needs
> some wider discussion first, imho.
>
> I can't think of a way to handle this without breaking stuff.
Currently Linux slowly moves in the direction of rootless systems. This
definitely need proper restrictions of CAP_* power. Network admin does
nothing with general modules. It _has_ to break something one day
because old assumptions about permission stuff don't conform CAP_*
things: old assumptions are very closely connected with just everything.
I'm not sure how this particular CAP_NET_ADMIN misuse should be fixed,
maybe distributions should supply script to upgrade modprobe configs.
Also note that change s/CAP_SYS_MODULE/CAP_NET_ADMIN/ was made in
2.6.32, so there is a possibility that the set of affected distributions
(that doesn't use udev stuff) is very small.
Thanks for your input,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, 25 Feb 2011 18:14:14 +0300, Vasiliy Kulikov said:
> Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
> to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
> anybody load any module not related to networking.
>
> This patch restricts an ability of autoloading modules to netdev modules
> with explicit aliases. Currently there are only three users of the
> feature: ipip, ip_gre and sit.
And you stop an attacker from simply recompiling the module with a suitable
MODULE_ALIAS line added, how, exactly? This patch may make sense down the
road, but not while it's still trivial for a malicious root user to drop stuff
into /lib/modules.
And if you're going the route "but SELinux/SMACK/Tomoyo will prevent a malicious
root user from doing that", then the obvious reply is "this should be part of those
subsystems rather than something done one-off like this (especially as it has a chance
of breaking legitimate setups that use the current scheme).
On Fri, Feb 25, 2011 at 12:25 -0500, [email protected] wrote:
> And you stop an attacker from simply recompiling the module with a suitable
> MODULE_ALIAS line added, how, exactly? This patch may make sense down the
> road, but not while it's still trivial for a malicious root user to drop stuff
> into /lib/modules.
The threat is not a malicious root, but non-root with CAP_NET_ADMIN.
It's hardly possible to load arbitrary module into the kernel having
CAP_NET_ADMIN without other capabilities.
> And if you're going the route "but SELinux/SMACK/Tomoyo will prevent a malicious
> root user from doing that", then the obvious reply is "this should be part of those
> subsystems rather than something done one-off like this (especially as it has a chance
> of breaking legitimate setups that use the current scheme).
No, I don't want to add anything about LSMs at all.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, 2011-02-25 at 12:25 -0500, [email protected] wrote:
> On Fri, 25 Feb 2011 18:14:14 +0300, Vasiliy Kulikov said:
> > Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> > CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> > that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
> > to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
> > anybody load any module not related to networking.
> >
> > This patch restricts an ability of autoloading modules to netdev modules
> > with explicit aliases. Currently there are only three users of the
> > feature: ipip, ip_gre and sit.
>
> And you stop an attacker from simply recompiling the module with a suitable
> MODULE_ALIAS line added, how, exactly? This patch may make sense down the
> road, but not while it's still trivial for a malicious root user to drop stuff
> into /lib/modules.
A process running as root normally has CAP_NET_ADMIN, but not every
process with CAP_NET_ADMIN will be running as root.
> And if you're going the route "but SELinux/SMACK/Tomoyo will prevent a malicious
> root user from doing that", then the obvious reply is "this should be part of those
> subsystems rather than something done one-off like this (especially as it has a chance
> of breaking legitimate setups that use the current scheme).
The notional attacker has CAP_NET_ADMIN, perhaps through a vulnerable
service or a vulnerable set-capability executable. They do not yet have
full root access and so cannot install a module, even in the absence of
an LSM.
So long as the attacker is able to load arbitrary modules, however, they
could exploit a vulnerability in any installed (not loaded) module.
Again, LSMs are irrelevant to this as they do not protect against kernel
bugs.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Vasiliy Kulikov <[email protected]>
Date: Fri, 25 Feb 2011 18:14:14 +0300
> Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
> to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
> anybody load any module not related to networking.
Why go through this naming change, which does break things, instead of
simply adding a capability mask tag or similar to modules somehow. You
could stick it into a special elf section or similar.
Doesn't that make tons more sense than this?
On Fri, Feb 25, 2011 at 10:47 -0800, David Miller wrote:
> From: Vasiliy Kulikov <[email protected]>
> Date: Fri, 25 Feb 2011 18:14:14 +0300
>
> > Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> > CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> > that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
> > to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
> > anybody load any module not related to networking.
>
> Why go through this naming change, which does break things, instead of
> simply adding a capability mask tag or similar to modules somehow. You
> could stick it into a special elf section or similar.
>
> Doesn't that make tons more sense than this?
This is not "simply", adding special section for a single workaround
seems like an overkill for me - this touches the core (modules'
internals), which is not related to the initial CAP_* problem at all.
I'd be happy with not breaking anything, but I don't see any acceptable
solution.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
From: Vasiliy Kulikov <[email protected]>
Date: Fri, 25 Feb 2011 22:02:05 +0300
> On Fri, Feb 25, 2011 at 10:47 -0800, David Miller wrote:
>> From: Vasiliy Kulikov <[email protected]>
>> Date: Fri, 25 Feb 2011 18:14:14 +0300
>>
>> > Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
>> > CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
>> > that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
>> > to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
>> > anybody load any module not related to networking.
>>
>> Why go through this naming change, which does break things, instead of
>> simply adding a capability mask tag or similar to modules somehow. You
>> could stick it into a special elf section or similar.
>>
>> Doesn't that make tons more sense than this?
>
> This is not "simply", adding special section for a single workaround
> seems like an overkill for me - this touches the core (modules'
> internals), which is not related to the initial CAP_* problem at all.
>
> I'd be happy with not breaking anything, but I don't see any acceptable
> solution.
I think it's warranted given that it allows us to avoid breaking things.
I don't understand there is resistence in response to the first idea
I've seen proprosed that actually allows to fix the problem and not
break anything at the same time.
That seems silly.
On Fri, 2011-02-25 at 11:05 -0800, David Miller wrote:
> From: Vasiliy Kulikov <[email protected]>
> Date: Fri, 25 Feb 2011 22:02:05 +0300
>
> > On Fri, Feb 25, 2011 at 10:47 -0800, David Miller wrote:
> >> From: Vasiliy Kulikov <[email protected]>
> >> Date: Fri, 25 Feb 2011 18:14:14 +0300
> >>
> >> > Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> >> > CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> >> > that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are limited
> >> > to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't allow
> >> > anybody load any module not related to networking.
> >>
> >> Why go through this naming change, which does break things, instead of
> >> simply adding a capability mask tag or similar to modules somehow. You
> >> could stick it into a special elf section or similar.
> >>
> >> Doesn't that make tons more sense than this?
> >
> > This is not "simply", adding special section for a single workaround
> > seems like an overkill for me - this touches the core (modules'
> > internals), which is not related to the initial CAP_* problem at all.
> >
> > I'd be happy with not breaking anything, but I don't see any acceptable
> > solution.
>
> I think it's warranted given that it allows us to avoid breaking things.
>
> I don't understand there is resistence in response to the first idea
> I've seen proprosed that actually allows to fix the problem and not
> break anything at the same time.
>
> That seems silly.
You realise that module loading doesn't actually run in the context of
request_module(), right?
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Ben Hutchings <[email protected]>
Date: Fri, 25 Feb 2011 19:07:59 +0000
> You realise that module loading doesn't actually run in the context of
> request_module(), right?
Why is that a barrier? We could simply pass a capability mask into
request_module if necessary.
It's an implementation detail, and not a deterrant to my suggested
scheme.
On Fri, 2011-02-25 at 11:16 -0800, David Miller wrote:
> From: Ben Hutchings <[email protected]>
> Date: Fri, 25 Feb 2011 19:07:59 +0000
>
> > You realise that module loading doesn't actually run in the context of
> > request_module(), right?
>
> Why is that a barrier? We could simply pass a capability mask into
> request_module if necessary.
>
> It's an implementation detail, and not a deterrant to my suggested
> scheme.
It's not an implementation detail. modprobe currently runs with full
capabilities; your proposal requires its capabilities to be limited to
those of the capabilities of the process that triggered the
request_module() (plus, presumably, CAP_SYS_MODULE).
Now modprobe doesn't have CAP_DAC_OVERRIDE and can't read modprobe
configuration files that belong to users other than root.
It doesn't have CAP_SYS_MKNOD so it can't run hooks that call mknod.
etc.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Ben Hutchings <[email protected]>
Date: Fri, 25 Feb 2011 19:30:16 +0000
> On Fri, 2011-02-25 at 11:16 -0800, David Miller wrote:
>> From: Ben Hutchings <[email protected]>
>> Date: Fri, 25 Feb 2011 19:07:59 +0000
>>
>> > You realise that module loading doesn't actually run in the context of
>> > request_module(), right?
>>
>> Why is that a barrier? We could simply pass a capability mask into
>> request_module if necessary.
>>
>> It's an implementation detail, and not a deterrant to my suggested
>> scheme.
>
> It's not an implementation detail. modprobe currently runs with full
> capabilities; your proposal requires its capabilities to be limited to
> those of the capabilities of the process that triggered the
> request_module() (plus, presumably, CAP_SYS_MODULE).
The idea was that the kernel will be the entity that will inspect the
elf sections and validate the capability bits, not the userspace
module loader.
Surely we if we can pass an arbitrary string out to the loading
process as part of the module loading context, we can pass along
capability bits as well.
On Fri, 2011-02-25 at 11:43 -0800, David Miller wrote:
> From: Ben Hutchings <[email protected]>
> Date: Fri, 25 Feb 2011 19:30:16 +0000
>
> > On Fri, 2011-02-25 at 11:16 -0800, David Miller wrote:
> >> From: Ben Hutchings <[email protected]>
> >> Date: Fri, 25 Feb 2011 19:07:59 +0000
> >>
> >> > You realise that module loading doesn't actually run in the context of
> >> > request_module(), right?
> >>
> >> Why is that a barrier? We could simply pass a capability mask into
> >> request_module if necessary.
> >>
> >> It's an implementation detail, and not a deterrant to my suggested
> >> scheme.
> >
> > It's not an implementation detail. modprobe currently runs with full
> > capabilities; your proposal requires its capabilities to be limited to
> > those of the capabilities of the process that triggered the
> > request_module() (plus, presumably, CAP_SYS_MODULE).
>
> The idea was that the kernel will be the entity that will inspect the
> elf sections and validate the capability bits, not the userspace
> module loader.
Yes, I understand that.
> Surely we if we can pass an arbitrary string out to the loading
> process as part of the module loading context, we can pass along
> capability bits as well.
If you want insert_module() to be able to deny loading some modules
based on the capabilities of the process calling request_module() then
you either have to *reduce* the capabilities given to modprobe or create
some extra process state, separate from the usual capability state,
specifically for this purpose.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Ben Hutchings <[email protected]>
Date: Fri, 25 Feb 2011 19:53:05 +0000
> On Fri, 2011-02-25 at 11:43 -0800, David Miller wrote:
>> Surely we if we can pass an arbitrary string out to the loading
>> process as part of the module loading context, we can pass along
>> capability bits as well.
>
> If you want insert_module() to be able to deny loading some modules
> based on the capabilities of the process calling request_module() then
> you either have to *reduce* the capabilities given to modprobe or create
> some extra process state, separate from the usual capability state,
> specifically for this purpose.
How is this any different from the patch posted which ties
capabilities to the prefix of name of the module to be loaded?
There is simply no difference, except that in my proposal existing
things do not break since the module name will not change.
I don't see where the complexity is, if the only place we can pass the
capability bits is in the execv args, then in the worst case we could
take a peek at those in the module load system call.
On Fri, 2011-02-25 at 19:53 +0000, Ben Hutchings wrote:
> On Fri, 2011-02-25 at 11:43 -0800, David Miller wrote:
> > From: Ben Hutchings <[email protected]>
> > Date: Fri, 25 Feb 2011 19:30:16 +0000
> >
> > > On Fri, 2011-02-25 at 11:16 -0800, David Miller wrote:
> > >> From: Ben Hutchings <[email protected]>
> > >> Date: Fri, 25 Feb 2011 19:07:59 +0000
> > >>
> > >> > You realise that module loading doesn't actually run in the context of
> > >> > request_module(), right?
> > >>
> > >> Why is that a barrier? We could simply pass a capability mask into
> > >> request_module if necessary.
> > >>
> > >> It's an implementation detail, and not a deterrant to my suggested
> > >> scheme.
> > >
> > > It's not an implementation detail. modprobe currently runs with full
> > > capabilities; your proposal requires its capabilities to be limited to
> > > those of the capabilities of the process that triggered the
> > > request_module() (plus, presumably, CAP_SYS_MODULE).
> >
> > The idea was that the kernel will be the entity that will inspect the
> > elf sections and validate the capability bits, not the userspace
> > module loader.
>
> Yes, I understand that.
>
> > Surely we if we can pass an arbitrary string out to the loading
> > process as part of the module loading context, we can pass along
> > capability bits as well.
>
> If you want insert_module() to be able to deny loading some modules
> based on the capabilities of the process calling request_module() then
> you either have to *reduce* the capabilities given to modprobe or create
> some extra process state, separate from the usual capability state,
> specifically for this purpose.
I bet something like this (plus Vasiliy's changes to static module
aliases) would cover 99.9% of legitimate uses of this feature:
diff --git a/net/core/dev.c b/net/core/dev.c
index 54aaca6..0d09baa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1120,8 +1120,20 @@ void dev_load(struct net *net, const char *name)
dev = dev_get_by_name_rcu(net, name);
rcu_read_unlock();
- if (!dev && capable(CAP_NET_ADMIN))
- request_module("%s", name);
+ if (!dev && capable(CAP_NET_ADMIN)) {
+ /* Check whether the name looks like one that a net
+ * driver will generate initially. If not, require a
+ * module alias with a suitable prefix, so that this
+ * can't be used to load arbitrary modules.
+ */
+ if ((strncmp(name, "eth", 3) == 0 &&
+ isdigit((unsigned char)name[3])) ||
+ (strncmp(name, "wlan", 4) == 0 &&
+ isdigit((unsigned char)name[4])))
+ request_module("%s", name);
+ else
+ request_module("netdev-%s", name);
+ }
}
EXPORT_SYMBOL(dev_load);
---
Note that we don't have to care about interfaces that get renamed from
eth%d or wlan%d, since renaming is triggered asynchronously and
therefore can't be used in conjunction with the auto-loading feature.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
2011/2/25 Ben Hutchings <[email protected]>:
> I bet something like this (plus Vasiliy's changes to static module
> aliases) would cover 99.9% of legitimate uses of this feature:
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 54aaca6..0d09baa 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1120,8 +1120,20 @@ void dev_load(struct net *net, const char *name)
> ? ? ? ?dev = dev_get_by_name_rcu(net, name);
> ? ? ? ?rcu_read_unlock();
>
> - ? ? ? if (!dev && capable(CAP_NET_ADMIN))
> - ? ? ? ? ? ? ? request_module("%s", name);
> + ? ? ? if (!dev && capable(CAP_NET_ADMIN)) {
> + ? ? ? ? ? ? ? /* Check whether the name looks like one that a net
> + ? ? ? ? ? ? ? ?* driver will generate initially. ?If not, require a
> + ? ? ? ? ? ? ? ?* module alias with a suitable prefix, so that this
> + ? ? ? ? ? ? ? ?* can't be used to load arbitrary modules.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? if ((strncmp(name, "eth", 3) == 0 &&
> + ? ? ? ? ? ? ? ? ? ?isdigit((unsigned char)name[3])) ||
> + ? ? ? ? ? ? ? ? ? (strncmp(name, "wlan", 4) == 0 &&
> + ? ? ? ? ? ? ? ? ? ?isdigit((unsigned char)name[4])))
> + ? ? ? ? ? ? ? ? ? ? ? request_module("%s", name);
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? request_module("netdev-%s", name);
> + ? ? ? }
> ?}
> ?EXPORT_SYMBOL(dev_load);
>
This might be better as:
if (request_module("netdev-%s", name))
... fallback
Then after some years the fallback could be removed if announced properly.
Best Regards,
Micha? Miros?aw
David,
On Fri, Feb 25, 2011 at 11:16 -0800, David Miller wrote:
> From: Ben Hutchings <[email protected]>
> Date: Fri, 25 Feb 2011 19:07:59 +0000
>
> > You realise that module loading doesn't actually run in the context of
> > request_module(), right?
>
> Why is that a barrier? We could simply pass a capability mask into
> request_module if necessary.
>
> It's an implementation detail, and not a deterrant to my suggested
> scheme.
Let's discuss your scheme. AFAIU, you suggest to change:
1. a) request_module("%s", devname) =>
request_module_with_caps(CAP_NET_ADMIN, "%s", devname)
b) call_usermodehelper() => call_usermodehelper_with_caps()
c) add some bits/sections into kernel module image indicating that
this module is safe to be loaded via CAP_NET_ADMIN
d) run modprobe with CAP_NET_ADMIN only
e) in load_module() check whether (the process has CAP_SYS_MODULE) or
(the process has CAP_NET_ADMIN and bit SAFE_NET_MODULE is raised in
the module image)
This obviously doesn't work - the kernel is not able to verify whether
the bit/section is not malformed by user with CAP_NET_ADMIN.
-OR-
1. a) request_module("%s", devname) => request_module_with_argument("--netdev", "%s", devname)
b) patch modprobe to add "--netmodule-only" argument (or bitmask,
whatever), this would indicate that only net/** modules may be loaded.
Then the things are still broken - a user has to update modprobe
together with the kernel, otherwise the updated kernel would call
"modprobe" with unsupported argument and even "sit0" wouldn't work.
Additionally this touches module loading process, which is not buggy.
Or you propose something else besides these 2 ways? Please clarify.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
From: Vasiliy Kulikov <[email protected]>
Date: Sun, 27 Feb 2011 14:44:38 +0300
> d) run modprobe with CAP_NET_ADMIN only
This is not part of my scheme.
The module loading will run with existing module loading privileges,
the "allowed capability" bits will be passed along back into the
kernel at module load time (via modprobe arguments or similar)
and validated by the kernel as it walks the ELF sections anyways
to perform relocations and whatnot.
From: Vasiliy Kulikov <[email protected]>
Date: Sun, 27 Feb 2011 14:44:38 +0300
> Then the things are still broken - a user has to update modprobe
> together with the kernel, otherwise the updated kernel would call
> "modprobe" with unsupported argument and even "sit0" wouldn't work.
The capability bits get passed on the modprobe command line.
The module loading system call in the kernel inspects the command
line looking for the argument, and uses it to validate the module
load by comparing the mask with the special ELF section.
On Friday 25 February 2011, Micha? Miros?aw wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 54aaca6..0d09baa 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1120,8 +1120,20 @@ void dev_load(struct net *net, const char *name)
> > dev = dev_get_by_name_rcu(net, name);
> > rcu_read_unlock();
> >
> > - if (!dev && capable(CAP_NET_ADMIN))
> > - request_module("%s", name);
> > + if (!dev && capable(CAP_NET_ADMIN)) {
> > + /* Check whether the name looks like one that a net
> > + * driver will generate initially. If not, require a
> > + * module alias with a suitable prefix, so that this
> > + * can't be used to load arbitrary modules.
> > + */
> > + if ((strncmp(name, "eth", 3) == 0 &&
> > + isdigit((unsigned char)name[3])) ||
> > + (strncmp(name, "wlan", 4) == 0 &&
> > + isdigit((unsigned char)name[4])))
> > + request_module("%s", name);
> > + else
> > + request_module("netdev-%s", name);
> > + }
> > }
> > EXPORT_SYMBOL(dev_load);
> >
>
> This might be better as:
>
> if (request_module("netdev-%s", name))
> ... fallback
>
> Then after some years the fallback could be removed if announced properly.
The backwards compatibility should mostly be for systems that today don't
use split capabilities, right?
The fallback could therefore rely on CAP_SYS_MODULE as well:
if (request_module("netdev-%s", name)) {
if (capable(CAP_SYS_MODULE))
request_module("%s", name);
}
Not 100% solution, but should solve the capability escalation nicely without
causing much pain.
Arnd
27.02.2011 23:22, Arnd Bergmann wrote:
> On Friday 25 February 2011, Micha? Miros?aw wrote:
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 54aaca6..0d09baa 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -1120,8 +1120,20 @@ void dev_load(struct net *net, const char *name)
>>> dev = dev_get_by_name_rcu(net, name);
>>> rcu_read_unlock();
>>>
>>> - if (!dev && capable(CAP_NET_ADMIN))
>>> - request_module("%s", name);
>>> + if (!dev && capable(CAP_NET_ADMIN)) {
>>> + /* Check whether the name looks like one that a net
>>> + * driver will generate initially. If not, require a
>>> + * module alias with a suitable prefix, so that this
>>> + * can't be used to load arbitrary modules.
>>> + */
>>> + if ((strncmp(name, "eth", 3) == 0 &&
>>> + isdigit((unsigned char)name[3])) ||
>>> + (strncmp(name, "wlan", 4) == 0 &&
>>> + isdigit((unsigned char)name[4])))
>>> + request_module("%s", name);
>>> + else
>>> + request_module("netdev-%s", name);
>>> + }
>>> }
>>> EXPORT_SYMBOL(dev_load);
>>>
>>
>> This might be better as:
>>
>> if (request_module("netdev-%s", name))
>> ... fallback
>>
>> Then after some years the fallback could be removed if announced properly.
>
> The backwards compatibility should mostly be for systems that today don't
> use split capabilities, right?
>
> The fallback could therefore rely on CAP_SYS_MODULE as well:
>
> if (request_module("netdev-%s", name)) {
> if (capable(CAP_SYS_MODULE))
> request_module("%s", name);
> }
>
> Not 100% solution, but should solve the capability escalation nicely without
> causing much pain.
To me this looks like the best solution so far - trivial and
compatible.
Thanks!
/mjt
On Mon, Feb 28, 2011 at 12:29 +0300, Michael Tokarev wrote:
> 27.02.2011 23:22, Arnd Bergmann wrote:
> > The backwards compatibility should mostly be for systems that today don't
> > use split capabilities, right?
> >
> > The fallback could therefore rely on CAP_SYS_MODULE as well:
> >
> > if (request_module("netdev-%s", name)) {
> > if (capable(CAP_SYS_MODULE))
> > request_module("%s", name);
> > }
> >
> > Not 100% solution, but should solve the capability escalation nicely without
> > causing much pain.
>
> To me this looks like the best solution so far - trivial and
> compatible.
Agreed, it's looks good. But before the request_module() there is a check
for capabile(CAP_NET_ADMIN), IMO it's better to request either
CAP_NET_ADMIN or CAP_SYS_MODULE, not both of them.
if (!dev) {
if (capable(CAP_NET_ADMIN))
request_module("netdev-%s", name))
if (capable(CAP_SYS_MODULE) {
if (!request_module("%s", name))
WARN_ONE(1, "Loading kernel module for a network device"
" with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias"
" netdev-%s instead\n", name);
}
}
The only drawback is distributions/setups that already use
CAP_SYS_MODULE'less network scripts.
David, are you OK with this way?
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
From: Vasiliy Kulikov <[email protected]>
Date: Mon, 28 Feb 2011 12:51:33 +0300
> David, are you OK with this way?
Sure.
Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
limited to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't
allow anybody load any module not related to networking.
This patch restricts an ability of autoloading modules to netdev modules
with explicit aliases. This fixes CVE-2011-1019.
Arnd Bergmann suggested to leave untouched the old pre-v2.6.32 behavior
of loading netdev modules by name (without any prefix) for processes
with CAP_SYS_MODULE to maintain the compatibility with network scripts
that use autoloading netdev modules by aliases like "eth0", "wlan0".
Currently there are only three users of the feature in the upstream
kernel: ipip, ip_gre and sit.
root@albatros:~# capsh --drop=$(seq -s, 0 11),$(seq -s, 13 34) --
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: fffffff800001000
CapEff: fffffff800001000
CapBnd: fffffff800001000
root@albatros:~# modprobe xfs
FATAL: Error inserting xfs
(/lib/modules/2.6.38-rc6-00001-g2bf4ca3/kernel/fs/xfs/xfs.ko): Operation not permitted
root@albatros:~# lsmod | grep xfs
root@albatros:~# ifconfig xfs
xfs: error fetching interface information: Device not found
root@albatros:~# lsmod | grep xfs
root@albatros:~# lsmod | grep sit
root@albatros:~# ifconfig sit
sit: error fetching interface information: Device not found
root@albatros:~# lsmod | grep sit
root@albatros:~# ifconfig sit0
sit0 Link encap:IPv6-in-IPv4
NOARP MTU:1480 Metric:1
root@albatros:~# lsmod | grep sit
sit 10457 0
tunnel4 2957 1 sit
For CAP_SYS_MODULE module loading is still relaxed:
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: ffffffffffffffff
CapEff: ffffffffffffffff
CapBnd: ffffffffffffffff
root@albatros:~# ifconfig xfs
xfs: error fetching interface information: Device not found
root@albatros:~# lsmod | grep xfs
xfs 745319 0
Reference: https://lkml.org/lkml/2011/2/24/203
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
include/linux/netdevice.h | 3 +++
net/core/dev.c | 12 ++++++++++--
net/ipv4/ip_gre.c | 2 +-
net/ipv4/ipip.c | 2 +-
net/ipv6/sit.c | 2 +-
5 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d971346..71caf7a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2392,6 +2392,9 @@ extern int netdev_notice(const struct net_device *dev, const char *format, ...)
extern int netdev_info(const struct net_device *dev, const char *format, ...)
__attribute__ ((format (printf, 2, 3)));
+#define MODULE_ALIAS_NETDEV(device) \
+ MODULE_ALIAS("netdev-" device)
+
#if defined(DEBUG)
#define netdev_dbg(__dev, format, args...) \
netdev_printk(KERN_DEBUG, __dev, format, ##args)
diff --git a/net/core/dev.c b/net/core/dev.c
index 8ae6631..fc6f037 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1119,8 +1119,16 @@ void dev_load(struct net *net, const char *name)
dev = dev_get_by_name_rcu(net, name);
rcu_read_unlock();
- if (!dev && capable(CAP_NET_ADMIN))
- request_module("%s", name);
+ if (!dev) {
+ if (capable(CAP_NET_ADMIN))
+ request_module("netdev-%s", name);
+ if (capable(CAP_SYS_MODULE)) {
+ if (!request_module("%s", name))
+ WARN_ONCE(1, "Loading kernel module for a "
+"network device with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias "
+"netdev-%s instead\n", name);
+ }
+ }
}
EXPORT_SYMBOL(dev_load);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 6613edf..d1d0e2c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
MODULE_LICENSE("GPL");
MODULE_ALIAS_RTNL_LINK("gre");
MODULE_ALIAS_RTNL_LINK("gretap");
-MODULE_ALIAS("gre0");
+MODULE_ALIAS_NETDEV("gre0");
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 988f52f..a5f58e7 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -913,4 +913,4 @@ static void __exit ipip_fini(void)
module_init(ipip_init);
module_exit(ipip_fini);
MODULE_LICENSE("GPL");
-MODULE_ALIAS("tunl0");
+MODULE_ALIAS_NETDEV("tunl0");
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 8ce38f1..d2c16e1 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1290,4 +1290,4 @@ static int __init sit_init(void)
module_init(sit_init);
module_exit(sit_cleanup);
MODULE_LICENSE("GPL");
-MODULE_ALIAS("sit0");
+MODULE_ALIAS_NETDEV("sit0");
--
1.7.0.4
On Tue, 2011-03-01 at 22:48 +0300, Vasiliy Kulikov wrote:
> Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
> limited to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't
> allow anybody load any module not related to networking.
>
> This patch restricts an ability of autoloading modules to netdev modules
> with explicit aliases. This fixes CVE-2011-1019.
>
> Arnd Bergmann suggested to leave untouched the old pre-v2.6.32 behavior
> of loading netdev modules by name (without any prefix) for processes
> with CAP_SYS_MODULE to maintain the compatibility with network scripts
> that use autoloading netdev modules by aliases like "eth0", "wlan0".
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8ae6631..fc6f037 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1119,8 +1119,16 @@ void dev_load(struct net *net, const char *name)
> dev = dev_get_by_name_rcu(net, name);
> rcu_read_unlock();
>
> - if (!dev && capable(CAP_NET_ADMIN))
> - request_module("%s", name);
> + if (!dev) {
> + if (capable(CAP_NET_ADMIN))
> + request_module("netdev-%s", name);
If this succeeds then the second request_module() should be skipped.
> + if (capable(CAP_SYS_MODULE)) {
> + if (!request_module("%s", name))
> + WARN_ONCE(1, "Loading kernel module for a "
> +"network device with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias "
> +"netdev-%s instead\n", name);
[...]
If this feature is to be deprecated, there should be an error message
for each interface that depends on it. However, use of the feature is
not a bug so WARN is not appropriate. I think pr_err() would be fine.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
limited to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't
allow anybody load any module not related to networking.
This patch restricts an ability of autoloading modules to netdev modules
with explicit aliases. This fixes CVE-2011-1019.
Arnd Bergmann suggested to leave untouched the old pre-v2.6.32 behavior
of loading netdev modules by name (without any prefix) for processes
with CAP_SYS_MODULE to maintain the compatibility with network scripts
that use autoloading netdev modules by aliases like "eth0", "wlan0".
Currently there are only three users of the feature in the upstream
kernel: ipip, ip_gre and sit.
root@albatros:~# capsh --drop=$(seq -s, 0 11),$(seq -s, 13 34) --
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: fffffff800001000
CapEff: fffffff800001000
CapBnd: fffffff800001000
root@albatros:~# modprobe xfs
FATAL: Error inserting xfs
(/lib/modules/2.6.38-rc6-00001-g2bf4ca3/kernel/fs/xfs/xfs.ko): Operation not permitted
root@albatros:~# lsmod | grep xfs
root@albatros:~# ifconfig xfs
xfs: error fetching interface information: Device not found
root@albatros:~# lsmod | grep xfs
root@albatros:~# lsmod | grep sit
root@albatros:~# ifconfig sit
sit: error fetching interface information: Device not found
root@albatros:~# lsmod | grep sit
root@albatros:~# ifconfig sit0
sit0 Link encap:IPv6-in-IPv4
NOARP MTU:1480 Metric:1
root@albatros:~# lsmod | grep sit
sit 10457 0
tunnel4 2957 1 sit
For CAP_SYS_MODULE module loading is still relaxed:
root@albatros:~# grep Cap /proc/$$/status
CapInh: 0000000000000000
CapPrm: ffffffffffffffff
CapEff: ffffffffffffffff
CapBnd: ffffffffffffffff
root@albatros:~# ifconfig xfs
xfs: error fetching interface information: Device not found
root@albatros:~# lsmod | grep xfs
xfs 745319 0
Reference: https://lkml.org/lkml/2011/2/24/203
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
v2 - use pr_err()
- don't try to load $name if netdev-$name is loaded
include/linux/netdevice.h | 3 +++
net/core/dev.c | 12 ++++++++++--
net/ipv4/ip_gre.c | 2 +-
net/ipv4/ipip.c | 2 +-
net/ipv6/sit.c | 2 +-
5 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d971346..71caf7a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2392,6 +2392,9 @@ extern int netdev_notice(const struct net_device *dev, const char *format, ...)
extern int netdev_info(const struct net_device *dev, const char *format, ...)
__attribute__ ((format (printf, 2, 3)));
+#define MODULE_ALIAS_NETDEV(device) \
+ MODULE_ALIAS("netdev-" device)
+
#if defined(DEBUG)
#define netdev_dbg(__dev, format, args...) \
netdev_printk(KERN_DEBUG, __dev, format, ##args)
diff --git a/net/core/dev.c b/net/core/dev.c
index 8ae6631..6561021 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1114,13 +1114,21 @@ EXPORT_SYMBOL(netdev_bonding_change);
void dev_load(struct net *net, const char *name)
{
struct net_device *dev;
+ int no_module;
rcu_read_lock();
dev = dev_get_by_name_rcu(net, name);
rcu_read_unlock();
- if (!dev && capable(CAP_NET_ADMIN))
- request_module("%s", name);
+ no_module = !dev;
+ if (no_module && capable(CAP_NET_ADMIN))
+ no_module = request_module("netdev-%s", name);
+ if (no_module && capable(CAP_SYS_MODULE)) {
+ if (!request_module("%s", name))
+ pr_err("Loading kernel module for a network device "
+"with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias netdev-%s "
+"instead\n", name);
+ }
}
EXPORT_SYMBOL(dev_load);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 6613edf..d1d0e2c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
MODULE_LICENSE("GPL");
MODULE_ALIAS_RTNL_LINK("gre");
MODULE_ALIAS_RTNL_LINK("gretap");
-MODULE_ALIAS("gre0");
+MODULE_ALIAS_NETDEV("gre0");
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 988f52f..a5f58e7 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -913,4 +913,4 @@ static void __exit ipip_fini(void)
module_init(ipip_init);
module_exit(ipip_fini);
MODULE_LICENSE("GPL");
-MODULE_ALIAS("tunl0");
+MODULE_ALIAS_NETDEV("tunl0");
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 8ce38f1..d2c16e1 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1290,4 +1290,4 @@ static int __init sit_init(void)
module_init(sit_init);
module_exit(sit_cleanup);
MODULE_LICENSE("GPL");
-MODULE_ALIAS("sit0");
+MODULE_ALIAS_NETDEV("sit0");
--
1.7.0.4
02.03.2011 00:33, Vasiliy Kulikov wrote:
> Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
> limited to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't
> allow anybody load any module not related to networking.
>
> This patch restricts an ability of autoloading modules to netdev modules
> with explicit aliases. This fixes CVE-2011-1019.
[]
> Reference: https://lkml.org/lkml/2011/2/24/203
>
> Signed-off-by: Vasiliy Kulikov <[email protected]>
This looks much saner :)
Signed-off-by: Michael Tokarev <[email protected]>
/mjt
On Wed, Mar 02, 2011 at 12:33:13AM +0300, Vasiliy Kulikov wrote:
> This patch restricts an ability of autoloading modules to netdev modules
> with explicit aliases. This fixes CVE-2011-1019.
> ...
> Signed-off-by: Vasiliy Kulikov <[email protected]>
Looks good; thanks for sorting this out.
Acked-by: Kees Cook <[email protected]>
-Kees
--
Kees Cook
Ubuntu Security Team
On Wed, Mar 02, 2011 at 12:39 -0700, Jake Edge wrote:
> > @@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
> > MODULE_LICENSE("GPL");
> > MODULE_ALIAS_RTNL_LINK("gre");
> > MODULE_ALIAS_RTNL_LINK("gretap");
> > -MODULE_ALIAS("gre0");
> > +MODULE_ALIAS_NETDEV("gre0");
>
> that is, instead of replacing MODULE_ALIAS("gre0") with the NETDEV
> version, don't you want both for backward compatibility?
The networking script will run with CAP_NET_ADMIN, this would request
netdev-gre0 and load ip_gre.
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
I am probably missing something, but shouldn't the existing
MODULE_ALIASes stay?
Vasiliy Kulikov <[email protected]> writes:
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 6613edf..d1d0e2c 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_RTNL_LINK("gre");
> MODULE_ALIAS_RTNL_LINK("gretap");
> -MODULE_ALIAS("gre0");
> +MODULE_ALIAS_NETDEV("gre0");
that is, instead of replacing MODULE_ALIAS("gre0") with the NETDEV
version, don't you want both for backward compatibility?
(if so, same goes for the other two)
jake
--
Jake Edge - [email protected] - http://lwn.net
On Wed, Mar 02, 2011 at 12:49 -0700, Jake Edge wrote:
> On Wed, 2 Mar 2011 22:43:54 +0300 Vasiliy Kulikov wrote:
> > On Wed, Mar 02, 2011 at 12:39 -0700, Jake Edge wrote:
> > > > @@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
> > > > MODULE_LICENSE("GPL");
> > > > MODULE_ALIAS_RTNL_LINK("gre");
> > > > MODULE_ALIAS_RTNL_LINK("gretap");
> > > > -MODULE_ALIAS("gre0");
> > > > +MODULE_ALIAS_NETDEV("gre0");
> > >
> > > that is, instead of replacing MODULE_ALIAS("gre0") with the NETDEV
> > > version, don't you want both for backward compatibility?
> >
> > The networking script will run with CAP_NET_ADMIN, this would request
> > netdev-gre0 and load ip_gre.
>
> and on systems that today use CAP_SYS_MODULE
Since Linux 2.6.32 CAP_SYS_MODULE may not load modules via "ifconfig
gre0". It was changed to CAP_NET_ADMIN. So nothing is broken here.
> (or really the full set of
> capabilities cuz they are running as root)?
As root has CAP_NET_ADMIN, the alias netdev-gre0 is tried and it succeeds.
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Wed, 2 Mar 2011 22:43:54 +0300 Vasiliy Kulikov wrote:
> On Wed, Mar 02, 2011 at 12:39 -0700, Jake Edge wrote:
> > > @@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
> > > MODULE_LICENSE("GPL");
> > > MODULE_ALIAS_RTNL_LINK("gre");
> > > MODULE_ALIAS_RTNL_LINK("gretap");
> > > -MODULE_ALIAS("gre0");
> > > +MODULE_ALIAS_NETDEV("gre0");
> >
> > that is, instead of replacing MODULE_ALIAS("gre0") with the NETDEV
> > version, don't you want both for backward compatibility?
>
> The networking script will run with CAP_NET_ADMIN, this would request
> netdev-gre0 and load ip_gre.
and on systems that today use CAP_SYS_MODULE (or really the full set of
capabilities cuz they are running as root)? won't those try to load
gre0 and fail because that alias doesn't exist?
jake
--
Jake Edge - LWN - [email protected] - http://lwn.net
On Wed, 2 Mar 2011 23:18:07 +0300 Vasiliy Kulikov wrote:
> > and on systems that today use CAP_SYS_MODULE
>
> Since Linux 2.6.32 CAP_SYS_MODULE may not load modules via "ifconfig
> gre0". It was changed to CAP_NET_ADMIN. So nothing is broken here.
>
> > (or really the full set of
> > capabilities cuz they are running as root)?
>
> As root has CAP_NET_ADMIN, the alias netdev-gre0 is tried and it
> succeeds.
(I feel like I'm beating a dead horse here, sorry if so ...)
If I have a setuid-root program today that loads ip_gre by using the
alias "gre0", and I run that program on a kernel with this change,
won't it fail because the "gre0" alias is missing? That program
doesn't know to try "netdev-gre0". i.e. won't backward compatibility be
affected by this change?
jake
--
Jake Edge - LWN - [email protected] - http://lwn.net
On Wed, 2 Mar 2011 23:18:07 +0300 Vasiliy Kulikov wrote:
> As root has CAP_NET_ADMIN, the alias netdev-gre0 is tried and it
> succeeds.
Never mind, my apologies for the noise ... one always reads replies
most carefully just *after* hitting send ...
jake
--
Jake Edge - LWN - [email protected] - http://lwn.net
On Wed, Mar 02, 2011 at 10:15 +0300, Michael Tokarev wrote:
> 02.03.2011 00:33, Vasiliy Kulikov wrote:
> > Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> > CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
> > that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
> > limited to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't
> > allow anybody load any module not related to networking.
> >
> > This patch restricts an ability of autoloading modules to netdev modules
> > with explicit aliases. This fixes CVE-2011-1019.
> []
> > Reference: https://lkml.org/lkml/2011/2/24/203
> >
> > Signed-off-by: Vasiliy Kulikov <[email protected]>
>
> This looks much saner :)
>
> Signed-off-by: Michael Tokarev <[email protected]>
Is there any reason why it is not yet merged? I've answered to Jake
Edge that it is not a regression.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
From: Vasiliy Kulikov <[email protected]>
Date: Thu, 10 Mar 2011 01:06:34 +0300
> On Wed, Mar 02, 2011 at 10:15 +0300, Michael Tokarev wrote:
>> 02.03.2011 00:33, Vasiliy Kulikov wrote:
>> > Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
>> > CAP_NET_ADMIN may load any module from /lib/modules/. This doesn't mean
>> > that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
>> > limited to /lib/modules/**. However, CAP_NET_ADMIN capability shouldn't
>> > allow anybody load any module not related to networking.
>> >
>> > This patch restricts an ability of autoloading modules to netdev modules
>> > with explicit aliases. This fixes CVE-2011-1019.
>> []
>> > Reference: https://lkml.org/lkml/2011/2/24/203
>> >
>> > Signed-off-by: Vasiliy Kulikov <[email protected]>
>>
>> This looks much saner :)
>>
>> Signed-off-by: Michael Tokarev <[email protected]>
>
> Is there any reason why it is not yet merged? I've answered to Jake
> Edge that it is not a regression.
I was hoping someone other than me would take this upstream, feel free
to submit it directly to Linus with my ack:
Acked-by: David S. Miller <[email protected]>
On Wed, 9 Mar 2011, David Miller wrote:
> I was hoping someone other than me would take this upstream, feel free
> to submit it directly to Linus with my ack:
>
> Acked-by: David S. Miller <[email protected]>
I can submit it via my tree.
--
James Morris
<[email protected]>
James,
On Thu, Mar 10, 2011 at 09:53 +1100, James Morris wrote:
> I can submit it via my tree.
Thank you!
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <[email protected]> wrote:
> Since a8f80e8ff94ecba629542d9b4b5f5a8ee3eb565c any process with
> CAP_NET_ADMIN may load any module from /lib/modules/. ?This doesn't mean
> that CAP_NET_ADMIN is a superset of CAP_SYS_MODULE as modules are
> limited to /lib/modules/**. ?However, CAP_NET_ADMIN capability shouldn't
> allow anybody load any module not related to networking.
>
> This patch restricts an ability of autoloading modules to netdev modules
> with explicit aliases. ?This fixes CVE-2011-1019.
>
> Arnd Bergmann suggested to leave untouched the old pre-v2.6.32 behavior
> of loading netdev modules by name (without any prefix) for processes
> with CAP_SYS_MODULE to maintain the compatibility with network scripts
> that use autoloading netdev modules by aliases like "eth0", "wlan0".
>
> Currently there are only three users of the feature in the upstream
> kernel: ipip, ip_gre and sit.
This patch is causing a bit of a problem in Fedora. The problem lies
mostly in the fact that we, by means of using SELinux, grant very very
few domains CAP_SYS_MODULE (and we record when domains attempt to use
this permission). Unlike most other distros in which uid==0 is for
all intensive purposes == CAP_FULL_SET and there is no logging of
failures to use capabilities. What happened is that as soon as we
instituted this change we started getting SELinux denials because lots
of domains (libvirt, udev, iw, NetworkManager) all the sudden started
trying to use CAP_SYS_MODULE. It took me a minute to make sure this
patch was the problem because I wasn't seeing any printk messages. I
had to make some changes to the patch to print every time a task got
into the CAP_SYS_MODULE case in order to get an idea what was causing
the problem. I found on my machine I hit the problem 3 times trying
to load "reg", "wifi0", and "virbr0" None of these are actual
modules in userspace so the upcall failed.
I'm trying to figure out how I should be dealing with this.
My first idea is changing the capable(CAP_SYS_MODULE) into a
has_capability_noaudit(). Which will not audit the access attempt.
I'm not sure I like that since it uses the read cred, it doesn't set
PF_SUPERPRIV, and it means we could likely miss recording a problem if
a task is doing this intentionally...
The next idea is I guess figuring out what's causing these and fix
them there, but I'm not certain a good way to debug it. I know from
our audit logs that wpa_supplicant is calling SIOCGIFINDEX which is
causing one of these, libvirt is calling SIOCGIFFLAGS. I'm not sure
what udev->iw is doing to trigger it's problem.....
If the name in question is not coming from direct userspace request I
guess I need help figuring out what is causing them....
So while this patch might not necessarily be breaking things it
certainly is not regression free and could potentially be breaking
systems with fine grained capabilities controls....
-Eric
> ? ?root@albatros:~# capsh --drop=$(seq -s, 0 11),$(seq -s, 13 34) --
> ? ?root@albatros:~# grep Cap /proc/$$/status
> ? ?CapInh: ? ? 0000000000000000
> ? ?CapPrm: ? ? fffffff800001000
> ? ?CapEff: ? ? fffffff800001000
> ? ?CapBnd: ? ? fffffff800001000
> ? ?root@albatros:~# modprobe xfs
> ? ?FATAL: Error inserting xfs
> ? ?(/lib/modules/2.6.38-rc6-00001-g2bf4ca3/kernel/fs/xfs/xfs.ko): Operation not permitted
> ? ?root@albatros:~# lsmod | grep xfs
> ? ?root@albatros:~# ifconfig xfs
> ? ?xfs: error fetching interface information: Device not found
> ? ?root@albatros:~# lsmod | grep xfs
> ? ?root@albatros:~# lsmod | grep sit
> ? ?root@albatros:~# ifconfig sit
> ? ?sit: error fetching interface information: Device not found
> ? ?root@albatros:~# lsmod | grep sit
> ? ?root@albatros:~# ifconfig sit0
> ? ?sit0 ? ? ?Link encap:IPv6-in-IPv4
> ? ? ? ? ? ? ?NOARP ?MTU:1480 ?Metric:1
>
> ? ?root@albatros:~# lsmod | grep sit
> ? ?sit ? ? ? ? ? ? ? ? ? ?10457 ?0
> ? ?tunnel4 ? ? ? ? ? ? ? ? 2957 ?1 sit
>
> For CAP_SYS_MODULE module loading is still relaxed:
>
> ? ?root@albatros:~# grep Cap /proc/$$/status
> ? ?CapInh: ? ? 0000000000000000
> ? ?CapPrm: ? ? ffffffffffffffff
> ? ?CapEff: ? ? ffffffffffffffff
> ? ?CapBnd: ? ? ffffffffffffffff
> ? ?root@albatros:~# ifconfig xfs
> ? ?xfs: error fetching interface information: Device not found
> ? ?root@albatros:~# lsmod | grep xfs
> ? ?xfs ? ? ? ? ? ? ? ? ? 745319 ?0
>
> Reference: https://lkml.org/lkml/2011/2/24/203
>
> Signed-off-by: Vasiliy Kulikov <[email protected]>
> ---
> ?v2 - use pr_err()
> ? ?- don't try to load $name if netdev-$name is loaded
>
> ?include/linux/netdevice.h | ? ?3 +++
> ?net/core/dev.c ? ? ? ? ? ?| ? 12 ++++++++++--
> ?net/ipv4/ip_gre.c ? ? ? ? | ? ?2 +-
> ?net/ipv4/ipip.c ? ? ? ? ? | ? ?2 +-
> ?net/ipv6/sit.c ? ? ? ? ? ?| ? ?2 +-
> ?5 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d971346..71caf7a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2392,6 +2392,9 @@ extern int netdev_notice(const struct net_device *dev, const char *format, ...)
> ?extern int netdev_info(const struct net_device *dev, const char *format, ...)
> ? ? ? ?__attribute__ ((format (printf, 2, 3)));
>
> +#define MODULE_ALIAS_NETDEV(device) \
> + ? ? ? MODULE_ALIAS("netdev-" device)
> +
> ?#if defined(DEBUG)
> ?#define netdev_dbg(__dev, format, args...) ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ?netdev_printk(KERN_DEBUG, __dev, format, ##args)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8ae6631..6561021 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1114,13 +1114,21 @@ EXPORT_SYMBOL(netdev_bonding_change);
> ?void dev_load(struct net *net, const char *name)
> ?{
> ? ? ? ?struct net_device *dev;
> + ? ? ? int no_module;
>
> ? ? ? ?rcu_read_lock();
> ? ? ? ?dev = dev_get_by_name_rcu(net, name);
> ? ? ? ?rcu_read_unlock();
>
> - ? ? ? if (!dev && capable(CAP_NET_ADMIN))
> - ? ? ? ? ? ? ? request_module("%s", name);
> + ? ? ? no_module = !dev;
> + ? ? ? if (no_module && capable(CAP_NET_ADMIN))
> + ? ? ? ? ? ? ? no_module = request_module("netdev-%s", name);
> + ? ? ? if (no_module && capable(CAP_SYS_MODULE)) {
> + ? ? ? ? ? ? ? if (!request_module("%s", name))
> + ? ? ? ? ? ? ? ? ? ? ? pr_err("Loading kernel module for a network device "
> +"with CAP_SYS_MODULE (deprecated). ?Use CAP_NET_ADMIN and alias netdev-%s "
> +"instead\n", name);
> + ? ? ? }
> ?}
> ?EXPORT_SYMBOL(dev_load);
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 6613edf..d1d0e2c 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
> ?MODULE_LICENSE("GPL");
> ?MODULE_ALIAS_RTNL_LINK("gre");
> ?MODULE_ALIAS_RTNL_LINK("gretap");
> -MODULE_ALIAS("gre0");
> +MODULE_ALIAS_NETDEV("gre0");
> diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> index 988f52f..a5f58e7 100644
> --- a/net/ipv4/ipip.c
> +++ b/net/ipv4/ipip.c
> @@ -913,4 +913,4 @@ static void __exit ipip_fini(void)
> ?module_init(ipip_init);
> ?module_exit(ipip_fini);
> ?MODULE_LICENSE("GPL");
> -MODULE_ALIAS("tunl0");
> +MODULE_ALIAS_NETDEV("tunl0");
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index 8ce38f1..d2c16e1 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -1290,4 +1290,4 @@ static int __init sit_init(void)
> ?module_init(sit_init);
> ?module_exit(sit_cleanup);
> ?MODULE_LICENSE("GPL");
> -MODULE_ALIAS("sit0");
> +MODULE_ALIAS_NETDEV("sit0");
> --
> 1.7.0.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
Quoting Eric Paris ([email protected]):
> On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <[email protected]> wrote:
...
> This patch is causing a bit of a problem in Fedora. The problem lies
Sorry, what exactly is the problem it is causing? I gather it's
spitting out printks? What exactly do the printks say? The patch
included at bottom checks for CAP_NET_ADMIN before checking for
CAP_SYS_MODULE, so these must be cases which historically always
quietly failed, and are now hitting the 'pr_err' which this patch
adds?
If it really is the capable(CAP_SYS_ADMIN) call itself, then there
must be a bug in the no_module logic in the patch. Every
case where it's currently checking for capable(CAP_SYS_ADMIN)
(and potentially auditing a failure) should be one where in the past
it would have (and currently it still would) spit out an audit msg
for the capable(CAP_NET_ADMIN) failure anyway.
If it's just the pr_err that's causing problems, then perhaps we
can turn it into a warn_once?
> mostly in the fact that we, by means of using SELinux, grant very very
> few domains CAP_SYS_MODULE (and we record when domains attempt to use
> this permission). Unlike most other distros in which uid==0 is for
> all intensive purposes == CAP_FULL_SET and there is no logging of
> failures to use capabilities. What happened is that as soon as we
> instituted this change we started getting SELinux denials because lots
> of domains (libvirt, udev, iw, NetworkManager) all the sudden started
> trying to use CAP_SYS_MODULE. It took me a minute to make sure this
> patch was the problem because I wasn't seeing any printk messages. I
> had to make some changes to the patch to print every time a task got
> into the CAP_SYS_MODULE case in order to get an idea what was causing
> the problem. I found on my machine I hit the problem 3 times trying
> to load "reg", "wifi0", and "virbr0" None of these are actual
> modules in userspace so the upcall failed.
>
> I'm trying to figure out how I should be dealing with this.
>
> My first idea is changing the capable(CAP_SYS_MODULE) into a
> has_capability_noaudit(). Which will not audit the access attempt.
> I'm not sure I like that since it uses the read cred, it doesn't set
> PF_SUPERPRIV, and it means we could likely miss recording a problem if
> a task is doing this intentionally...
>
> The next idea is I guess figuring out what's causing these and fix
> them there, but I'm not certain a good way to debug it. I know from
> our audit logs that wpa_supplicant is calling SIOCGIFINDEX which is
> causing one of these, libvirt is calling SIOCGIFFLAGS. I'm not sure
> what udev->iw is doing to trigger it's problem.....
>
> If the name in question is not coming from direct userspace request I
> guess I need help figuring out what is causing them....
>
> So while this patch might not necessarily be breaking things it
> certainly is not regression free and could potentially be breaking
> systems with fine grained capabilities controls....
>
> -Eric
>
> > ? ?root@albatros:~# capsh --drop=$(seq -s, 0 11),$(seq -s, 13 34) --
> > ? ?root@albatros:~# grep Cap /proc/$$/status
> > ? ?CapInh: ? ? 0000000000000000
> > ? ?CapPrm: ? ? fffffff800001000
> > ? ?CapEff: ? ? fffffff800001000
> > ? ?CapBnd: ? ? fffffff800001000
> > ? ?root@albatros:~# modprobe xfs
> > ? ?FATAL: Error inserting xfs
> > ? ?(/lib/modules/2.6.38-rc6-00001-g2bf4ca3/kernel/fs/xfs/xfs.ko): Operation not permitted
> > ? ?root@albatros:~# lsmod | grep xfs
> > ? ?root@albatros:~# ifconfig xfs
> > ? ?xfs: error fetching interface information: Device not found
> > ? ?root@albatros:~# lsmod | grep xfs
> > ? ?root@albatros:~# lsmod | grep sit
> > ? ?root@albatros:~# ifconfig sit
> > ? ?sit: error fetching interface information: Device not found
> > ? ?root@albatros:~# lsmod | grep sit
> > ? ?root@albatros:~# ifconfig sit0
> > ? ?sit0 ? ? ?Link encap:IPv6-in-IPv4
> > ? ? ? ? ? ? ?NOARP ?MTU:1480 ?Metric:1
> >
> > ? ?root@albatros:~# lsmod | grep sit
> > ? ?sit ? ? ? ? ? ? ? ? ? ?10457 ?0
> > ? ?tunnel4 ? ? ? ? ? ? ? ? 2957 ?1 sit
> >
> > For CAP_SYS_MODULE module loading is still relaxed:
> >
> > ? ?root@albatros:~# grep Cap /proc/$$/status
> > ? ?CapInh: ? ? 0000000000000000
> > ? ?CapPrm: ? ? ffffffffffffffff
> > ? ?CapEff: ? ? ffffffffffffffff
> > ? ?CapBnd: ? ? ffffffffffffffff
> > ? ?root@albatros:~# ifconfig xfs
> > ? ?xfs: error fetching interface information: Device not found
> > ? ?root@albatros:~# lsmod | grep xfs
> > ? ?xfs ? ? ? ? ? ? ? ? ? 745319 ?0
> >
> > Reference: https://lkml.org/lkml/2011/2/24/203
> >
> > Signed-off-by: Vasiliy Kulikov <[email protected]>
> > ---
> > ?v2 - use pr_err()
> > ? ?- don't try to load $name if netdev-$name is loaded
> >
> > ?include/linux/netdevice.h | ? ?3 +++
> > ?net/core/dev.c ? ? ? ? ? ?| ? 12 ++++++++++--
> > ?net/ipv4/ip_gre.c ? ? ? ? | ? ?2 +-
> > ?net/ipv4/ipip.c ? ? ? ? ? | ? ?2 +-
> > ?net/ipv6/sit.c ? ? ? ? ? ?| ? ?2 +-
> > ?5 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index d971346..71caf7a 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -2392,6 +2392,9 @@ extern int netdev_notice(const struct net_device *dev, const char *format, ...)
> > ?extern int netdev_info(const struct net_device *dev, const char *format, ...)
> > ? ? ? ?__attribute__ ((format (printf, 2, 3)));
> >
> > +#define MODULE_ALIAS_NETDEV(device) \
> > + ? ? ? MODULE_ALIAS("netdev-" device)
> > +
> > ?#if defined(DEBUG)
> > ?#define netdev_dbg(__dev, format, args...) ? ? ? ? ? ? ? ? ? ? \
> > ? ? ? ?netdev_printk(KERN_DEBUG, __dev, format, ##args)
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 8ae6631..6561021 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1114,13 +1114,21 @@ EXPORT_SYMBOL(netdev_bonding_change);
> > ?void dev_load(struct net *net, const char *name)
> > ?{
> > ? ? ? ?struct net_device *dev;
> > + ? ? ? int no_module;
> >
> > ? ? ? ?rcu_read_lock();
> > ? ? ? ?dev = dev_get_by_name_rcu(net, name);
> > ? ? ? ?rcu_read_unlock();
> >
> > - ? ? ? if (!dev && capable(CAP_NET_ADMIN))
> > - ? ? ? ? ? ? ? request_module("%s", name);
> > + ? ? ? no_module = !dev;
> > + ? ? ? if (no_module && capable(CAP_NET_ADMIN))
> > + ? ? ? ? ? ? ? no_module = request_module("netdev-%s", name);
> > + ? ? ? if (no_module && capable(CAP_SYS_MODULE)) {
> > + ? ? ? ? ? ? ? if (!request_module("%s", name))
> > + ? ? ? ? ? ? ? ? ? ? ? pr_err("Loading kernel module for a network device "
> > +"with CAP_SYS_MODULE (deprecated). ?Use CAP_NET_ADMIN and alias netdev-%s "
> > +"instead\n", name);
> > + ? ? ? }
> > ?}
> > ?EXPORT_SYMBOL(dev_load);
> >
> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > index 6613edf..d1d0e2c 100644
> > --- a/net/ipv4/ip_gre.c
> > +++ b/net/ipv4/ip_gre.c
> > @@ -1765,4 +1765,4 @@ module_exit(ipgre_fini);
> > ?MODULE_LICENSE("GPL");
> > ?MODULE_ALIAS_RTNL_LINK("gre");
> > ?MODULE_ALIAS_RTNL_LINK("gretap");
> > -MODULE_ALIAS("gre0");
> > +MODULE_ALIAS_NETDEV("gre0");
> > diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> > index 988f52f..a5f58e7 100644
> > --- a/net/ipv4/ipip.c
> > +++ b/net/ipv4/ipip.c
> > @@ -913,4 +913,4 @@ static void __exit ipip_fini(void)
> > ?module_init(ipip_init);
> > ?module_exit(ipip_fini);
> > ?MODULE_LICENSE("GPL");
> > -MODULE_ALIAS("tunl0");
> > +MODULE_ALIAS_NETDEV("tunl0");
> > diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> > index 8ce38f1..d2c16e1 100644
> > --- a/net/ipv6/sit.c
> > +++ b/net/ipv6/sit.c
> > @@ -1290,4 +1290,4 @@ static int __init sit_init(void)
> > ?module_init(sit_init);
> > ?module_exit(sit_cleanup);
> > ?MODULE_LICENSE("GPL");
> > -MODULE_ALIAS("sit0");
> > +MODULE_ALIAS_NETDEV("sit0");
> > --
> > 1.7.0.4
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at ?http://www.tux.org/lkml/
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-03-24 at 10:37 -0500, Serge E. Hallyn wrote:
> Quoting Eric Paris ([email protected]):
> > On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <[email protected]> wrote:
> ...
> > This patch is causing a bit of a problem in Fedora. The problem lies
>
> Sorry, what exactly is the problem it is causing? I gather it's
> spitting out printks? What exactly do the printks say? The patch
> included at bottom checks for CAP_NET_ADMIN before checking for
> CAP_SYS_MODULE, so these must be cases which historically always
> quietly failed, and are now hitting the 'pr_err' which this patch
> adds?
Not quite. SELinux logs every time an operation is denied. This patch
means that every time a module is requested which does not exist as
netdev-* we check CAP_SYS_MODULE. SELinux does not allow CAP_SYS_MODULE
and thus we get SELinux complaining that tasks are trying to load
modules. I do have one report from a user who claims this is breaking
his system, but I'm not sure I believe him as I have yet to see any
dmesg printk from the pr_err.
On my local system reproduce the SELinux denials on every boot as
something tries to autoload "reg", "wifi0", and "virbr0". I have no
modules which match these. Thus the first try for CAP_NET_ADMIN
+netdev-wifi0 fails. We then hit the CAP_SYS_MODULE check which SELinux
rejects and puts up a huge warning that someone is trying to load code
into the kernel. Big red flags. Even in permissive, where the
capable(CAP_SYS_MODULE) passes, we won't hit the pr_err() since there is
not module for "wifi"
I think there are 3 possibilities:
Change SELinux policy so as to not complain when udev/NM/libvirt try to
check CAP_SYS_MODULE, but that's a bad idea, since if they every try to
use init_module(2) we won't get denials.
Change this callsite to a _noaudit check. Which is better than above
but still not great since we wouldn't get a denial log if anybody had
tried to load xfs....
Figure out a way to stop the calls to "reg" "wifi0" and "virbr0" if they
don't exist.
I feel like the last one is the best way, but I don't know what a
solution could look like....
On Thu, 2011-03-24 at 14:03 -0400, Eric Paris wrote:
> On Thu, 2011-03-24 at 10:37 -0500, Serge E. Hallyn wrote:
> > Quoting Eric Paris ([email protected]):
> > > On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <[email protected]> wrote:
> > ...
> > > This patch is causing a bit of a problem in Fedora. The problem lies
> >
> > Sorry, what exactly is the problem it is causing? I gather it's
> > spitting out printks? What exactly do the printks say? The patch
> > included at bottom checks for CAP_NET_ADMIN before checking for
> > CAP_SYS_MODULE, so these must be cases which historically always
> > quietly failed, and are now hitting the 'pr_err' which this patch
> > adds?
>
> Not quite. SELinux logs every time an operation is denied. This patch
> means that every time a module is requested which does not exist as
> netdev-* we check CAP_SYS_MODULE. SELinux does not allow CAP_SYS_MODULE
> and thus we get SELinux complaining that tasks are trying to load
> modules.
This is exactly what would have happened before 2.6.32. Unfortunately
the incorrect behaviour introduced in 2.6.32 (CAP_NET_ADMIN allows you
to load any module installed in the usual place) is now present in
basically every current distribution, and it sounds like some of them
now assume that dev_load() no longer requires CAP_SYS_MODULE.
[...]
> I think there are 3 possibilities:
>
> Change SELinux policy so as to not complain when udev/NM/libvirt try to
> check CAP_SYS_MODULE, but that's a bad idea, since if they every try to
> use init_module(2) we won't get denials.
>
> Change this callsite to a _noaudit check. Which is better than above
> but still not great since we wouldn't get a denial log if anybody had
> tried to load xfs....
There are no evil bits in device or module names, so the kernel can't
tell whether the attempt should be logged. But then, adding some sort
of policy option for whether to audit CAP_SYS_MODULE use here strikes me
as over-engineering. Just make a decision based on what SELinux users
seem to prefer.
> Figure out a way to stop the calls to "reg" "wifi0" and "virbr0" if they
> don't exist.
>
> I feel like the last one is the best way, but I don't know what a
> solution could look like....
This really has to be done in userland, where these names are being
invented. Though I suspect the usual way to check whether an interface
exists would be SIOCGIFINDEX, which calls dev_load()! An alternate
would be to check whether /sys/class/net/<name> exists, but I seem to
recall that /sys/class is somewhat deprecated.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Quoting Ben Hutchings ([email protected]):
> On Thu, 2011-03-24 at 14:03 -0400, Eric Paris wrote:
> > Not quite. SELinux logs every time an operation is denied. This patch
> > means that every time a module is requested which does not exist as
Ah. I see.
...
> > I think there are 3 possibilities:
...(3)
> > Figure out a way to stop the calls to "reg" "wifi0" and "virbr0" if they
> > don't exist.
> >
> > I feel like the last one is the best way, but I don't know what a
> > solution could look like....
>
> This really has to be done in userland, where these names are being
> invented. Though I suspect the usual way to check whether an interface
> exists would be SIOCGIFINDEX, which calls dev_load()! An alternate
> would be to check whether /sys/class/net/<name> exists, but I seem to
> recall that /sys/class is somewhat deprecated.
Of course this will mean pain for users until distributions have
updated to userspace which is doing this, but yeah, this clearly seems
like the best way. (And the only sane one.)
thanks,
-serge
On Thu, 24 Mar 2011 15:26:34 -0500
"Serge E. Hallyn" <[email protected]> wrote:
> Quoting Ben Hutchings ([email protected]):
> > On Thu, 2011-03-24 at 14:03 -0400, Eric Paris wrote:
> > > Not quite. SELinux logs every time an operation is denied. This patch
> > > means that every time a module is requested which does not exist as
>
> Ah. I see.
>
> ...
>
> > > I think there are 3 possibilities:
>
> ...(3)
>
> > > Figure out a way to stop the calls to "reg" "wifi0" and "virbr0" if they
> > > don't exist.
> > >
> > > I feel like the last one is the best way, but I don't know what a
> > > solution could look like....
> >
> > This really has to be done in userland, where these names are being
> > invented. Though I suspect the usual way to check whether an interface
> > exists would be SIOCGIFINDEX, which calls dev_load()! An alternate
> > would be to check whether /sys/class/net/<name> exists, but I seem to
> > recall that /sys/class is somewhat deprecated.
>
> Of course this will mean pain for users until distributions have
> updated to userspace which is doing this, but yeah, this clearly seems
> like the best way. (And the only sane one.)
>
This breaks for many of the tunneling protocols, that rely on
autoload for names like "sit0"
From: Stephen Hemminger <[email protected]>
Date: Thu, 24 Mar 2011 14:39:44 -0700
> This breaks for many of the tunneling protocols, that rely on
> autoload for names like "sit0"
Frankly I'm very disappointed in the fallout this has been causing.
Everyone supporting this change, get real, and admit it doing in fact
cause a serious regression.
If you can't get past that simple fact, you cannot discuss this issue
intelligently.
You can't say "userland will fix things up"
Because we're never supposed to break userland in the first place.
There is simply no excuse for this and I want this change reverted
both in Linus's tree and in -stable.
On Thu, Mar 24, 2011 at 02:46:28PM -0700, David Miller wrote:
> From: Stephen Hemminger <[email protected]>
> Date: Thu, 24 Mar 2011 14:39:44 -0700
>
> > This breaks for many of the tunneling protocols, that rely on
> > autoload for names like "sit0"
>
> Frankly I'm very disappointed in the fallout this has been causing.
>
> Everyone supporting this change, get real, and admit it doing in fact
> cause a serious regression.
>
> If you can't get past that simple fact, you cannot discuss this issue
> intelligently.
>
> You can't say "userland will fix things up"
>
> Because we're never supposed to break userland in the first place.
>
> There is simply no excuse for this and I want this change reverted
> both in Linus's tree and in -stable.
Ok, as I totally got lost in the thread here, what is the git commit id
of the patch to revert?
thanks,
greg k-h
Quoting David Miller ([email protected]):
> From: Stephen Hemminger <[email protected]>
> Date: Thu, 24 Mar 2011 14:39:44 -0700
>
> > This breaks for many of the tunneling protocols, that rely on
> > autoload for names like "sit0"
>
> Frankly I'm very disappointed in the fallout this has been causing.
>
> Everyone supporting this change, get real, and admit it doing in fact
> cause a serious regression.
Sorry, I thought this was causing some extra audit messages but no
actual breakage?
> If you can't get past that simple fact, you cannot discuss this issue
> intelligently.
>
> You can't say "userland will fix things up"
>
> Because we're never supposed to break userland in the first place.
>
> There is simply no excuse for this and I want this change reverted
> both in Linus's tree and in -stable.
Eric, in this particular case, since we've already done a
'capable(CAP_NET_ADMIN)', I woudl argue that doing the check
for CAP_SYS_ADMIN without auditing failure (even if it requires
a new helper in capability.c) isn't horrible. Thoughts?
-serge
On Thu, 2011-03-24 at 16:57 -0500, Serge E. Hallyn wrote:
> Quoting David Miller ([email protected]):
> > From: Stephen Hemminger <[email protected]>
> > Date: Thu, 24 Mar 2011 14:39:44 -0700
> >
> > > This breaks for many of the tunneling protocols, that rely on
> > > autoload for names like "sit0"
> >
> > Frankly I'm very disappointed in the fallout this has been causing.
> >
> > Everyone supporting this change, get real, and admit it doing in fact
> > cause a serious regression.
>
> Sorry, I thought this was causing some extra audit messages but no
> actual breakage?
I've got one report of someone claiming their system broke, but I'm not
convinced I believe it since his dmesg didn't show the magic pr_err()
when it should have. It's certainly possible this can break someone in
a system which uses fine grained capabilities controls, but I agree it's
pretty unlikely. My biggest personal concern is that I have a whole
darn bunch of new scary messages which are popping out of people's
computers since they don't have CAP_SYS_MODULE. While I can silence
them, it's going to hide use of init_module() directly as well, which I
really don't want to hide from the scary logs....
> > If you can't get past that simple fact, you cannot discuss this issue
> > intelligently.
> >
> > You can't say "userland will fix things up"
> >
> > Because we're never supposed to break userland in the first place.
> >
> > There is simply no excuse for this and I want this change reverted
> > both in Linus's tree and in -stable.
>
> Eric, in this particular case, since we've already done a
> 'capable(CAP_NET_ADMIN)', I woudl argue that doing the check
> for CAP_SYS_ADMIN without auditing failure (even if it requires
> a new helper in capability.c) isn't horrible. Thoughts?
s/CAP_SYS_ADMIN/CAP_SYS_MODULE/
I can do that. It was actually my #2 suggestion. But, I'm certainly
willing to put some of the burden on userspace. SELinux policy is a
userspace construct and we often force other userspace applications to
fix things they do poorly (even if it gets us a rep for being
'difficult') Non-SELinux systems aren't going to see this problem,
because basically noone else I know of tries to enforce any kind of
capabilities sets other than all or none, so you'll never see
CAP_NET_ADMIN without CAP_SYS_MODULE.
I guess what it comes down to is that I'm happy to break Fedora user's
with SELinux if in the end it gets us a better system. I'd be happy to
just rip the whole CAP_SYS_MODULE portion out and blame it on SELinux,
but I know that's not what upstream does. So given what we have today I
personally would push for a no_audit() interface rather than a complete
revert. (or maybe a compile option so I can turn off the fallback
altogether and force people to come into compliance)
-Eric
On Thu, Mar 24, 2011 at 14:46 -0700, David Miller wrote:
> You can't say "userland will fix things up"
>
> Because we're never supposed to break userland in the first place.
I admit that the patch breaks things.
But the thing is that kernel changes _are_ breaking userspace here and
there, not only by such obvious policy changes, but by indirect changes.
Note that the patch that changed CAP_SYS_MODULE to CAP_NET_ADMIN has
broken userspace behavior too - one could load modules with
CAP_SYS_MODULE without CAP_NET_ADMIN via "ifconfig wifi0" and after the
patch it could not.
Look at this patch:
http://patchwork.ozlabs.org/patch/42148/
It breaks userspace tools too - one might run LSM in learning mode to
create a profile for netfilter configuring, saw it didn't need any CAP_*
and totally denied them in the profile. After many years (the bug was
fixed after 5+ years!) of good work it was broken by the patch. The same
with plenty of patches that introduce different checks in places where
there were no permission checks at all or these checks were broken.
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments