[PATCH] NFQUEUE: Fix bug with order of fanout and bypass

[PATCH] NFQUEUE: Fix bug with order of fanout and bypass

[Shivani Bhardwaj]

NFQUEUE had a bug with the ordering of fanout and bypass options which
was arising due to same and odd values for flags and bypass when used
together. Because of this, during bitwise ANDing of flags and
NFQ_FLAG_CPU_FANOUT, the value always evaluated to false (since
NFQ_FLAG_CPU_FANOUT=0x02) and led to skipping of fanout option
whenever it was used before bypass because then flags would be 1.

Before this patch,

$ sudo iptables -A FORWARD -j NFQUEUE -p TCP --sport 80 --queue-balance 0:3 --queue-cpu-fanout --queue-bypass

Chain FORWARD (policy ACCEPT)
target     prot opt source               destination
NFQUEUE    tcp  --  anywhere             anywhere             tcp spt:http NFQUEUE balance 0:3 bypass

After this patch,

Chain FORWARD (policy ACCEPT)
target     prot opt source               destination
NFQUEUE    tcp  --  anywhere             anywhere             tcp spt:http NFQUEUE balance 0:3 bypass cpu-fanout

Closes bugzilla entry: http://bugzilla.netfilter.org/show_bug.cgi?id=939

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 extensions/libxt_NFQUEUE.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/libxt_NFQUEUE.c b/extensions/libxt_NFQUEUE.c
index 8115457..0b5becc 100644
--- a/extensions/libxt_NFQUEUE.c
+++ b/extensions/libxt_NFQUEUE.c
@@ -99,7 +99,7 @@ static void NFQUEUE_parse_v2(struct xt_option_call *cb)
 	NFQUEUE_parse_v1(cb);
 	switch (cb->entry->id) {
 	case O_QUEUE_BYPASS:
-		info->bypass = 1;
+		info->bypass |= NFQ_FLAG_BYPASS;
 		break;
 	}
 }
-- 
1.9.1

Re: [PATCH] NFQUEUE: Fix bug with order of fanout and bypass

[Florian Westphal]

Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> NFQUEUE had a bug with the ordering of fanout and bypass options which
> was arising due to same and odd values for flags and bypass when used
> together. Because of this, during bitwise ANDing of flags and
> NFQ_FLAG_CPU_FANOUT, the value always evaluated to false (since
> NFQ_FLAG_CPU_FANOUT=0x02) and led to skipping of fanout option
> whenever it was used before bypass because then flags would be 1.
> 
> Before this patch,
> 
> $ sudo iptables -A FORWARD -j NFQUEUE -p TCP --sport 80 --queue-balance 0:3 --queue-cpu-fanout --queue-bypass
> 
> Chain FORWARD (policy ACCEPT)
> target     prot opt source               destination
> NFQUEUE    tcp  --  anywhere             anywhere             tcp spt:http NFQUEUE balance 0:3 bypass
> 
> After this patch,
> 
> Chain FORWARD (policy ACCEPT)
> target     prot opt source               destination
> NFQUEUE    tcp  --  anywhere             anywhere             tcp spt:http NFQUEUE balance 0:3 bypass cpu-fanout
 
> Closes bugzilla entry: http://bugzilla.netfilter.org/show_bug.cgi?id=939

Ugh, good catch!

