[PATCH net 0/2] fixes for net/sctp/auth.c

[PATCH net 0/2] fixes for net/sctp/auth.c

[Daniel Borkmann]

This set fixes two issues in the net/sctp/auth.c implementation.

Daniel Borkmann (2):
  net: sctp: sctp_auth_make_key_vector: fix undefined ref-count behaviour
  net: sctp: sctp_auth_key_put: use kzfree instead of kfree

 net/sctp/auth.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
1.7.11.7

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

[Daniel Borkmann]

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

Signed-off-by: Daniel Borkmann <dborkman@redhat.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);
 
-- 
1.7.11.7

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

[Vlad Yasevich]

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.

-vlad

>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.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);
>
>

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

[Daniel Borkmann]

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

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.

>> Signed-off-by: Daniel Borkmann <dborkman@redhat.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);
>>
>>
>

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

[Vlad Yasevich]

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@redhat.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);
>>>
>>>
>>

[PATCH net 2/2] net: sctp: sctp_auth_key_put: use kzfree instead of kfree

[Daniel Borkmann]

For sensitive data like keying material, it is common practice to zero
out keys before returning the memory back to the allocator. Thus, use
kzfree instead of kfree.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/auth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 55f1b06..69fcd6d 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -71,7 +71,7 @@ void sctp_auth_key_put(struct sctp_auth_bytes *key)
 		return;
 
 	if (atomic_dec_and_test(&key->refcnt)) {
-		kfree(key);
+		kzfree(key);
 		SCTP_DBG_OBJCNT_DEC(keys);
 	}
 }
-- 
1.7.11.7

Re: [PATCH net 2/2] net: sctp: sctp_auth_key_put: use kzfree instead of kfree

[Vlad Yasevich]

On 02/07/2013 05:55 AM, Daniel Borkmann wrote:
> For sensitive data like keying material, it is common practice to zero
> out keys before returning the memory back to the allocator. Thus, use
> kzfree instead of kfree.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/auth.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 55f1b06..69fcd6d 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -71,7 +71,7 @@ void sctp_auth_key_put(struct sctp_auth_bytes *key)
>   		return;
>
>   	if (atomic_dec_and_test(&key->refcnt)) {
> -		kfree(key);
> +		kzfree(key);
>   		SCTP_DBG_OBJCNT_DEC(keys);
>   	}
>   }
>

Re: [PATCH net 2/2] net: sctp: sctp_auth_key_put: use kzfree instead of kfree

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu,  7 Feb 2013 11:55:37 +0100

> For sensitive data like keying material, it is common practice to zero
> out keys before returning the memory back to the allocator. Thus, use
> kzfree instead of kfree.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Applied and queued up for -stable.

Re: [PATCH net 0/2] fixes for net/sctp/auth.c

[Neil Horman]

On Thu, Feb 07, 2013 at 11:55:35AM +0100, Daniel Borkmann wrote:
> This set fixes two issues in the net/sctp/auth.c implementation.
> 
> Daniel Borkmann (2):
>   net: sctp: sctp_auth_make_key_vector: fix undefined ref-count behaviour
>   net: sctp: sctp_auth_key_put: use kzfree instead of kfree
> 
>  net/sctp/auth.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>