| Message ID | 1282567801-2673-1-git-send-email-luciano.coelho@nokia.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Aug 23, 2010 at 8:50 PM, <luciano.coelho@nokia.com> wrote: > From: Luciano Coelho <luciano.coelho@nokia.com> > > Add a module parameter that allows the required security capability to > change the conditions from userspace to be specified. By default the > module will require the CAP_NET_ADMIN capability. > > Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com> > --- > net/netfilter/xt_condition.c | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c > index 06205aa..fd279e5 100644 > --- a/net/netfilter/xt_condition.c > +++ b/net/netfilter/xt_condition.c > @@ -29,11 +29,13 @@ > #include <linux/netfilter/xt_condition.h> > #include <net/netns/generic.h> > #include <asm/uaccess.h> > +#include <linux/capability.h> > > /* Defaults, these can be overridden on the module command-line. */ > static unsigned int condition_list_perms = S_IRUGO | S_IWUSR; > static unsigned int condition_uid_perms = 0; > static unsigned int condition_gid_perms = 0; > +static unsigned int condition_capabilities = CAP_NET_ADMIN; > > MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca>"); > MODULE_AUTHOR("Massimiliano Hofer <max@nucleus.it>"); > @@ -47,6 +49,8 @@ module_param(condition_uid_perms, uint, S_IRUSR | S_IWUSR); > MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files"); > module_param(condition_gid_perms, uint, S_IRUSR | S_IWUSR); > MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files"); > +module_param(condition_capabilities, uint, CAP_NET_ADMIN); > +MODULE_PARM_DESC(condition_capabilities, "default capabilities required to change /proc/net/nf_condition/* files"); It is strange that we set security policy in this way. Maybe the permission of the proc file is enough in this case. > MODULE_ALIAS("ipt_condition"); > MODULE_ALIAS("ip6t_condition"); > > @@ -88,6 +92,12 @@ static int condition_proc_write(struct file *file, const char __user *input, > char buf[sizeof("+037777777777")]; > unsigned long long value; > > + if (!capable(condition_capabilities)) { > + pr_debug("not enough capabilities (requires %0X)\n", > + condition_capabilities); > + return -EPERM; > + } > + > if (length == 0) > return 0; > > -- > 1.7.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, 2010-08-23 at 15:37 +0200, ext Changli Gao wrote: > On Mon, Aug 23, 2010 at 8:50 PM, <luciano.coelho@nokia.com> wrote: > > From: Luciano Coelho <luciano.coelho@nokia.com> > > > > Add a module parameter that allows the required security capability to > > change the conditions from userspace to be specified. By default the > > module will require the CAP_NET_ADMIN capability. > > > > Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com> > > --- > > net/netfilter/xt_condition.c | 10 ++++++++++ > > 1 files changed, 10 insertions(+), 0 deletions(-) > > > > diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c > > index 06205aa..fd279e5 100644 > > --- a/net/netfilter/xt_condition.c > > +++ b/net/netfilter/xt_condition.c > > @@ -29,11 +29,13 @@ > > #include <linux/netfilter/xt_condition.h> > > #include <net/netns/generic.h> > > #include <asm/uaccess.h> > > +#include <linux/capability.h> > > > > /* Defaults, these can be overridden on the module command-line. */ > > static unsigned int condition_list_perms = S_IRUGO | S_IWUSR; > > static unsigned int condition_uid_perms = 0; > > static unsigned int condition_gid_perms = 0; > > +static unsigned int condition_capabilities = CAP_NET_ADMIN; > > > > MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca>"); > > MODULE_AUTHOR("Massimiliano Hofer <max@nucleus.it>"); > > @@ -47,6 +49,8 @@ module_param(condition_uid_perms, uint, S_IRUSR | S_IWUSR); > > MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files"); > > module_param(condition_gid_perms, uint, S_IRUSR | S_IWUSR); > > MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files"); > > +module_param(condition_capabilities, uint, CAP_NET_ADMIN); > > +MODULE_PARM_DESC(condition_capabilities, "default capabilities required to change /proc/net/nf_condition/* files"); > > It is strange that we set security policy in this way. Maybe the > permission of the proc file is enough in this case. Yes, that is another way to do it. But in our device we use security capabilities more extensively than normal file permissions. That's why we need this. If this is too restrictive (ie. having CAP_NET_ADMIN) for most users, we could change the default value to no capabilities needed. Then we can set CAP_NET_ADMIN when loading the module.
On Monday 2010-08-23 15:42, Luciano Coelho wrote: >> > /* Defaults, these can be overridden on the module command-line. */ >> > static unsigned int condition_list_perms = S_IRUGO | S_IWUSR; >> > static unsigned int condition_uid_perms = 0; >> > static unsigned int condition_gid_perms = 0; >> > +static unsigned int condition_capabilities = CAP_NET_ADMIN; >> > >> It is strange that we set security policy in this way. Maybe the >> permission of the proc file is enough in this case. > >Yes, that is another way to do it. But in our device we use security >capabilities more extensively than normal file permissions. That's why >we need this. > >If this is too restrictive (ie. having CAP_NET_ADMIN) for most users, we >could change the default value to no capabilities needed. Then we can >set CAP_NET_ADMIN when loading the module. But it looks as strange as the Yama code attempt. This is the one time where I would personally be looking into SELinux, or perhaps SMACK if the former is too complex, to whether _t'ing off procfs is possible. -- 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 Mon, 2010-08-23 at 16:34 +0200, ext Jan Engelhardt wrote: > On Monday 2010-08-23 15:42, Luciano Coelho wrote: > >> > /* Defaults, these can be overridden on the module command-line. */ > >> > static unsigned int condition_list_perms = S_IRUGO | S_IWUSR; > >> > static unsigned int condition_uid_perms = 0; > >> > static unsigned int condition_gid_perms = 0; > >> > +static unsigned int condition_capabilities = CAP_NET_ADMIN; > >> > > >> It is strange that we set security policy in this way. Maybe the > >> permission of the proc file is enough in this case. > > > >Yes, that is another way to do it. But in our device we use security > >capabilities more extensively than normal file permissions. That's why > >we need this. > > > >If this is too restrictive (ie. having CAP_NET_ADMIN) for most users, we > >could change the default value to no capabilities needed. Then we can > >set CAP_NET_ADMIN when loading the module. > > But it looks as strange as the Yama code attempt. What is so strange about it? Is it because it's possible to set the capability requirement from modprobe arguments? The capability check already exists in at least in nfnetlink, where it checks for received messages for the CAP_NET_ADMIN capability. Is it strange because we're checking for the capability when someone tries to write to a file? > This is the one time > where I would personally be looking into SELinux, or perhaps SMACK if > the former is too complex, to whether _t'ing off procfs is possible. Yeah, but it's not up to me to decide this. We have one entire team dedicated to figuring out how to ensure "security" in our device. It was that team who advised us to protect this file by checking for CAP_NET_ADMIN.
On Monday 2010-08-23 20:45, Luciano Coelho wrote: >> But it looks as strange as the Yama code attempt. > >What is so strange about it? Is it because it's possible to set the >capability requirement from modprobe arguments? The capability check >already exists in at least in nfnetlink, where it checks for received >messages for the CAP_NET_ADMIN capability. Is it strange because we're >checking for the capability when someone tries to write to a file? It is strange that you check this capability from a module focused on packet handling. For lack of a better example, it's as if you tried to check the uid of the file, the latter of which is better left to the routines in fs/. >> This is the one time >> where I would personally be looking into SELinux, or perhaps SMACK if >> the former is too complex, to whether _t'ing off procfs is possible. > >Yeah, but it's not up to me to decide this. We have one entire team >dedicated to figuring out how to ensure "security" in our device. It >was that team who advised us to protect this file by checking for >CAP_NET_ADMIN. You can do whatever you want with your product. I am just saying this does not look like kernel material, and I suppose it won't go well with the maintainers up the chain either. -- 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 Mon, 2010-08-23 at 20:58 +0200, ext Jan Engelhardt wrote: > On Monday 2010-08-23 20:45, Luciano Coelho wrote: > >> But it looks as strange as the Yama code attempt. > > > >What is so strange about it? Is it because it's possible to set the > >capability requirement from modprobe arguments? The capability check > >already exists in at least in nfnetlink, where it checks for received > >messages for the CAP_NET_ADMIN capability. Is it strange because we're > >checking for the capability when someone tries to write to a file? > > It is strange that you check this capability from a module focused on > packet handling. For lack of a better example, it's as if you tried > to check the uid of the file, the latter of which is better left to > the routines in fs/. Okay, thanks for the explanation! I'll have to figure something else out then. What I don't understand is that I see lots of components, which have nothing to do with security, making this kind of checks. Most of them (if not all) are checking for input from userspace where they provide their own interfaces (eg. ioctl calls, netlink messages). I can see that ip_tables checks for this capability when it gets "set" socket calls. Now, with the xt_condition, we're opening a new route from userspace to the kernel and I think it might be a good idea to protect that too. It's kind of useless to protect someone without CAP_NET_ADMIN from creating a condition rule if it is possible to change the condition from userspace without any capability protection. > >> This is the one time > >> where I would personally be looking into SELinux, or perhaps SMACK if > >> the former is too complex, to whether _t'ing off procfs is possible. > > > >Yeah, but it's not up to me to decide this. We have one entire team > >dedicated to figuring out how to ensure "security" in our device. It > >was that team who advised us to protect this file by checking for > >CAP_NET_ADMIN. > > You can do whatever you want with your product. I am just saying this > does not look like kernel material, and I suppose it won't go well > with the maintainers up the chain either. Yeah, my company can do whatever they want with the product. But my role here is to work with upstream, so if you guys think that what I'm doing is total bulls*it, I'll have to go back to the drawing board and restart my work ;) Again, I must stress that I really appreciate your comments and help. I know I can get good advice from people who are millions of times more experienced in this area than I am (and yes, that's *you* ;) -- and this is one of the various reasons I send my patches to netfilter-devel in the first place. Another reason is that I want to contribute my work (however shitty it is) to an eventual benefit for the community as a whole. :)
On Tuesday 2010-08-24 09:00, Luciano Coelho wrote: >> >>It is strange that you check this capability from a module focused >>on packet handling. For lack of a better example, it's as if you >>tried to check the uid of the file, the latter of which is better >>left to the routines in fs/. > >What I don't understand is that I see lots of components, which have >nothing to do with security, making this kind of checks. Most of >them (if not all) are checking for input from userspace where they >provide their own interfaces (eg. ioctl calls, netlink messages). >[..] Now, with the xt_condition, we're opening a new route from >userspace to the kernel and I think it might be a good idea to >protect that too. Indeed so. But you did not invent any new interface. You are reusing files, which can be protected by DAC modes, or LSMs doing funky-stuff. xt_{condition,recent,..} already implement file modes, but does it check for it? Well no, because fs/namei.c does it for them. As for LSMs, well, I hope they do cater for testing for capability bits. >It's kind of useless to protect someone without CAP_NET_ADMIN from >creating a condition rule if it is possible to change the condition from >userspace without any capability protection. Certainly not. An administrator may create a condition rule and thus procfs entry, but the rule may be sufficiently safe and/or integrated that a subordinate may be given permission to control things in a limited fashion on a per-need basis. One I use personally is -A FORWARD -m condition --condition windows -s 192.168.100.0/25 -i eth1 -o eth0 -j ACCEPT -P FORWARD DROP chown jengelh /proc/net/nf_condition/windows; The presence of such rule would indicate that the administrator generally allows Windows machines out; but personal paranoia defaults to rejecting that unless explicitly enabled. (Gives the reassurance that they won't succeed talking home unless Internet connectivity is explicitly needed by the user.) -- 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
Hi Jan, Thanks for your reply. On Tue, 2010-08-24 at 09:32 +0200, ext Jan Engelhardt wrote: > On Tuesday 2010-08-24 09:00, Luciano Coelho wrote: > >> > >>It is strange that you check this capability from a module focused > >>on packet handling. For lack of a better example, it's as if you > >>tried to check the uid of the file, the latter of which is better > >>left to the routines in fs/. > > > >What I don't understand is that I see lots of components, which have > >nothing to do with security, making this kind of checks. Most of > >them (if not all) are checking for input from userspace where they > >provide their own interfaces (eg. ioctl calls, netlink messages). > >[..] Now, with the xt_condition, we're opening a new route from > >userspace to the kernel and I think it might be a good idea to > >protect that too. > > Indeed so. But you did not invent any new interface. You are reusing > files, which can be protected by DAC modes, or LSMs doing > funky-stuff. xt_{condition,recent,..} already implement file modes, > but does it check for it? Well no, because fs/namei.c does it for > them. As for LSMs, well, I hope they do cater for testing for > capability bits. Thanks, I'll investigate all this a bit more and contact our security people again. I dug deeper into the code and I can see that /sys/net has capability checks (implemented in netdev_store() in net-sysfs.c) and nobody without CAP_NET_ADMIN will be able to write to the files there. But in procfs I couldn't see anything similar and anyone with file write permissions can modify the files in /proc/net/*. > >It's kind of useless to protect someone without CAP_NET_ADMIN from > >creating a condition rule if it is possible to change the condition from > >userspace without any capability protection. > > Certainly not. An administrator may create a condition rule and thus > procfs entry, but the rule may be sufficiently safe and/or integrated > that a subordinate may be given permission to control things in a > limited fashion on a per-need basis. One I use personally is > > -A FORWARD -m condition --condition windows -s > 192.168.100.0/25 -i eth1 -o eth0 -j ACCEPT > -P FORWARD DROP > > chown jengelh /proc/net/nf_condition/windows; > > The presence of such rule would indicate that the administrator > generally allows Windows machines out; but personal paranoia defaults > to rejecting that unless explicitly enabled. (Gives the reassurance > that they won't succeed talking home unless Internet connectivity is > explicitly needed by the user.) I see, this is a good example. :) I know that his works (with simple DAC control), but I think we're looking into a bit more security than that. I have to discuss this with our platform security people. Again, thanks for your explanations.
On Wednesday 2010-08-25 09:09, Luciano Coelho wrote: >> >> Indeed so. But you did not invent any new interface. You are reusing >> files, which can be protected by DAC modes, or LSMs doing >> funky-stuff. xt_{condition,recent,..} already implement file modes, >> but does it check for it? Well no, because fs/namei.c does it for >> them. As for LSMs, well, I hope they do cater for testing for >> capability bits. > >I dug deeper into the code and I can see that /sys/net has capability >checks (implemented in netdev_store() in net-sysfs.c) and nobody without >CAP_NET_ADMIN will be able to write to the files there. But in procfs I >couldn't see anything similar and anyone with file write permissions can >modify the files in /proc/net/*. I did not say there was. fs/ will handle the DAC part, and if you wanted MAC/capability checking/other sparkles, an LSM sounds like the right spot. -- 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
Hi Jan, On Wed, 2010-08-25 at 11:04 +0200, ext Jan Engelhardt wrote: > On Wednesday 2010-08-25 09:09, Luciano Coelho wrote: > >> > >> Indeed so. But you did not invent any new interface. You are reusing > >> files, which can be protected by DAC modes, or LSMs doing > >> funky-stuff. xt_{condition,recent,..} already implement file modes, > >> but does it check for it? Well no, because fs/namei.c does it for > >> them. As for LSMs, well, I hope they do cater for testing for > >> capability bits. > > > >I dug deeper into the code and I can see that /sys/net has capability > >checks (implemented in netdev_store() in net-sysfs.c) and nobody without > >CAP_NET_ADMIN will be able to write to the files there. But in procfs I > >couldn't see anything similar and anyone with file write permissions can > >modify the files in /proc/net/*. > > I did not say there was. fs/ will handle the DAC part, and if you > wanted MAC/capability checking/other sparkles, an LSM sounds like > the right spot. There is one fundamental difference that I forgot to point out. While in most cases netfilters are used by _sysadmins_ to route and control traffic, we are using it in a very different way. Our goal is to check throughput and other connection characteristics to control power saving and other features in an embedded device. In such devices we typically have one single user and the sysadmin is us who define all the rules and pre-install them in the device beforehand. We need to prevent binaries rather than users from doing bad things. Using basic DAC extensively to do this may end up getting messy. That's what I tried to say when I said that we have a security team taking care of this. They are implementing solutions to make the product more secure, defending it against malware, misuse, attacks and other such things. In this specific case, security-wise, we are trying to prevent some bogus or malicious application from changing our netfilter rules and causing havoc. LSM doesn't seem to be an option, here I quote Juhani (my colleague from our security team): > The problem with capabilites is that they are assigned to binaries, not > users. Kind of a setuid-mechanism, really. In our embedded environment > that makes a lot of sense, but in a server-type environment with > multiple users and a competent sysadmin, not so much. In such an > environment using a LSM might also actually make sense. But for us it's > not an option, mostly because LSMs are not stackable - you can have only > one effective at any time - and I'm afraid we have already reserved some > of the LSM hooks. Maybe Juhani can clarify this a bit more. One other idea that Juhani had was to add an option to the condition match/target where the capability requiremets could be set, instead of checking them by default. If nothing is specified, everything still works as before (no caps checks). Or even a Kconfig option? I don't see any other option for checking the capabilities besides doing it in the xt_condition module itself. Or maybe move the condition variable from procfs to sysfs and use the existing netdev_store protection that is implemented there? The problem with the latter is the same old story we discussed with the idletimer target: where in the kernel object tree should the variable be added?
Am 27.08.2010 09:55, schrieb Luciano Coelho: > That's what I tried to say when I said that we have a security team > taking care of this. They are implementing solutions to make the > product more secure, defending it against malware, misuse, attacks and > other such things. In this specific case, security-wise, we are trying > to prevent some bogus or malicious application from changing our > netfilter rules and causing havoc. > > LSM doesn't seem to be an option, here I quote Juhani (my colleague from > our security team): > >> The problem with capabilites is that they are assigned to binaries, not >> users. Kind of a setuid-mechanism, really. In our embedded environment >> that makes a lot of sense, but in a server-type environment with >> multiple users and a competent sysadmin, not so much. In such an >> environment using a LSM might also actually make sense. But for us it's >> not an option, mostly because LSMs are not stackable - you can have only >> one effective at any time - and I'm afraid we have already reserved some >> of the LSM hooks. > > Maybe Juhani can clarify this a bit more. > > One other idea that Juhani had was to add an option to the condition > match/target where the capability requiremets could be set, instead of > checking them by default. If nothing is specified, everything still > works as before (no caps checks). Or even a Kconfig option? I agree with Jan, adding module parameters to control permission checks or capabilities seems like a bad precedent. -- 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, 2010-09-15 at 22:14 +0200, ext Patrick McHardy wrote: > Am 27.08.2010 09:55, schrieb Luciano Coelho: > > That's what I tried to say when I said that we have a security team > > taking care of this. They are implementing solutions to make the > > product more secure, defending it against malware, misuse, attacks and > > other such things. In this specific case, security-wise, we are trying > > to prevent some bogus or malicious application from changing our > > netfilter rules and causing havoc. > > > > LSM doesn't seem to be an option, here I quote Juhani (my colleague from > > our security team): > > > >> The problem with capabilites is that they are assigned to binaries, not > >> users. Kind of a setuid-mechanism, really. In our embedded environment > >> that makes a lot of sense, but in a server-type environment with > >> multiple users and a competent sysadmin, not so much. In such an > >> environment using a LSM might also actually make sense. But for us it's > >> not an option, mostly because LSMs are not stackable - you can have only > >> one effective at any time - and I'm afraid we have already reserved some > >> of the LSM hooks. > > > > Maybe Juhani can clarify this a bit more. > > > > One other idea that Juhani had was to add an option to the condition > > match/target where the capability requiremets could be set, instead of > > checking them by default. If nothing is specified, everything still > > works as before (no caps checks). Or even a Kconfig option? > > I agree with Jan, adding module parameters to control permission checks > or capabilities seems like a bad precedent. Okay, the idea is now officially dropped. We'll use normal ACL to control access to the conditions. Thanks for your comments and sorry for trying to push ;)
diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c index 06205aa..fd279e5 100644 --- a/net/netfilter/xt_condition.c +++ b/net/netfilter/xt_condition.c @@ -29,11 +29,13 @@ #include <linux/netfilter/xt_condition.h> #include <net/netns/generic.h> #include <asm/uaccess.h> +#include <linux/capability.h> /* Defaults, these can be overridden on the module command-line. */ static unsigned int condition_list_perms = S_IRUGO | S_IWUSR; static unsigned int condition_uid_perms = 0; static unsigned int condition_gid_perms = 0; +static unsigned int condition_capabilities = CAP_NET_ADMIN; MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca>"); MODULE_AUTHOR("Massimiliano Hofer <max@nucleus.it>"); @@ -47,6 +49,8 @@ module_param(condition_uid_perms, uint, S_IRUSR | S_IWUSR); MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files"); module_param(condition_gid_perms, uint, S_IRUSR | S_IWUSR); MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files"); +module_param(condition_capabilities, uint, CAP_NET_ADMIN); +MODULE_PARM_DESC(condition_capabilities, "default capabilities required to change /proc/net/nf_condition/* files"); MODULE_ALIAS("ipt_condition"); MODULE_ALIAS("ip6t_condition"); @@ -88,6 +92,12 @@ static int condition_proc_write(struct file *file, const char __user *input, char buf[sizeof("+037777777777")]; unsigned long long value; + if (!capable(condition_capabilities)) { + pr_debug("not enough capabilities (requires %0X)\n", + condition_capabilities); + return -EPERM; + } + if (length == 0) return 0;