headers
Changli Gao | 1 Apr 2012 16:22
Picon

[PATCH] netfilter: check the length of the data before dereferencing it

We should check the length of the data before dereferencing it when parsing
the TCP options.

Signed-off-by: Changli Gao <xiaosuo <at> gmail.com>
---
 net/netfilter/nf_conntrack_proto_tcp.c |    4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 361eade..9e446c5 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
 <at>  <at>  -404,6 +404,8  <at>  <at>  static void tcp_options(const struct sk_buff *skb,
 			length--;
 			continue;
 		default:
+			if (length < 2)
+				return;
 			opsize=*ptr++;
 			if (opsize < 2) /* "silly options" */
 				return;
 <at>  <at>  -464,6 +466,8  <at>  <at>  static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
 			length--;
 			continue;
 		default:
+			if (length < 2)
+				return;
 			opsize = *ptr++;
 			if (opsize < 2) /* "silly options" */
 				return;
--
(Continue reading)

Permalink | Reply | 4 comments |
headers
Pablo Neira Ayuso | 1 Apr 2012 18:21
Favicon

Re: [PATCH] netfilter: don't do window scaling for a picked up connection

On Sun, Apr 01, 2012 at 11:04:43PM +0800, Changli Gao wrote:
> For a picked up connection, the window scaling option is also lost, because this
> option is only valid in SYN or SYN/ACK segments. We should remove the useless
> expression to save the CPU power.
> 
> Signed-off-by: Changli Gao <xiaosuo <at> gmail.com>
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c |    1 -
>  1 file changed, 1 deletion(-)
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 361eade..22f0500 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
>  <at>  <at>  -584,7 +584,6  <at>  <at>  static bool tcp_in_window(const struct nf_conn *ct,
>  			 * Let's try to use the data from the packet.
>  			 */
>  			sender->td_end = end;
> -			win <<= sender->td_scale;

This breaks conntrackd and its ability to recover flows by injecting
the window scaling via ctnetlink.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 1 comment |
headers
Changli Gao | 1 Apr 2012 18:41
Picon

Re: [PATCH] netfilter: don't do window scaling for a picked up connection

On Mon, Apr 2, 2012 at 12:21 AM, Pablo Neira Ayuso <pablo <at> netfilter.org> wrote:
>
> This breaks conntrackd and its ability to recover flows by injecting
> the window scaling via ctnetlink.

Got it. Thanks.

Just forget this patch. I'll post another patch to fix the duplicate
window scaling: one is here, and the other is before the state
updating code.

--

-- 
Regards,
Changli Gao(xiaosuo <at> gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Pablo Neira Ayuso | 1 Apr 2012 18:43
Favicon

Re: [PATCH] netfilter: check the length of the data before dereferencing it

On Sun, Apr 01, 2012 at 10:22:50PM +0800, Changli Gao wrote:
> We should check the length of the data before dereferencing it when parsing
> the TCP options.
> 
> Signed-off-by: Changli Gao <xiaosuo <at> gmail.com>
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c |    4 ++++
>  1 file changed, 4 insertions(+)
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 361eade..9e446c5 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
>  <at>  <at>  -404,6 +404,8  <at>  <at>  static void tcp_options(const struct sk_buff *skb,
>  			length--;
>  			continue;
>  		default:
> +			if (length < 2)
> +				return;
>  			opsize=*ptr++;
>  			if (opsize < 2) /* "silly options" */
>  				return;

length is always multiple of 4:

int length = (tcph->doff*4) - sizeof(struct tcphdr);
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply | 3 comments |
headers
Eric Dumazet | 1 Apr 2012 18:55
Picon

Re: [PATCH] netfilter: check the length of the data before dereferencing it

On Sun, 2012-04-01 at 18:43 +0200, Pablo Neira Ayuso wrote:
> On Sun, Apr 01, 2012 at 10:22:50PM +0800, Changli Gao wrote:
> > We should check the length of the data before dereferencing it when parsing
> > the TCP options.
> > 
> > Signed-off-by: Changli Gao <xiaosuo <at> gmail.com>
> > ---
> >  net/netfilter/nf_conntrack_proto_tcp.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > index 361eade..9e446c5 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> >  <at>  <at>  -404,6 +404,8  <at>  <at>  static void tcp_options(const struct sk_buff *skb,
> >  			length--;
> >  			continue;
> >  		default:
> > +			if (length < 2)
> > +				return;
> >  			opsize=*ptr++;
> >  			if (opsize < 2) /* "silly options" */
> >  				return;
> 
> length is always multiple of 4:
> 
> int length = (tcph->doff*4) - sizeof(struct tcphdr);
> --

initial value yes, but it can change in the loop.
(Continue reading)

Permalink | Reply | 2 comments |
headers
Pablo Neira Ayuso | 1 Apr 2012 19:02
Favicon

Re: [PATCH] netfilter: check the length of the data before dereferencing it

On Sun, Apr 01, 2012 at 06:55:22PM +0200, Eric Dumazet wrote:
> On Sun, 2012-04-01 at 18:43 +0200, Pablo Neira Ayuso wrote:
> > On Sun, Apr 01, 2012 at 10:22:50PM +0800, Changli Gao wrote:
> > > We should check the length of the data before dereferencing it when parsing
> > > the TCP options.
> > > 
> > > Signed-off-by: Changli Gao <xiaosuo <at> gmail.com>
> > > ---
> > >  net/netfilter/nf_conntrack_proto_tcp.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > > index 361eade..9e446c5 100644
> > > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > >  <at>  <at>  -404,6 +404,8  <at>  <at>  static void tcp_options(const struct sk_buff *skb,
> > >  			length--;
> > >  			continue;
> > >  		default:
> > > +			if (length < 2)
> > > +				return;
> > >  			opsize=*ptr++;
> > >  			if (opsize < 2) /* "silly options" */
> > >  				return;
> > 
> > length is always multiple of 4:
> > 
> > int length = (tcph->doff*4) - sizeof(struct tcphdr);
> > --
> 
> initial value yes, but it can change in the loop.
(Continue reading)

Permalink | Reply | 1 comment |
headers
Pablo Neira Ayuso | 1 Apr 2012 19:42
Favicon

Re: `iptables -m tcp --syn` doesn't do what the man says

On Sat, Mar 31, 2012 at 12:58:54AM +0400, Artyom Gavrichenkov wrote:
> Hi all,
> 
> The iptables(8) manpage says:
> 
> --- [cut here] ---
>    tcp
>        These extensions can be used if `--protocol tcp' is specified. It provides the following options:
>        [!] --syn
>               Only  match  TCP packets with the SYN bit set and the ACK,RST and FIN bits cleared.  Such packets are used to
request TCP connection initia‐
>               tion; for example, blocking such packets coming in an interface will prevent incoming TCP connections,
but outgoing TCP connections will  be
>               unaffected.   It  is  equivalent  to  --tcp-flags  SYN,RST,ACK,FIN  SYN.   If  the "!" flag precedes the "--syn",
the sense of the option is
>               inverted.
> --- [cut here] ---
> 
> Unfortunately, with current stable Linux kernel release (as well as
> with most of the previous versions) blocking TCP packets with the SYN
> bit set and the ACK,RST and FIN bits cleared won't prevent incoming
> TCP connections.
> 
> Currently Linux TCP stack considers an incoming TCP segment to be a
> connection initiation request if the segment only has SYN flag set and
> ACK and RST flags cleared. You can easily check it yourself with your
> nearest Linux box, as well as on the netfilter.org (213.95.27.115):
> 
> # hping3 -c 2 -n -FS -p 80 netfilter.org
> HPING netfilter.org (wlan0 213.95.27.115): SF set, 40 headers + 0 data bytes
(Continue reading)

Permalink | Reply | 1 comment |
headers
Pablo Neira Ayuso | 1 Apr 2012 19:51
Favicon

Re: NFQUEUE target with --treat-accept-as-continue?

On Sat, Mar 31, 2012 at 10:09:42PM +0800, Amm Snort wrote:
> Hello all,
> 
> I would like to suggest a feature (additional parameter) for NFQUEUE target.
> 
> I am basically trying to use snort with NFQUEUE.
> 
> Basic iptables rule is somewhat like this:
> 
> iptables -A INPUT -i ppp1 -j NFQUEUE
> iptables -A INPUT -p tcp --dport 22 -j ACCEPT
> iptables -A INPUT -j DROP
> 
> Idea is to pass all packets to snort first and then if snort detects trojan signature then
> it can DROP (IPS) the packet (drop rule of snort). If snort detects port scan it can
> LOG but still ACCEPT (IDS) (alert rule of snort). If snort ACCEPTs the packet then
> continue processing next rule.
> 
> On side note, this idea also reduces load on snort as it does not have to sniff every
> packet coming on network card and also we can pass only selected packets to snort.
> 
> 
> This idea however, DOES NOT work because currently when QUEUE program (snort)
> gives verdict as NF_ACCEPT, iptables STOPS processing further rules and
> ACCEPTs the packet.
> 
> This actually becomes security risk because default snort rules DO NOT DO port blocking.
> and ACCEPTs everything that is not EXPLICITLY dropped.
> 
> And hence NFQUEUE target indirectly becomes ACCEPT target for all packets and
(Continue reading)

Permalink | Reply | 3 comments |
headers
Eric Dumazet | 1 Apr 2012 19:53
Picon

Re: `iptables -m tcp --syn` doesn't do what the man says

On Sun, 2012-04-01 at 19:42 +0200, Pablo Neira Ayuso wrote:

> I understand your concern, but the info in the manpage is correct:
> basically, it can be extracted from it that --syn will not match
> SYN+FIN packets.
> 
> As you point in your patch, you have to use:
> 
> --tcp-flags  SYN,RST,ACK  SYN
> 
> in your rule-set for the situation that you describe.
> 
> Changing the default behaviour of --syn to catch this case is
> delicate, I don't want to break backward compatibility.

Agreed.

With TCP Fast Open, it might be possible to send a SYN+FIN+cookies+DATA
in a single frame.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
David Miller | 1 Apr 2012 22:17
Favicon

Re: [patch 1/1] net/netfilter/nfnetlink_acct.c: use linux/atomic.h

From: akpm <at> linux-foundation.org
Date: Wed, 28 Mar 2012 15:10:57 -0700

> From: Andrew Morton <akpm <at> linux-foundation.org>
> Subject: net/netfilter/nfnetlink_acct.c: use linux/atomic.h
> 
> There's no known problem here, but this is one of only two non-arch files
> in the kernel which use asm/atomic.h instead of linux/atomic.h.
> 
> Acked-by: Pablo Neira Ayuso <pablo <at> netfilter.org>
> Cc: Patrick McHardy <kaber <at> trash.net>
> Cc: "David S. Miller" <davem <at> davemloft.net>
> Signed-off-by: Andrew Morton <akpm <at> linux-foundation.org>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |

Gmane