[PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[Liping Zhang]

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

In iptables, if the user add a rule to send tcp RST and specify the
non-TCP protocol, such as UDP, kernel will reject this request. But
in nftables, this validity check only occurs in nft tool, i.e. only
in userspace.

This means that user can add such a rule like follows via nfnetlink:
  "nft add rule filter forward ip protocol udp reject with tcp reset"

This will generate some confusing tcp RST packets. So we should send
tcp RST only when it is TCP packet.

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

diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index b6ea57e..fd82202 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -24,6 +24,9 @@ const struct tcphdr *nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,
 	if (ip_hdr(oldskb)->frag_off & htons(IP_OFFSET))
 		return NULL;
 
+	if (ip_hdr(oldskb)->protocol != IPPROTO_TCP)
+		return NULL;
+
 	oth = skb_header_pointer(oldskb, ip_hdrlen(oldskb),
 				 sizeof(struct tcphdr), _oth);
 	if (oth == NULL)
-- 
1.7.9.5

Re: [PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[Marcelo Ricardo Leitner]

Hi Liping,

On Mon, Jun 20, 2016 at 09:26:28PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> In iptables, if the user add a rule to send tcp RST and specify the
> non-TCP protocol, such as UDP, kernel will reject this request. But
> in nftables, this validity check only occurs in nft tool, i.e. only
> in userspace.
> 
> This means that user can add such a rule like follows via nfnetlink:
>   "nft add rule filter forward ip protocol udp reject with tcp reset"
> 
> This will generate some confusing tcp RST packets. So we should send
> tcp RST only when it is TCP packet.
> 
> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
> ---
>  net/ipv4/netfilter/nf_reject_ipv4.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
> index b6ea57e..fd82202 100644
> --- a/net/ipv4/netfilter/nf_reject_ipv4.c
> +++ b/net/ipv4/netfilter/nf_reject_ipv4.c
> @@ -24,6 +24,9 @@ const struct tcphdr *nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,
>  	if (ip_hdr(oldskb)->frag_off & htons(IP_OFFSET))
>  		return NULL;
>  
> +	if (ip_hdr(oldskb)->protocol != IPPROTO_TCP)
> +		return NULL;
> +
>  	oth = skb_header_pointer(oldskb, ip_hdrlen(oldskb),
>  				 sizeof(struct tcphdr), _oth);
>  	if (oth == NULL)

A different check/log is made for ip6:
nf_reject_ip6_tcphdr_get():
        /* IP header checks: fragment, too short. */
        if (proto != IPPROTO_TCP || *otcplen < sizeof(struct tcphdr)) {
                pr_debug("proto(%d) != IPPROTO_TCP or too short (len = %d)\n",
                         proto, *otcplen);
                return NULL;
        }

Would be nice to have some consistency on this log message as it
increases debug-ability.

  Marcelo

Re: [PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[Liping Zhang]

Hi Marcelo,

2016-06-20 23:48 GMT+08:00 Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>:
>
> A different check/log is made for ip6:
> nf_reject_ip6_tcphdr_get():
>         /* IP header checks: fragment, too short. */
>         if (proto != IPPROTO_TCP || *otcplen < sizeof(struct tcphdr)) {
>                 pr_debug("proto(%d) != IPPROTO_TCP or too short (len = %d)\n",
>                          proto, *otcplen);
>                 return NULL;
>         }
>
> Would be nice to have some consistency on this log message as it
> increases debug-ability.
>

Thanks for your opinion.

But you can see, there are many inconsistent things between
nf_reject_ip6_tcphdr_get and nf_reject_ip_tcphdr_get.

For example, when tcp->rst is set, reject_ip6 will call
pr_debug("RST is set\n"), while there's nothing in reject_ip4.

IMO, these debug informations are almost useless, so there's
no need to add this debug info only for consistent with nf_reject_ip6.

Re: [PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[Marcelo Ricardo Leitner]

On Tue, Jun 21, 2016 at 09:35:55AM +0800, Liping Zhang wrote:
> Hi Marcelo,
> 
> 2016-06-20 23:48 GMT+08:00 Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>:
> >
> > A different check/log is made for ip6:
> > nf_reject_ip6_tcphdr_get():
> >         /* IP header checks: fragment, too short. */
> >         if (proto != IPPROTO_TCP || *otcplen < sizeof(struct tcphdr)) {
> >                 pr_debug("proto(%d) != IPPROTO_TCP or too short (len = %d)\n",
> >                          proto, *otcplen);
> >                 return NULL;
> >         }
> >
> > Would be nice to have some consistency on this log message as it
> > increases debug-ability.
> >
> 
> Thanks for your opinion.
> 
> But you can see, there are many inconsistent things between
> nf_reject_ip6_tcphdr_get and nf_reject_ip_tcphdr_get.

That's true, yet sooner or later we can catch up the differences.

> 
> For example, when tcp->rst is set, reject_ip6 will call
> pr_debug("RST is set\n"), while there's nothing in reject_ip4.
> 
> IMO, these debug informations are almost useless, so there's
> no need to add this debug info only for consistent with nf_reject_ip6.

Fair enough. Although I did the comment, I don't have a strong opinion
on this.

Thanks,
Marcelo

Re: [PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[Pablo Neira Ayuso]

On Tue, Jun 21, 2016 at 04:03:01PM -0300, Marcelo Ricardo Leitner wrote:
> On Tue, Jun 21, 2016 at 09:35:55AM +0800, Liping Zhang wrote:
> > For example, when tcp->rst is set, reject_ip6 will call
> > pr_debug("RST is set\n"), while there's nothing in reject_ip4.
> > 
> > IMO, these debug informations are almost useless, so there's
> > no need to add this debug info only for consistent with nf_reject_ip6.
> 
> Fair enough. Although I did the comment, I don't have a strong opinion
> on this.

I'd take a patch to get rid of those pr_debug() in nf_reject_ip6.

Thanks.

Re: [PACTH nf-next] netfilter: nf_reject_ipv4: don't send tcp RST if the packet is non-TCP

[Pablo Neira Ayuso]

On Mon, Jun 20, 2016 at 09:26:28PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> In iptables, if the user add a rule to send tcp RST and specify the
> non-TCP protocol, such as UDP, kernel will reject this request. But
> in nftables, this validity check only occurs in nft tool, i.e. only
> in userspace.
> 
> This means that user can add such a rule like follows via nfnetlink:
>   "nft add rule filter forward ip protocol udp reject with tcp reset"
> 
> This will generate some confusing tcp RST packets. So we should send
> tcp RST only when it is TCP packet.

Applied, thanks.