| Message ID | 20110301213313.GA6507@albatros |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <segoon@openwall.com> This looks much saner :) Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> /mjt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <segoon@openwall.com> Looks good; thanks for sorting this out. Acked-by: Kees Cook <kees.cook@canonical.com> -Kees
I am probably missing something, but shouldn't the existing MODULE_ALIASes stay? Vasiliy Kulikov <segoon@openwall.com> 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
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.
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
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.
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
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
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 <segoon@openwall.com> > > This looks much saner :) > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> Is there any reason why it is not yet merged? I've answered to Jake Edge that it is not a regression. Thanks,
From: Vasiliy Kulikov <segoon@openwall.com> 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 <segoon@openwall.com> >> >> This looks much saner :) >> >> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > > 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 <davem@davemloft.net> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <davem@davemloft.net> I can submit it via my tree.
James,
On Thu, Mar 10, 2011 at 09:53 +1100, James Morris wrote:
> I can submit it via my tree.
Thank you!
On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <segoon@openwall.com> 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 <segoon@openwall.com> > --- > 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 majordomo@vger.kernel.org > 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric Paris (eparis@parisplace.org): > On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <segoon@openwall.com> 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 <segoon@openwall.com> > > --- > > 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 majordomo@vger.kernel.org > > 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 majordomo@vger.kernel.org > 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 (eparis@parisplace.org): > > On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <segoon@openwall.com> 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.... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 (eparis@parisplace.org): > > > On Tue, Mar 1, 2011 at 4:33 PM, Vasiliy Kulikov <segoon@openwall.com> 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.
Quoting Ben Hutchings (bhutchings@solarflare.com): > 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" <serge.hallyn@ubuntu.com> wrote: > Quoting Ben Hutchings (bhutchings@solarflare.com): > > 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" -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Hemminger <shemminger@vyatta.com> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting David Miller (davem@davemloft.net): > From: Stephen Hemminger <shemminger@vyatta.com> > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 24, 2011 at 02:46:28PM -0700, David Miller wrote: > From: Stephen Hemminger <shemminger@vyatta.com> > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-03-24 at 16:57 -0500, Serge E. Hallyn wrote: > Quoting David Miller (davem@davemloft.net): > > From: Stephen Hemminger <shemminger@vyatta.com> > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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");
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 <segoon@openwall.com> --- 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(-)