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

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

[Changli Gao]

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

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/ipv4/tcp_input.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index e886e2f..5099f08 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o
 			length--;
 			continue;
 		default:
+			if (length < 2)
+				return;
 			opsize = *ptr++;
 			if (opsize < 2) /* "silly options" */
 				return;

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

[Eric Dumazet]

On Mon, 2012-04-02 at 11:10 +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@gmail.com>
> ---
>  net/ipv4/tcp_input.c |    2 ++
>  1 file changed, 2 insertions(+)
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index e886e2f..5099f08 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o
>  			length--;
>  			continue;
>  		default:
> +			if (length < 2)
> +				return;
>  			opsize = *ptr++;
>  			if (opsize < 2) /* "silly options" */
>  				return;

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Apr 2012 05:19:33 +0200

>> @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o
>>  			length--;
>>  			continue;
>>  		default:
>> +			if (length < 2)
>> +				return;
>>  			opsize = *ptr++;
>>  			if (opsize < 2) /* "silly options" */
>>  				return;
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Tag Eric, you're it.

You ACK'd this patch, so you get to show how this is actually able
to cause some kind of problem.

I assert that this is adding a useless test, that doesn't fix any kind
of possible crash or misbehavior.  If length == 1 at the default:, the
code will absolutely do the right thing.

Prove me wrong.

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

[Changli Gao]

On Mon, Apr 2, 2012 at 11:29 AM, David Miller <davem@davemloft.net> wrote:
>
> Tag Eric, you're it.
>
> You ACK'd this patch, so you get to show how this is actually able
> to cause some kind of problem.
>
> I assert that this is adding a useless test, that doesn't fix any kind
> of possible crash or misbehavior.  If length == 1 at the default:, the
> code will absolutely do the right thing.
>
> Prove me wrong.

Thinking about a malformed tcp segment, which has no data but silly
options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we
will try to dereference one byte over the boundary when parsing the
options. I know we have skb_shared_info at the end and it won't cause
any crash, but should we rely on this fact?

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Dumazet]

On Mon, 2012-04-02 at 11:45 +0800, Changli Gao wrote:

> Thinking about a malformed tcp segment, which has no data but silly
> options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we
> will try to dereference one byte over the boundary when parsing the
> options. I know we have skb_shared_info at the end and it won't cause
> any crash, but should we rely on this fact?
> 

No we cant rely on this, kmemcheck might barf on us.

Your patch (and the netfilter one) is fine.

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Apr 2012 05:53:17 +0200

> On Mon, 2012-04-02 at 11:45 +0800, Changli Gao wrote:
> 
>> Thinking about a malformed tcp segment, which has no data but silly
>> options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we
>> will try to dereference one byte over the boundary when parsing the
>> options. I know we have skb_shared_info at the end and it won't cause
>> any crash, but should we rely on this fact?
>> 
> 
> No we cant rely on this, kmemcheck might barf on us.

Give me a break.

The code does the right thing, in every possible case, and
in every possible valid state of an SKB.

If we can't make kmemcheck handle that, tough, I'm not adding useless
tests to a function, specifically tests which are always there and
that don't fix anything.

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

[Eric Dumazet]

On Sun, 2012-04-01 at 23:57 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 02 Apr 2012 05:53:17 +0200
> 
> > On Mon, 2012-04-02 at 11:45 +0800, Changli Gao wrote:
> > 
> >> Thinking about a malformed tcp segment, which has no data but silly
> >> options, and whose last byte is neither TCPOPT_EOL or TCPOPT_NOP, we
> >> will try to dereference one byte over the boundary when parsing the
> >> options. I know we have skb_shared_info at the end and it won't cause
> >> any crash, but should we rely on this fact?
> >> 
> > 
> > No we cant rely on this, kmemcheck might barf on us.
> 
> Give me a break.

Sure. End of discussion.

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

[Changli Gao]

On Mon, Apr 2, 2012 at 11:53 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> No we cant rely on this, kmemcheck might barf on us.
>
> Your patch (and the netfilter one) is fine.
>
>

Got it. Thanks. FYI, the tcp options are copied to the stack before
being parsed.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

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

[Eric Dumazet]

On Mon, 2012-04-02 at 12:47 +0800, Changli Gao wrote:

> Got it. Thanks. FYI, the tcp options are copied to the stack before
> being parsed.
> 

What do you mean ?

code looks like :

const struct tcphdr *th = tcp_hdr(skb);
int length = (th->doff * 4) - sizeof(struct tcphdr);

ptr = (const unsigned char *)(th + 1);



Therefore ptr points somewhere in skb->head ... 

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

[Changli Gao]

On Mon, Apr 2, 2012 at 12:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2012-04-02 at 12:47 +0800, Changli Gao wrote:
>
>> Got it. Thanks. FYI, the tcp options are copied to the stack before
>> being parsed.
>>
>
> What do you mean ?
>
> code looks like :
>
> const struct tcphdr *th = tcp_hdr(skb);
> int length = (th->doff * 4) - sizeof(struct tcphdr);
>
> ptr = (const unsigned char *)(th + 1);
>
>
>
> Therefore ptr points somewhere in skb->head ...
>
>
>

