netdev - Re: [PATCH net 1/2] net: sctp: sctp_auth_make_key_vector: fix undefined ref-count behaviour

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5113CC04.40600@gmail.com>
Date:	Thu, 07 Feb 2013 10:45:08 -0500
From:	Vlad Yasevich <vyasevich@...il.com>
To:	Daniel Borkmann <dborkman@...hat.com>
CC:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-sctp@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: sctp: sctp_auth_make_key_vector: fix undefined
 ref-count behaviour

On 02/07/2013 10:24 AM, Daniel Borkmann wrote:
> On 02/07/2013 04:04 PM, Vlad Yasevich wrote:
>> On 02/07/2013 05:55 AM, Daniel Borkmann wrote:
>>> In sctp_auth_make_key_vector(), a sctp_auth_bytes structure is being
>>> allocated, but without setting its object reference count, thus it's
>>> initialized with a random value from the memory, which can lead to
>>> i) premature free's of this object when being put (with possible
>>> subsequent kernel panics), or ii) memory leaks when refcount has a
>>> high value.
>>>
>>> Fix this by using the appropriate sctp_auth_create_key() allocator,
>>> which performs sanity checks, sets length and the refcount, as similar
>>> done in sctp_auth_asoc_set_secret() and others. This bug seems to be
>>> present since 2007 (1f485649f529: Implement SCTP-AUTH internals).
>>
>> Not strictly a bug.  The vectors are temporary and directly freed by
>> the caller.   They are only used by sctp_auth_asoc_create_secret()
>> which builds the association secret key.  The vectors are destroyed at
>> the end of that function using kfree() thus noone really cares about
>> the refcount on them and there are no leaks.
>>
>> If you are going to convert to using sctp_auth_create_key() then you
>> need to convert the callers to user to use sctp_auth_key_put().
>> Otherwise you are leaking object counts.
>
> Thanks for your feedback!
>
> If Dave is okay with this, then:
>
>   - [PATCH net 2/2] net: sctp: sctp_auth_key_put: use kzfree instead of
> kfree
>   - [PATCH net-next] net: sctp: sctp_auth_make_key_vector: remove
> duplicate ntohs calls
>
> can be applied as is. (If necessary, I could send the first one
> _unchanged_ as a
> single patch again, since it was part of a patchset. However, it will apply
> cleanly as we have it right here.)
>

That's fine. Those 2 patches are ok.

> Then, to avoid any future confusion and to stay consistent, I'll convert
> this
> to sctp_auth_create_key() API as well and make use of the
> sctp_auth_key_put()
> in a later possible patch *after* those two have been applied.

Fine by me.

-vlad

>
>>> Signed-off-by: Daniel Borkmann <dborkman@...hat.com>
>>> ---
>>>   net/sctp/auth.c | 4 +---
>>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
>>> index 159b9bc..55f1b06 100644
>>> --- a/net/sctp/auth.c
>>> +++ b/net/sctp/auth.c
>>> @@ -205,12 +205,10 @@ static struct sctp_auth_bytes
>>> *sctp_auth_make_key_vector(
>>>           if (chunks)
>>>           len += ntohs(chunks->param_hdr.length);
>>>
>>> -    new = kmalloc(sizeof(struct sctp_auth_bytes) + len, gfp);
>>> +    new = sctp_auth_create_key(len, gfp);
>>>       if (!new)
>>>           return NULL;
>>>
>>> -    new->len = len;
>>> -
>>>       memcpy(new->data, random, ntohs(random->param_hdr.length));
>>>       offset += ntohs(random->param_hdr.length);
>>>
>>>
>>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html