Patchwork net: check the length of the data before dereferencing it

login
register
mail settings
Submitter Changli Gao
Date April 2, 2012, 3:10 a.m.
Message ID <1333336250-4110-1-git-send-email-xiaosuo@gmail.com>
Download mbox | patch
Permalink /patch/150053/
State Rejected
Headers show

Comments

Changli Gao - April 2, 2012, 3:10 a.m.
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(+)
--
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
Eric Dumazet - April 2, 2012, 3:19 a.m.
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>


--
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
David Miller - April 2, 2012, 3:19 a.m.
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.

--
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
David Miller - April 2, 2012, 3:29 a.m.
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.
--
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
Changli Gao - April 2, 2012, 3:45 a.m.
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?
Eric Dumazet - April 2, 2012, 3:45 a.m.
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.



--
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
Eric Dumazet - April 2, 2012, 3:53 a.m.
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.


--
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
David Miller - April 2, 2012, 3:55 a.m.
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.
--
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
David Miller - April 2, 2012, 3:57 a.m.
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.
--
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
Eric Dumazet - April 2, 2012, 3:58 a.m.
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.




--
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
Eric Dumazet - April 2, 2012, 3:59 a.m.
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.


--
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
David Miller - April 2, 2012, 4:14 a.m.
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
--
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
Changli Gao - April 2, 2012, 4:47 a.m.
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.
Eric Dumazet - April 2, 2012, 4:54 a.m.
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 ... 



--
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
Changli Gao - April 2, 2012, 6:27 a.m.
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);
Eric Dumazet - April 2, 2012, 6:43 a.m.
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.



--
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

Patch

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;