Oh, sorry. I forgot to add the condition when I wrote it down. I mean
the code in netfilter.

        unsigned char buff[(15 * 4) - sizeof(struct tcphdr)];
        const unsigned char *ptr;
        int length = (tcph->doff*4) - sizeof(struct tcphdr);

        if (!length)
                return;

        ptr = skb_header_pointer(skb, dataoff + sizeof(struct tcphdr),
                                 length, buff);
        BUG_ON(ptr == NULL);

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

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

[Eric Dumazet]

On Mon, 2012-04-02 at 14:27 +0800, Changli Gao wrote:
> On Mon, Apr 2, 2012 at 12:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Mon, 2012-04-02 at 12:47 +0800, Changli Gao wrote:
> >
> >> Got it. Thanks. FYI, the tcp options are copied to the stack before
> >> being parsed.
> >>
> >
> > What do you mean ?
> >
> > code looks like :
> >
> > const struct tcphdr *th = tcp_hdr(skb);
> > int length = (th->doff * 4) - sizeof(struct tcphdr);
> >
> > ptr = (const unsigned char *)(th + 1);
> >
> >
> >
> > Therefore ptr points somewhere in skb->head ...
> >
> >
> >
> 
> Oh, sorry. I forgot to add the condition when I wrote it down. I mean
> the code in netfilter.
> 
>         unsigned char buff[(15 * 4) - sizeof(struct tcphdr)];
>         const unsigned char *ptr;
>         int length = (tcph->doff*4) - sizeof(struct tcphdr);
> 
>         if (!length)
>                 return;
> 
>         ptr = skb_header_pointer(skb, dataoff + sizeof(struct tcphdr),
>                                  length, buff);
>         BUG_ON(ptr == NULL);
> 

Doesnt really save us, skb_header_pointer() copies only if block not
directly and fully accessible in skb->head

So if skb->head contains exactly the tcp options, we still can read one
uninit byte.

So potential problem in netfilter too.

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

[Eric Dumazet]

On Sun, 2012-04-01 at 23:29 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 02 Apr 2012 05:19:33 +0200
> 
> >> @@ -3845,6 +3845,8 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o
> >>  			length--;
> >>  			continue;
> >>  		default:
> >> +			if (length < 2)
> >> +				return;
> >>  			opsize = *ptr++;
> >>  			if (opsize < 2) /* "silly options" */
> >>  				return;
> > 
> > Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Tag Eric, you're it.
> 
> You ACK'd this patch, so you get to show how this is actually able
> to cause some kind of problem.
> 
> I assert that this is adding a useless test, that doesn't fix any kind
> of possible crash or misbehavior.  If length == 1 at the default:, the
> code will absolutely do the right thing.
> 
> Prove me wrong.

No problem.

You can have NOP,NOP,NOP,EVIL-OPTION

initial length=4  (multiple of 4)

We can read 5 bytes, and access 'out of bound' memory.

Usually not a problem since we have many bytes after our head.

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Apr 2012 05:45:45 +0200

> Usually not a problem since we have many bytes after our head.

We always have bytes after the head, it's guarenteed, and whether it's
garbage bytes or skb_shared_info() it simply doesn't matter.

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

[Eric Dumazet]

On Sun, 2012-04-01 at 23:55 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 02 Apr 2012 05:45:45 +0200
> 
> > Usually not a problem since we have many bytes after our head.
> 
> We always have bytes after the head, it's guarenteed, and whether it's
> garbage bytes or skb_shared_info() it simply doesn't matter.

Then you have to add a kmemcheck_something() to make this clear and
avoid possible warnings.

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Apr 2012 05:58:25 +0200

> On Sun, 2012-04-01 at 23:55 -0400, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Mon, 02 Apr 2012 05:45:45 +0200
>> 
>> > Usually not a problem since we have many bytes after our head.
>> 
>> We always have bytes after the head, it's guarenteed, and whether it's
>> garbage bytes or skb_shared_info() it simply doesn't matter.
> 
> Then you have to add a kmemcheck_something() to make this clear and
> avoid possible warnings.

That's perfectly fine and would document the situation.  And we can
add a similar annotation to the two other nearly identical pieces of
code in net/netfilter/nf_conntrack_proto_tcp.c

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

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Mon,  2 Apr 2012 11:10:50 +0800

> We should check the length of the data before dereferencing it when parsing
> the TCP options.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

Proper Subject prefix here would be "tcp: ", not "net: "
and maybe adjust the subject line to also mention the
specific function being fixed, which in this case would
be tcp_parse_options().  So:

	tcp: Validate length of data before dereference in tcp_parse_options().

and then you can make the commit message just be your signoff.