[PATCH nf-next] netfilter: nft_queue: check the validation of queues_total and queuenum

[PATCH nf-next] netfilter: nft_queue: check the validation of queues_total and queuenum

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Although the validation of queues_total and queuenum is checked in nft
utility, but user can add nft rules via nfnetlink, so it is necessary
to check the validation at the nft_queue expr init routine too.

Tested by run ./nft-test.py any/queue.t:
  any/queue.t: 6 unit tests, 0 error, 0 warning

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_queue.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/netfilter/nft_queue.c b/net/netfilter/nft_queue.c
index 61d216e..d16d599 100644
--- a/net/netfilter/nft_queue.c
+++ b/net/netfilter/nft_queue.c
@@ -65,6 +65,7 @@ static int nft_queue_init(const struct nft_ctx *ctx,
 			   const struct nlattr * const tb[])
 {
 	struct nft_queue *priv = nft_expr_priv(expr);
+	u32 maxid;
 
 	if (tb[NFTA_QUEUE_NUM] == NULL)
 		return -EINVAL;
@@ -74,6 +75,16 @@ static int nft_queue_init(const struct nft_ctx *ctx,
 
 	if (tb[NFTA_QUEUE_TOTAL] != NULL)
 		priv->queues_total = ntohs(nla_get_be16(tb[NFTA_QUEUE_TOTAL]));
+	else
+		priv->queues_total = 1;
+
+	if (priv->queues_total == 0)
+		return -EINVAL;
+
+	maxid = priv->queues_total - 1 + priv->queuenum;
+	if (maxid > U16_MAX)
+		return -ERANGE;
+
 	if (tb[NFTA_QUEUE_FLAGS] != NULL) {
 		priv->flags = ntohs(nla_get_be16(tb[NFTA_QUEUE_FLAGS]));
 		if (priv->flags & ~NFT_QUEUE_FLAG_MASK)
-- 
2.5.5

Re: [PATCH nf-next] netfilter: nft_queue: check the validation of queues_total and queuenum

[Pablo Neira Ayuso]

On Tue, Sep 06, 2016 at 10:33:37PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Although the validation of queues_total and queuenum is checked in nft
> utility, but user can add nft rules via nfnetlink, so it is necessary
> to check the validation at the nft_queue expr init routine too.

Applied, thanks.

More comments on things I see on nft_queue at this stage:

1) Another issue, I can see nfqueue_hash() depends on
CONFIG_IP6_NF_IPTABLES, this is not good since nft_queue
infrastructure should not depend on iptables. Probably making this
dependent of CONFIG_IPV6 is enough, unless you find anything better.

2) It would be good if nft_queue takes a _SREG_FROM and _SREG_TO to
select the queue numbers, in a similar fashion to nft_nat. The idea is
that we allow plugging nft_queue into nftables mapping, currently this
is not working given that the queue number is passed as an attribute
that contains the value.

3) It would be good to add py tests with larger range. I remember that
the range 1-65535 currently doesn't work in both nft_queue and
xt_NFQUEUE because the queue_total arithmetics are not right.

It would be great if you can have a look into this.

Thanks!

Re: [PATCH nf-next] netfilter: nft_queue: check the validation of queues_total and queuenum

[Liping Zhang]

2016-09-09 22:04 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> More comments on things I see on nft_queue at this stage:
>
> 1) Another issue, I can see nfqueue_hash() depends on
> CONFIG_IP6_NF_IPTABLES, this is not good since nft_queue
> infrastructure should not depend on iptables. Probably making this
> dependent of CONFIG_IPV6 is enough, unless you find anything better.

Yes, I can send a patch to improve this.

> 2) It would be good if nft_queue takes a _SREG_FROM and _SREG_TO to
> select the queue numbers, in a similar fashion to nft_nat. The idea is
> that we allow plugging nft_queue into nftables mapping, currently this
> is not working given that the queue number is passed as an attribute
> that contains the value.

I will have a look into this. I think _SREG_FROM and _SREG_TO
stands for [startqnum:endqnum], currently queuenum and queue_total
has a restriction, see below.

> 3) It would be good to add py tests with larger range. I remember that
> the range 1-65535 currently doesn't work in both nft_queue and
> xt_NFQUEUE because the queue_total arithmetics are not right.

Cover with large range is necessary. Yes, I find that range 0-65535(not 1-65535)
currently doesn't work in both. This is because queue_total is U16 type,
but 0-65535 means queue_total is 65536, this is overflow, so queue_total is
treated as zero.

A workaround method is that when the queue_total is 0, we treat it as 65536,
but it's a little ugly and tricky. But if we use [startqnum:endqnum],
this problem
will be not exist.

I think for iptables, it's simple, we can introduce a new revision 4
to fix this problem.

But for nftables, if we just convert to use the new attribute _SREG_FROM and
_SREG_TO, when the user use the new nftables and the old kernel, queue expr
cannot work well. It seems that we must handle the compatibility in
the user space
too.

Thanks Pablo!