[PATCH net 0/2] two small fixes for sctp key usage

[PATCH net 0/2] two small fixes for sctp key usage

[Daniel Borkmann]

Cryptographically used keys should be zeroed out when our session
ends resp. memory is freed, thus do not leave them somewhere in the
memory.

Daniel Borkmann (2):
  net: sctp: sctp_setsockopt_auth_key: use kzfree instead of kfree
  net: sctp: sctp_endpoint_free: zero out secret key data

 net/sctp/endpointola.c | 5 +++++
 net/sctp/socket.c      | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
1.7.11.7

[PATCH net 0/2] two small fixes for sctp key usage

[Daniel Borkmann]

Cryptographically used keys should be zeroed out when our session
ends resp. memory is freed, thus do not leave them somewhere in the
memory.

Daniel Borkmann (2):
  net: sctp: sctp_setsockopt_auth_key: use kzfree instead of kfree
  net: sctp: sctp_endpoint_free: zero out secret key data

 net/sctp/endpointola.c | 5 +++++
 net/sctp/socket.c      | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
1.7.11.7

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

[Daniel Borkmann]

In sctp_setsockopt_auth_key, we create a temporary copy of the user
passed shared auth key for the endpoint or association and after
internal setup, we free it right away. Since it's sensitive data, we
should zero out the key before returning the memory back to the
allocator. Thus, use kzfree instead of kfree, just as we do in
sctp_auth_key_put().

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

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9e65758..cedd9bf 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3390,7 +3390,7 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
 
 	ret = sctp_auth_set_key(sctp_sk(sk)->ep, asoc, authkey);
 out:
-	kfree(authkey);
+	kzfree(authkey);
 	return ret;
 }
 
-- 
1.7.11.7

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

[Daniel Borkmann]

In sctp_setsockopt_auth_key, we create a temporary copy of the user
passed shared auth key for the endpoint or association and after
internal setup, we free it right away. Since it's sensitive data, we
should zero out the key before returning the memory back to the
allocator. Thus, use kzfree instead of kfree, just as we do in
sctp_auth_key_put().

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

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9e65758..cedd9bf 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3390,7 +3390,7 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
 
 	ret = sctp_auth_set_key(sctp_sk(sk)->ep, asoc, authkey);
 out:
-	kfree(authkey);
+	kzfree(authkey);
 	return ret;
 }
 
-- 
1.7.11.7

[PATCH net 2/2] net: sctp: sctp_endpoint_free: zero out secret key data

[Daniel Borkmann]

