Re: [PATCH] sctp: prevent reading out-of-bounds memory

From: Vlad Yasevich
Date: Fri Sep 03 2010 - 13:11:14 EST

Next message: Christoph Lameter: "Re: [PATCH 1/3] vmalloc: pcpu_get/free_vm_areas() aren't needed onUP"
Previous message: Jean Pihet: "Re: [PATCH] tracing, perf: add more power related events"
In reply to: Dan Rosenberg: "Re: [PATCH] sctp: prevent reading out-of-bounds memory"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 09/03/2010 11:54 AM, Dan Rosenberg wrote:
> Hopefully this covers everything.
>
> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@xxxxxxxxx>
>
> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400
> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 11:52:28.239595395 -0400
> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
> /* Walk through the addrs buffer and count the number of addresses. */
> addr_buf = kaddrs;
> while (walk_size < addrs_size) {
> +
> + if (walk_size + sizeof(sa_family_t) > addrs_size) {
> + kfree(kaddrs);
> + return -EINVAL;
> + }
> +
> sa_addr = (struct sockaddr *)addr_buf;
> af = sctp_get_af_specific(sa_addr->sa_family);
>
> @@ -1002,9 +1008,14 @@ static int __sctp_connect(struct sock* s
> /* Walk through the addrs buffer and count the number of addresses. */
> addr_buf = kaddrs;
> while (walk_size < addrs_size) {
> +
> + if (walk_size + sizeof(sa_family_t) > addrs_size) {
> + err = -EINVAL;
> + goto out_free;
> + }
> +
> sa_addr = (union sctp_addr *)addr_buf;
> af = sctp_get_af_specific(sa_addr->sa.sa_family);
> - port = ntohs(sa_addr->v4.sin_port);
>
> /* If the address family is not supported or if this address
> * causes the address buffer to overflow return EINVAL.
> @@ -1013,6 +1024,8 @@ static int __sctp_connect(struct sock* s
> err = -EINVAL;
> goto out_free;
> }
> +
> + port = ntohs(sa_addr->v4.sin_port);
>
> /* Save current address so we can work with it */
> memcpy(&to, sa_addr, af->sockaddr_len);
>
>

Looks good. Now you just need to resend a clean version. :)

-vlad

>
> On Fri, Sep 3, 2010 at 11:49 AM, Vlad Yasevich
> <vladislav.yasevich@xxxxxx> wrote:
>> On 09/03/2010 10:47 AM, Dan Rosenberg wrote:
>>> Ugh, just remembered the port number is also dereferenced, so the
>>> second of these two checks needs to be expanded to the size of a
>>> sockaddr_in. Note to self: don't write patches on too little sleep.
>>> Apologies for the unnecessary traffic.
>>>
>>
>> Actually, you can move that down. Otherwise, we'd end up executing the same code
>> twice which is just silly.
>>
>> So, the code should be like this:
>> 1. see if we can get the address family.
>> 2. Get the address family.
>> 3. see if we get the sockaddr of appropriate size,
>> 4. Get that structure.
>> 5. reference fields.
>>
>> -vlad
>>
>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@xxxxxxxxx>
>>>
>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400
>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:45:08.467098052 -0400
>>> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
>>> /* Walk through the addrs buffer and count the number of addresses. */
>>> addr_buf = kaddrs;
>>> while (walk_size < addrs_size) {
>>> +
>>> + if (walk_size + sizeof(sa_family_t) > addrs_size) {
>>> + kfree(kaddrs);
>>> + return -EINVAL;
>>> + }
>>> +
>>> sa_addr = (struct sockaddr *)addr_buf;
>>> af = sctp_get_af_specific(sa_addr->sa_family);
>>>
>>> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s
>>> /* Walk through the addrs buffer and count the number of addresses. */
>>> addr_buf = kaddrs;
>>> while (walk_size < addrs_size) {
>>> +
>>> + if (walk_size + sizeof(struct sockaddr_in) > addrs_size) {
>>> + err = -EINVAL;
>>> + goto out_free;
>>> + }
>>> +
>>> sa_addr = (union sctp_addr *)addr_buf;
>>> af = sctp_get_af_specific(sa_addr->sa.sa_family);
>>> port = ntohs(sa_addr->v4.sin_port);
>>>
>>>
>>>
>>> On Fri, Sep 3, 2010 at 10:35 AM, Dan Rosenberg
>>> <dan.j.rosenberg@xxxxxxxxx> wrote:
>>>> Ha, I knew there was an easier way. Take two:
>>>>
>>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@xxxxxxxxx>
>>>>
>>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400
>>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:28:14.929595312 -0400
>>>> @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
>>>> /* Walk through the addrs buffer and count the number of addresses. */
>>>> addr_buf = kaddrs;
>>>> while (walk_size < addrs_size) {
>>>> +
>>>> + if (walk_size + sizeof(sa_family_t) > addrs_size) {
>>>> + kfree(kaddrs);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> sa_addr = (struct sockaddr *)addr_buf;
>>>> af = sctp_get_af_specific(sa_addr->sa_family);
>>>>
>>>> @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s
>>>> /* Walk through the addrs buffer and count the number of addresses. */
>>>> addr_buf = kaddrs;
>>>> while (walk_size < addrs_size) {
>>>> +
>>>> + if (walk_size + sizeof(sa_family_t) > addrs_size) {
>>>> + err = -EINVAL;
>>>> + goto out_free;
>>>> + }
>>>> +
>>>> sa_addr = (union sctp_addr *)addr_buf;
>>>> af = sctp_get_af_specific(sa_addr->sa.sa_family);
>>>> port = ntohs(sa_addr->v4.sin_port);
>>>>
>>>>
>>>>>
>>>>> Hm.. we already validate that we have the proper amount of space for a given sockaddr.
>>>>> The only thing we are missing is making sure that there is room to get the proper address
>>>>> family and I think you can do that without adding any extra variables:
>>>>>
>>>>> if (walk_size + sizeof(sa_family_t) > addr_size) {
>>>>> /* Not enough room for address family */
>>>>> kfree(kaddrs);
>>>>> return -EINVAL;
>>>>> }
>>>>>
>>>>> -vlad
>>>>>
>>>>
>>>> On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich
>>>> <vladislav.yasevich@xxxxxx> wrote:
>>>>> On 09/03/2010 09:48 AM, Dan Rosenberg wrote:
>>>>>> Two user-controlled allocations in SCTP are subsequently dereferenced
>>>>>> as sockaddr structs, without checking if the dereferenced struct
>>>>>> members fall beyond the end of the allocated chunk. There doesn't
>>>>>> appear to be any information leakage here based on how these members
>>>>>> are used and additional checking, but it's still worth fixing.
>>>>>>
>>>>>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@xxxxxxxxx>
>>>>>>
>>>>>> --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400
>>>>>> +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 09:22:06.337096825 -0400
>>>>>> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
>>>>>> int err;
>>>>>> int addrcnt = 0;
>>>>>> int walk_size = 0;
>>>>>> + unsigned int remaining = addrs_size;
>>>>>> struct sockaddr *sa_addr;
>>>>>> void *addr_buf;
>>>>>> struct sctp_af *af;
>>>>>> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
>>>>>> /* Walk through the addrs buffer and count the number of addresses. */
>>>>>> addr_buf = kaddrs;
>>>>>> while (walk_size < addrs_size) {
>>>>>> +
>>>>>> + /* Don't read out-of-bounds memory */
>>>>>> + if (remaining < sizeof(struct sockaddr)) {
>>>>>> + kfree(kaddrs);
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> sa_addr = (struct sockaddr *)addr_buf;
>>>>>> af = sctp_get_af_specific(sa_addr->sa_family);
>>>>>>
>>>>>> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
>>>>>> addrcnt++;
>>>>>> addr_buf += af->sockaddr_len;
>>>>>> walk_size += af->sockaddr_len;
>>>>>> + remaining -= af->sockaddr_len;
>>>>>> }
>>>>>>
>>>>>> /* Do the work. */
>>>>>> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s
>>>>>> void *addr_buf;
>>>>>> unsigned short port;
>>>>>> unsigned int f_flags = 0;
>>>>>> + unsigned int remaining = addrs_size;
>>>>>>
>>>>>> sp = sctp_sk(sk);
>>>>>> ep = sp->ep;
>>>>>> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s
>>>>>> /* Walk through the addrs buffer and count the number of addresses. */
>>>>>> addr_buf = kaddrs;
>>>>>> while (walk_size < addrs_size) {
>>>>>> +
>>>>>> + /* Don't read out-of-bounds memory */
>>>>>> + if (remaining < sizeof(union sctp_addr)) {
>>>>>> + err = -EINVAL;
>>>>>> + goto out_free;
>>>>>> + }
>>>>>> +
>>>>>> sa_addr = (union sctp_addr *)addr_buf;
>>>>>> af = sctp_get_af_specific(sa_addr->sa.sa_family);
>>>>>> port = ntohs(sa_addr->v4.sin_port);
>>>>>> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s
>>>>>> addrcnt++;
>>>>>> addr_buf += af->sockaddr_len;
>>>>>> walk_size += af->sockaddr_len;
>>>>>> + remaining -= af->sockaddr_len;
>>>>>> }
>>>>>>
>>>>>> /* In case the user of sctp_connectx() wants an association
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Christoph Lameter: "Re: [PATCH 1/3] vmalloc: pcpu_get/free_vm_areas() aren't needed onUP"
Previous message: Jean Pihet: "Re: [PATCH] tracing, perf: add more power related events"
In reply to: Dan Rosenberg: "Re: [PATCH] sctp: prevent reading out-of-bounds memory"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]