[PATCH 4/7] add missing module_alias_subsys

[PATCH 4/7] add missing module_alias_subsys

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

Add missing module alias. This is a must to load ctnetlink on demand.
For example, the conntrack tool will fail if the module isn't loaded.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

[-- Attachment #2: 07-alias.patch --]
[-- Type: text/plain, Size: 719 bytes --]

Add missing module alias. This is a must to load ctnetlink on demand. For 
example, the conntrack tool will fail if the module isn't loaded.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
--- net-2.6.git.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2005-11-04 17:55:24.000000000 +0100
+++ net-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2005-11-04 18:31:04.000000000 +0100
@@ -1538,6 +1538,8 @@ static struct nfnetlink_subsystem ctnl_e
 	.cb				= ctnl_exp_cb,
 };
 
+MODULE_ALIAS_NFNL_SUBSYS(NFNL_SUBSYS_CTNETLINK);
+
 static int __init ctnetlink_init(void)
 {
 	int ret;

Re: [PATCH 4/7] add missing module_alias_subsys

[Patrick McHardy]

Pablo Neira wrote:
> Add missing module alias. This is a must to load ctnetlink on demand.
> For example, the conntrack tool will fail if the module isn't loaded.

I don't think this is a good idea currently. Capability checking is
done after module autoloading, so any user can load ctnetlink,
ip_conntrack and all related modules. Please make sure to move
capability checking in nfnetlink before module loading first.

BTW: The same applies to ip_tables and ipt_ULOG through netlink
autoloading.

Re: [PATCH 4/7] add missing module_alias_subsys

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 3122 bytes --]

On Sat, Nov 05, 2005 at 08:31:08AM +0100, Patrick McHardy wrote:
> Pablo Neira wrote:
> >Add missing module alias. This is a must to load ctnetlink on demand.
> >For example, the conntrack tool will fail if the module isn't loaded.
> 
> I don't think this is a good idea currently. Capability checking is
> done after module autoloading, so any user can load ctnetlink,
> ip_conntrack and all related modules. 

interesting point, thanks for mentioning it.

> Please make sure to move capability checking in nfnetlink before
> module loading first.

This unfortunately doesn't work with the current architecture, where
every nfnetlink subsystem can specifiy the required capabilities per
message.  That specification isn't available before loading the module,
though.

I think we can (in addition to our usual capability checks) add a
capability check to only do autoloading of a module if CAP_NET_ADMIN is
set.  Like:


[NETFILTER] nfnetlink: only load subsystems if CAP_NET_ADMIN is set

Without this patch, any user can cause nfnetlink subsystems to be
autoloaded.  Those subsystems however could add significant processing
overhead to packet processing, and would refuse any configuration messages
from non-CAP_NET_ADMIN processes anyway.

This patch follows a suggestion from Patrick McHardy.

Signed-off-by: Harald Welte <laforge@netfilter.org>

---
commit b73cccaf3b2d71b8f516c9b28cd44bf3b4efab1c
tree a67a2728bae003a7974ed9e5fd69bc8a40fd7f03
parent 5bd49cc95f4f8ef5e6782242709927cd3ee6337d
author Harald Welte <laforge@hanuman.de.gnumonks.org> Sat, 05 Nov 2005 12:52:07 +0100
committer Harald Welte <laforge@netfilter.org> Sat, 05 Nov 2005 12:52:07 +0100

 net/netfilter/nfnetlink.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -240,15 +240,18 @@ static inline int nfnetlink_rcv_msg(stru
 	ss = nfnetlink_get_subsys(type);
 	if (!ss) {
 #ifdef CONFIG_KMOD
-		/* don't call nfnl_shunlock, since it would reenter
-		 * with further packet processing */
-		up(&nfnl_sem);
-		request_module("nfnetlink-subsys-%d", NFNL_SUBSYS_ID(type));
-		nfnl_shlock();
-		ss = nfnetlink_get_subsys(type);
+		if (cap_raised(NETLINK_CB(skb).eff_cap, CAP_NET_ADMIN)) {
+			/* don't call nfnl_shunlock, since it would reenter
+			 * with further packet processing */
+			up(&nfnl_sem);
+			request_module("nfnetlink-subsys-%d",
+					NFNL_SUBSYS_ID(type));
+			nfnl_shlock();
+			ss = nfnetlink_get_subsys(type);
+		}
 		if (!ss)
 #endif
-		goto err_inval;
+			goto err_inval;
 	}
 
 	nc = nfnetlink_find_client(type, ss);

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/7] add missing module_alias_subsys

[Patrick McHardy]

Harald Welte wrote:
> On Sat, Nov 05, 2005 at 08:31:08AM +0100, Patrick McHardy wrote:
> 
>>Pablo Neira wrote:
>>
>>>Add missing module alias. This is a must to load ctnetlink on demand.
>>>For example, the conntrack tool will fail if the module isn't loaded.
>>
>>I don't think this is a good idea currently. Capability checking is
>>done after module autoloading, so any user can load ctnetlink,
>>ip_conntrack and all related modules. 
> 
> interesting point, thanks for mentioning it.
> 
> 
>>Please make sure to move capability checking in nfnetlink before
>>module loading first.
> 
> 
> This unfortunately doesn't work with the current architecture, where
> every nfnetlink subsystem can specifiy the required capabilities per
> message.  That specification isn't available before loading the module,
> though.

Didn't we decide to remove the per-subsys capabilities and make all
of them require CAP_NET_ADMIN?

> I think we can (in addition to our usual capability checks) add a
> capability check to only do autoloading of a module if CAP_NET_ADMIN is
> set.  Like:

That also a possiblity, but I can't think of a case where we wouldn't
insist on CAP_NET_ADMIN, so just removing the whole per-subsys
capabilities seems easier to me.

Re: [PATCH 4/7] add missing module_alias_subsys

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Sat, Nov 05, 2005 at 12:56:06PM +0100, Patrick McHardy wrote:

> > This unfortunately doesn't work with the current architecture, where
> > every nfnetlink subsystem can specifiy the required capabilities per
> > message.  That specification isn't available before loading the module,
> > though.
> 
> Didn't we decide to remove the per-subsys capabilities and make all
> of them require CAP_NET_ADMIN?

We did?  I knew we were talking about it, but I must have forgotten the
result, sorry.

> That also a possiblity, but I can't think of a case where we wouldn't
> insist on CAP_NET_ADMIN, so just removing the whole per-subsys
> capabilities seems easier to me.

Mh, ok.  But you owe me a beer if we ever need to re-introduce it ;)

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/7] add missing module_alias_subsys

[Patrick McHardy]

Harald Welte wrote:
> On Sat, Nov 05, 2005 at 12:56:06PM +0100, Patrick McHardy wrote:
> 
> 
>>>This unfortunately doesn't work with the current architecture, where
>>>every nfnetlink subsystem can specifiy the required capabilities per
>>>message.  That specification isn't available before loading the module,
>>>though.
>>
>>Didn't we decide to remove the per-subsys capabilities and make all
>>of them require CAP_NET_ADMIN?
> 
> 
> We did?  I knew we were talking about it, but I must have forgotten the
> result, sorry.

I think we did in Montreal.

>>That also a possiblity, but I can't think of a case where we wouldn't
>>insist on CAP_NET_ADMIN, so just removing the whole per-subsys
>>capabilities seems easier to me.
> 
> Mh, ok.  But you owe me a beer if we ever need to re-introduce it ;)

Agreed :)