| Message ID | 20110225151414.GA5211@albatros |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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, Valdis.Kletnieks@vt.edu 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,
On Fri, 2011-02-25 at 12:25 -0500, Valdis.Kletnieks@vt.edu 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.
From: Vasiliy Kulikov <segoon@openwall.com> 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? -- 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 Fri, Feb 25, 2011 at 10:47 -0800, David Miller wrote: > From: Vasiliy Kulikov <segoon@openwall.com> > 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,
From: Vasiliy Kulikov <segoon@openwall.com> Date: Fri, 25 Feb 2011 22:02:05 +0300 > On Fri, Feb 25, 2011 at 10:47 -0800, David Miller wrote: >> From: Vasiliy Kulikov <segoon@openwall.com> >> 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. -- 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 Fri, 2011-02-25 at 11:05 -0800, David Miller wrote: > From: Vasiliy Kulikov <segoon@openwall.com> > Date: Fri, 25 Feb 2011 22:02:05 +0300 > > > On Fri, Feb 25, 2011 at 10:47 -0800, David Miller wrote: > >> From: Vasiliy Kulikov <segoon@openwall.com> > >> 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.
From: Ben Hutchings <bhutchings@solarflare.com> 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. -- 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 Fri, 2011-02-25 at 11:16 -0800, David Miller wrote: > From: Ben Hutchings <bhutchings@solarflare.com> > 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.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Fri, 25 Feb 2011 19:30:16 +0000 > On Fri, 2011-02-25 at 11:16 -0800, David Miller wrote: >> From: Ben Hutchings <bhutchings@solarflare.com> >> 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. -- 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 Fri, 2011-02-25 at 11:43 -0800, David Miller wrote: > From: Ben Hutchings <bhutchings@solarflare.com> > Date: Fri, 25 Feb 2011 19:30:16 +0000 > > > On Fri, 2011-02-25 at 11:16 -0800, David Miller wrote: > >> From: Ben Hutchings <bhutchings@solarflare.com> > >> 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.
From: Ben Hutchings <bhutchings@solarflare.com> 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. -- 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
David, On Fri, Feb 25, 2011 at 11:16 -0800, David Miller wrote: > From: Ben Hutchings <bhutchings@solarflare.com> > 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,
From: Vasiliy Kulikov <segoon@openwall.com> 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. -- 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: Vasiliy Kulikov <segoon@openwall.com> 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. -- 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
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");
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 <segoon@openwall.com> --- 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(-)