On sctp_endpoint_destroy, previously used sensitive keying material
should be zeroed out before the memory is returned, as we already do
with e.g. auth keys when released.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/endpointola.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 17a001b..1a9c5fb 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -249,6 +249,8 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
 /* Final destructor for endpoint.  */
 static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 {
+	int i;
+
 	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
 
 	/* Free up the HMAC transform. */
@@ -271,6 +273,9 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 	sctp_inq_free(&ep->base.inqueue);
 	sctp_bind_addr_free(&ep->base.bind_addr);
 
+	for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
+		memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
+
 	/* Remove and free the port */
 	if (sctp_sk(ep->base.sk)->bind_hash)
 		sctp_put_port(ep->base.sk);
-- 
1.7.11.7

[PATCH net 2/2] net: sctp: sctp_endpoint_free: zero out secret key data

[Daniel Borkmann]

On sctp_endpoint_destroy, previously used sensitive keying material
should be zeroed out before the memory is returned, as we already do
with e.g. auth keys when released.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/endpointola.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 17a001b..1a9c5fb 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -249,6 +249,8 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
 /* Final destructor for endpoint.  */
 static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 {
+	int i;
+
 	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
 
 	/* Free up the HMAC transform. */
@@ -271,6 +273,9 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 	sctp_inq_free(&ep->base.inqueue);
 	sctp_bind_addr_free(&ep->base.bind_addr);
 
+	for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
+		memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
+
 	/* Remove and free the port */
 	if (sctp_sk(ep->base.sk)->bind_hash)
 		sctp_put_port(ep->base.sk);
-- 
1.7.11.7

Re: [PATCH net 2/2] net: sctp: sctp_endpoint_free: zero out secret key data

[Vlad Yasevich]

On 02/08/2013 08:04 AM, Daniel Borkmann wrote:
> On sctp_endpoint_destroy, previously used sensitive keying material
> should be zeroed out before the memory is returned, as we already do
> with e.g. auth keys when released.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

I'll ack this, but the whole multiple cookie keys code is completely 
unused and has been all this time.  Noone uses anything other then the 
secret_key[0] since there is no changeover support anywhere.  It might 
be nice to clean that up too.

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

> ---
>   net/sctp/endpointola.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 17a001b..1a9c5fb 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -249,6 +249,8 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
>   /* Final destructor for endpoint.  */
>   static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>   {
> +	int i;
> +
>   	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>
>   	/* Free up the HMAC transform. */
> @@ -271,6 +273,9 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>   	sctp_inq_free(&ep->base.inqueue);
>   	sctp_bind_addr_free(&ep->base.bind_addr);
>
> +	for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
> +		memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
> +
>   	/* Remove and free the port */
>   	if (sctp_sk(ep->base.sk)->bind_hash)
>   		sctp_put_port(ep->base.sk);
>


c
If

Re: [PATCH net 2/2] net: sctp: sctp_endpoint_free: zero out secret key data

[Vlad Yasevich]

On 02/08/2013 08:04 AM, Daniel Borkmann wrote:
> On sctp_endpoint_destroy, previously used sensitive keying material
> should be zeroed out before the memory is returned, as we already do
> with e.g. auth keys when released.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

I'll ack this, but the whole multiple cookie keys code is completely 
unused and has been all this time.  Noone uses anything other then the 
secret_key[0] since there is no changeover support anywhere.  It might 
be nice to clean that up too.

Acked-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

> ---
>   net/sctp/endpointola.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 17a001b..1a9c5fb 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -249,6 +249,8 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
>   /* Final destructor for endpoint.  */
>   static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>   {
> +	int i;
> +
>   	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>
>   	/* Free up the HMAC transform. */
> @@ -271,6 +273,9 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>   	sctp_inq_free(&ep->base.inqueue);
>   	sctp_bind_addr_free(&ep->base.bind_addr);
>
> +	for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
> +		memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
> +
>   	/* Remove and free the port */
>   	if (sctp_sk(ep->base.sk)->bind_hash)
>   		sctp_put_port(ep->base.sk);
>


c
If

Re: [PATCH net 2/2] net: sctp: sctp_endpoint_free: zero out secret key data

[Daniel Borkmann]

On 02/08/2013 04:50 PM, Vlad Yasevich wrote:
> On 02/08/2013 08:04 AM, Daniel Borkmann wrote:
>> On sctp_endpoint_destroy, previously used sensitive keying material
>> should be zeroed out before the memory is returned, as we already do
>> with e.g. auth keys when released.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>
> I'll ack this, but the whole multiple cookie keys code is completely unused and has been all this time.  Noone uses anything other then the secret_key[0] since there is no changeover support anywhere.  It might be nice to clean that up too.

Put on my todo list for follow-up patches, thanks.

> Acked-by: Vlad Yasevich <vyasevic@redhat.com>
>
> -vlad
>
>> ---
>>   net/sctp/endpointola.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
>> index 17a001b..1a9c5fb 100644
>> --- a/net/sctp/endpointola.c
>> +++ b/net/sctp/endpointola.c
>> @@ -249,6 +249,8 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
>>   /* Final destructor for endpoint.  */
>>   static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>   {
>> +    int i;
>> +
>>       SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>>
>>       /* Free up the HMAC transform. */
>> @@ -271,6 +273,9 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>       sctp_inq_free(&ep->base.inqueue);
>>       sctp_bind_addr_free(&ep->base.bind_addr);
>>
>> +    for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
>> +        memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
>> +
>>       /* Remove and free the port */
>>       if (sctp_sk(ep->base.sk)->bind_hash)
>>           sctp_put_port(ep->base.sk);
>>
>
>
> c
> If

Re: [PATCH net 2/2] net: sctp: sctp_endpoint_free: zero out secret key data

[Daniel Borkmann]

On 02/08/2013 04:50 PM, Vlad Yasevich wrote:
> On 02/08/2013 08:04 AM, Daniel Borkmann wrote:
>> On sctp_endpoint_destroy, previously used sensitive keying material
>> should be zeroed out before the memory is returned, as we already do
>> with e.g. auth keys when released.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>
> I'll ack this, but the whole multiple cookie keys code is completely unused and has been all this time.  Noone uses anything other then the secret_key[0] since there is no changeover support anywhere.  It might be nice to clean that up too.

Put on my todo list for follow-up patches, thanks.

> Acked-by: Vlad Yasevich <vyasevic@redhat.com>
>
> -vlad
>
>> ---
>>   net/sctp/endpointola.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
>> index 17a001b..1a9c5fb 100644
>> --- a/net/sctp/endpointola.c
>> +++ b/net/sctp/endpointola.c
>> @@ -249,6 +249,8 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
>>   /* Final destructor for endpoint.  */
>>   static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>   {
>> +    int i;
>> +
>>       SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>>
>>       /* Free up the HMAC transform. */
>> @@ -271,6 +273,9 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>       sctp_inq_free(&ep->base.inqueue);
>>       sctp_bind_addr_free(&ep->base.bind_addr);
>>
>> +    for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
>> +        memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
>> +
>>       /* Remove and free the port */
>>       if (sctp_sk(ep->base.sk)->bind_hash)
>>           sctp_put_port(ep->base.sk);
>>
>
>
> c
> If

Re: [PATCH net 0/2] two small fixes for sctp key usage

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Fri,  8 Feb 2013 14:04:33 +0100

> Cryptographically used keys should be zeroed out when our session
> ends resp. memory is freed, thus do not leave them somewhere in the
> memory.
> 
> Daniel Borkmann (2):
>   net: sctp: sctp_setsockopt_auth_key: use kzfree instead of kfree
>   net: sctp: sctp_endpoint_free: zero out secret key data

Applied and queued up for -stable, thanks.

Re: [PATCH net 0/2] two small fixes for sctp key usage

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Fri,  8 Feb 2013 14:04:33 +0100

> Cryptographically used keys should be zeroed out when our session
> ends resp. memory is freed, thus do not leave them somewhere in the
> memory.
> 
> Daniel Borkmann (2):
>   net: sctp: sctp_setsockopt_auth_key: use kzfree instead of kfree
>   net: sctp: sctp_endpoint_free: zero out secret key data

Applied and queued up for -stable, thanks.