[PATCH nf 1/3] netfilter: nfnetlink: validate nfnetlink header from batch

[PATCH nf 1/3] netfilter: nfnetlink: validate nfnetlink header from batch

[Pablo Neira Ayuso]

Make sure there is enough room for the nfnetlink header in the
netlink messages that are part of the batch. There is a similar
check in netlink_rcv_skb().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 13c2e17..c6619d4 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -321,7 +321,8 @@ replay:
 		nlh = nlmsg_hdr(skb);
 		err = 0;
 
-		if (nlh->nlmsg_len < NLMSG_HDRLEN) {
+		if (nlmsg_len(nlh) < sizeof(struct nfgenmsg) ||
+		    skb->len < nlh->nlmsg_len) {
 			err = -EINVAL;
 			goto ack;
 		}
-- 
1.7.10.4

[PATCH nf 2/3] netfilter: nfnetlink: relax strict multicast group from netlink_bind

[Pablo Neira Ayuso]

Relax the checking that was introduced in 97840cb ("netfilter:
nfnetlink: fix insufficient validation in nfnetlink_bind") when the
subscription bitmask is used. Existing userspace code code may request
to listen to all of the existing netlink groups by setting an all to one
subscription group bitmask. Netlink already validates subscription via
setsockopt() for us.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index c6619d4..5082d81 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -469,8 +469,11 @@ static int nfnetlink_bind(int group)
 	const struct nfnetlink_subsystem *ss;
 	int type;
 
+	/* No strict check for groups when subscription bitmask is used, group
+	 * binding via setsockopt() already rejects this from netlink.
+	 */
 	if (group <= NFNLGRP_NONE || group > NFNLGRP_MAX)
-		return -EINVAL;
+		return 0;
 
 	type = nfnl_group2type[group];
 
-- 
1.7.10.4

Re: [PATCH nf 2/3] netfilter: nfnetlink: relax strict multicast group from netlink_bind

[Patrick McHardy]

On 05.01, Pablo Neira Ayuso wrote:
> Relax the checking that was introduced in 97840cb ("netfilter:
> nfnetlink: fix insufficient validation in nfnetlink_bind") when the
> subscription bitmask is used. Existing userspace code code may request
> to listen to all of the existing netlink groups by setting an all to one
> subscription group bitmask. Netlink already validates subscription via
> setsockopt() for us.

What is the point of doing this? I don't think its particulary
reasonable to subscribe to ~0 unless you're implementing some kind of
monitor.

We also don't know whether a bitmask or an invalid group number was
used, so the comment below is at least misleading.

And, unrelated, but since it went in via netfilter asking anyway, why
is the group number signed? That doesn't make any sense, it is treated
as unsigned everywhere else.

> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index c6619d4..5082d81 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -469,8 +469,11 @@ static int nfnetlink_bind(int group)
>  	const struct nfnetlink_subsystem *ss;
>  	int type;
>  
> +	/* No strict check for groups when subscription bitmask is used, group
> +	 * binding via setsockopt() already rejects this from netlink.
> +	 */
>  	if (group <= NFNLGRP_NONE || group > NFNLGRP_MAX)
> -		return -EINVAL;
> +		return 0;
>  
>  	type = nfnl_group2type[group];
>  
> -- 
> 1.7.10.4
> 

Re: [PATCH nf 2/3] netfilter: nfnetlink: relax strict multicast group from netlink_bind

[Pablo Neira Ayuso]

On Mon, Jan 05, 2015 at 11:22:35AM +0000, Patrick McHardy wrote:
> On 05.01, Pablo Neira Ayuso wrote:
> > Relax the checking that was introduced in 97840cb ("netfilter:
> > nfnetlink: fix insufficient validation in nfnetlink_bind") when the
> > subscription bitmask is used. Existing userspace code code may request
> > to listen to all of the existing netlink groups by setting an all to one
> > subscription group bitmask. Netlink already validates subscription via
> > setsockopt() for us.
> 
> What is the point of doing this? I don't think its particulary
> reasonable to subscribe to ~0 unless you're implementing some kind of
> monitor.

This is how we've been supporting this since the beginning. So
userspace applications could subscribe to ~0 and don't care if the
group exists or not.

After the recent change, those will break. None of the userspace
netfilter codebase actually need this, but other third party
application will break when binding if they were using ~0 for
monitoring.

> We also don't know whether a bitmask or an invalid group number was
> used, so the comment below is at least misleading.
> 
> And, unrelated, but since it went in via netfilter asking anyway, why
> is the group number signed? That doesn't make any sense, it is treated
> as unsigned everywhere else.

That should be changed, yes.

> > diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> > index c6619d4..5082d81 100644
> > --- a/net/netfilter/nfnetlink.c
> > +++ b/net/netfilter/nfnetlink.c
> > @@ -469,8 +469,11 @@ static int nfnetlink_bind(int group)
> >  	const struct nfnetlink_subsystem *ss;
> >  	int type;
> >  
> > +	/* No strict check for groups when subscription bitmask is used, group
> > +	 * binding via setsockopt() already rejects this from netlink.
> > +	 */
> >  	if (group <= NFNLGRP_NONE || group > NFNLGRP_MAX)
> > -		return -EINVAL;
> > +		return 0;
> >  
> >  	type = nfnl_group2type[group];
> >  
> > -- 
> > 1.7.10.4
> > 

Re: [PATCH nf 2/3] netfilter: nfnetlink: relax strict multicast group from netlink_bind

[Patrick McHardy]

On 05.01, Pablo Neira Ayuso wrote:
> On Mon, Jan 05, 2015 at 11:22:35AM +0000, Patrick McHardy wrote:
> > On 05.01, Pablo Neira Ayuso wrote:
> > > Relax the checking that was introduced in 97840cb ("netfilter:
> > > nfnetlink: fix insufficient validation in nfnetlink_bind") when the
> > > subscription bitmask is used. Existing userspace code code may request
> > > to listen to all of the existing netlink groups by setting an all to one
> > > subscription group bitmask. Netlink already validates subscription via
> > > setsockopt() for us.
> > 
> > What is the point of doing this? I don't think its particulary
> > reasonable to subscribe to ~0 unless you're implementing some kind of
> > monitor.
> 
> This is how we've been supporting this since the beginning. So
> userspace applications could subscribe to ~0 and don't care if the
> group exists or not.
> 
> After the recent change, those will break. None of the userspace
> netfilter codebase actually need this, but other third party
> application will break when binding if they were using ~0 for
> monitoring.
> 
> > We also don't know whether a bitmask or an invalid group number was
> > used, so the comment below is at least misleading.
> > 
> > And, unrelated, but since it went in via netfilter asking anyway, why
> > is the group number signed? That doesn't make any sense, it is treated
> > as unsigned everywhere else.
> 
> That should be changed, yes.

Assuming you mean both the signedness and the comment, that seems fine.