> diff --git a/extensions/libxt_NFQUEUE.c b/extensions/libxt_NFQUEUE.c
> index 8115457..0b5becc 100644
> --- a/extensions/libxt_NFQUEUE.c
> +++ b/extensions/libxt_NFQUEUE.c
> @@ -99,7 +99,7 @@ static void NFQUEUE_parse_v2(struct xt_option_call *cb)
>  	NFQUEUE_parse_v1(cb);
>  	switch (cb->entry->id) {
>  	case O_QUEUE_BYPASS:
> -		info->bypass = 1;
> +		info->bypass |= NFQ_FLAG_BYPASS;
>  		break;

I don't like this mix of v2 and v3 layout.

Could you try to create an alternate patch that changes
NFQUEUE_parse_v3 to call NFQUEUE_parse_v1 and then add
	case O_QUEUE_BYPASS:
		info->bypass |= NFQ_FLAG_BYPASS;

to NFQUEUE_parse_v3?

I think that this would make it a bit clearer and
it also avoids the v3/v2/v1 stacking.

Thanks!

Re: [PATCH] NFQUEUE: Fix bug with order of fanout and bypass

[Shivani Bhardwaj]

On Tue, Apr 12, 2016 at 10:58 PM, Florian Westphal <fw@strlen.de> wrote:
> Shivani Bhardwaj <shivanib134@gmail.com> wrote:
>> NFQUEUE had a bug with the ordering of fanout and bypass options which
>> was arising due to same and odd values for flags and bypass when used
>> together. Because of this, during bitwise ANDing of flags and
>> NFQ_FLAG_CPU_FANOUT, the value always evaluated to false (since
>> NFQ_FLAG_CPU_FANOUT=0x02) and led to skipping of fanout option
>> whenever it was used before bypass because then flags would be 1.
>>
>> Before this patch,
>>
>> $ sudo iptables -A FORWARD -j NFQUEUE -p TCP --sport 80 --queue-balance 0:3 --queue-cpu-fanout --queue-bypass
>>
>> Chain FORWARD (policy ACCEPT)
>> target     prot opt source               destination
>> NFQUEUE    tcp  --  anywhere             anywhere             tcp spt:http NFQUEUE balance 0:3 bypass
>>
>> After this patch,
>>
>> Chain FORWARD (policy ACCEPT)
>> target     prot opt source               destination
>> NFQUEUE    tcp  --  anywhere             anywhere             tcp spt:http NFQUEUE balance 0:3 bypass cpu-fanout
>
>> Closes bugzilla entry: http://bugzilla.netfilter.org/show_bug.cgi?id=939
>
> Ugh, good catch!
>
>> diff --git a/extensions/libxt_NFQUEUE.c b/extensions/libxt_NFQUEUE.c
>> index 8115457..0b5becc 100644
>> --- a/extensions/libxt_NFQUEUE.c
>> +++ b/extensions/libxt_NFQUEUE.c
>> @@ -99,7 +99,7 @@ static void NFQUEUE_parse_v2(struct xt_option_call *cb)
>>       NFQUEUE_parse_v1(cb);
>>       switch (cb->entry->id) {
>>       case O_QUEUE_BYPASS:
>> -             info->bypass = 1;
>> +             info->bypass |= NFQ_FLAG_BYPASS;
>>               break;
>
> I don't like this mix of v2 and v3 layout.
>
> Could you try to create an alternate patch that changes
> NFQUEUE_parse_v3 to call NFQUEUE_parse_v1 and then add
>         case O_QUEUE_BYPASS:
>                 info->bypass |= NFQ_FLAG_BYPASS;
>
> to NFQUEUE_parse_v3?
>
> I think that this would make it a bit clearer and
> it also avoids the v3/v2/v1 stacking.
>
Sure.
Just to make sure I get this right, should I be using two objects of
structures xt_NFQ_info_v3 and xt_NFQ_info_v2 (since v3 does not have
bypass) and make switch cases accordingly in v3?
Should I be doing this for all the functions (save, xlate, print)
since the same stacking is there too?

Thanks!

> Thanks!

Re: [PATCH] NFQUEUE: Fix bug with order of fanout and bypass

[Florian Westphal]

Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> > I think that this would make it a bit clearer and
> > it also avoids the v3/v2/v1 stacking.
> >
> Sure.
> Just to make sure I get this right, should I be using two objects of
> structures xt_NFQ_info_v3 and xt_NFQ_info_v2 (since v3 does not have
> bypass) and make switch cases accordingly in v3?

I meant something like this (untested):

diff --git a/extensions/libxt_NFQUEUE.c b/extensions/libxt_NFQUEUE.c
index 8115457..9750ce0 100644
--- a/extensions/libxt_NFQUEUE.c
+++ b/extensions/libxt_NFQUEUE.c
@@ -108,11 +108,14 @@ static void NFQUEUE_parse_v3(struct xt_option_call *cb)
 {
 	struct xt_NFQ_info_v3 *info = cb->data;
 
-	NFQUEUE_parse_v2(cb);
+	NFQUEUE_parse_v1(cb);
 	switch (cb->entry->id) {
 	case O_QUEUE_CPU_FANOUT:
 		info->flags |= NFQ_FLAG_CPU_FANOUT;
 		break;
+	case O_QUEUE_BYPASS:
+		info->flags |= NFQ_FLAG_BYPASS;
+		break;
 	}
 }

> Should I be doing this for all the functions (save, xlate, print)
> since the same stacking is there too?

Hmm, I think it would make sense to disentangle this as well
(as a 2nd cleanup patch).

The ->bypass/->flag overloading works but its not really obvious...

I don't have a strong opinion however, if you think your v1 patch
is ok I'm fine with it as well.