Re: [PATCH] sctp: integer overflow in sctp_auth_create_key()

Re: [PATCH] sctp: integer overflow in sctp_auth_create_key()

[Vladislav Yasevich]

On 11/22/2011 08:25 PM, Xi Wang wrote:
> The previous commit 30c2235c is incomplete and cannot prevent integer
> overflows. For example, when key_len is 0x80000000 (INT_MAX + 1), the
> left-hand side of the check, (INT_MAX - key_len), which is unsigned,
> becomes 0xffffffff (UINT_MAX) and bypasses the check.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  net/sctp/auth.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 865e68f..989e0fd 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -82,7 +82,7 @@ static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)
>  	struct sctp_auth_bytes *key;
>  
>  	/* Verify that we are not going to overflow INT_MAX */
> -	if ((INT_MAX - key_len) < sizeof(struct sctp_auth_bytes))
> +	if (key_len > INT_MAX - sizeof(struct sctp_auth_bytes))
>  		return NULL;
>  
>  	/* Allocate the shared key */


Hmm.  Yes, this is a more correct check.

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>


However, I don't think this is a security issue.  As I've written before, this function is
called from 2 places:

  1) setsockopt() code path

  2) sctp_auth_asoc_set_secret() code path

In case (1), sca_keylength is never going to exceed 65535 since it's
bounded by a u16 from the user api.  As such, The integer promotion will
not impact anything and the malloc() will never overflow.

In case (2), sca_keylength is computed based on the key the user provided
(MAX_USHORT) and the combination of protocol negotiated data where that
combination has a max size of 3 * MAX_USHORT (see sctp_auth_make_key_vector()).
So, even this case, our maximum key length can be 4* MAX_USHORT which still
will always be below MAX_INT and will not overflow.

So, I don't think there is big security consideration here, just a bad
check that just happens to always work.

-vlad

Re: [PATCH] sctp: integer overflow in sctp_auth_create_key()

[Xi Wang]

I agree that this is not a security issue if key_len can never get large.

So how about just removing the overflow check at all?

- xi

On Nov 28, 2011, at 10:45 AM, Vladislav Yasevich wrote:
> 
> Hmm.  Yes, this is a more correct check.
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> 
> 
> However, I don't think this is a security issue.  As I've written before, this function is
> called from 2 places:
> 
>  1) setsockopt() code path
> 
>  2) sctp_auth_asoc_set_secret() code path
> 
> In case (1), sca_keylength is never going to exceed 65535 since it's
> bounded by a u16 from the user api.  As such, The integer promotion will
> not impact anything and the malloc() will never overflow.
> 
> In case (2), sca_keylength is computed based on the key the user provided
> (MAX_USHORT) and the combination of protocol negotiated data where that
> combination has a max size of 3 * MAX_USHORT (see sctp_auth_make_key_vector()).
> So, even this case, our maximum key length can be 4* MAX_USHORT which still
> will always be below MAX_INT and will not overflow.
> 
> So, I don't think there is big security consideration here, just a bad
> check that just happens to always work.
> 
> -vlad

Re: [PATCH] sctp: integer overflow in sctp_auth_create_key()

[Vladislav Yasevich]

On 11/29/2011 02:33 AM, Xi Wang wrote:
> I agree that this is not a security issue if key_len can never get large.
> 
> So how about just removing the overflow check at all?

That should be ok as well.  There is an overflow guard in the api
entry point so that should guard against overflows from user space.

On the network end I miscalculated a little.  The key is actually made up
of user_key (1 short) + 2 * key_vector (3 shorts) for a total of 7*MAX_USHORT;
however, that still will not overflow 32 bits.

-vlad

> 
> - xi
> 
> On Nov 28, 2011, at 10:45 AM, Vladislav Yasevich wrote:
>>
>> Hmm.  Yes, this is a more correct check.
>>
>> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>>
>>
>> However, I don't think this is a security issue.  As I've written before, this function is
>> called from 2 places:
>>
>>  1) setsockopt() code path
>>
>>  2) sctp_auth_asoc_set_secret() code path
>>
>> In case (1), sca_keylength is never going to exceed 65535 since it's
>> bounded by a u16 from the user api.  As such, The integer promotion will
>> not impact anything and the malloc() will never overflow.
>>
>> In case (2), sca_keylength is computed based on the key the user provided
>> (MAX_USHORT) and the combination of protocol negotiated data where that
>> combination has a max size of 3 * MAX_USHORT (see sctp_auth_make_key_vector()).
>> So, even this case, our maximum key length can be 4* MAX_USHORT which still
>> will always be below MAX_INT and will not overflow.
>>
>> So, I don't think there is big security consideration here, just a bad
>> check that just happens to always work.
>>
>> -vlad
> 
> --
> 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
> 

Re: [PATCH] sctp: integer overflow in sctp_auth_create_key()

[Xi Wang]

Thanks for clarifying this!

I will leave the check there and incorporate your comments into a new patch.

- xi

On Nov 29, 2011, at 10:03 AM, Vladislav Yasevich wrote:
> 
> That should be ok as well.  There is an overflow guard in the api
> entry point so that should guard against overflows from user space.
> 
> On the network end I miscalculated a little.  The key is actually made up
> of user_key (1 short) + 2 * key_vector (3 shorts) for a total of 7*MAX_USHORT;
> however, that still will not overflow 32 bits.
> 
> -vlad

[PATCH v2] sctp: better integer overflow check in sctp_auth_create_key()

[Xi Wang]

The check from commit 30c2235c is incomplete and cannot prevent
cases like key_len = 0x80000000 (INT_MAX + 1).  In that case, the
left-hand side of the check (INT_MAX - key_len), which is unsigned,
becomes 0xffffffff (UINT_MAX) and bypasses the check.

However this shouldn't be a security issue.  The function is called
from the following two code paths:

 1) setsockopt()

 2) sctp_auth_asoc_set_secret()

In case (1), sca_keylength is never going to exceed 65535 since it's
bounded by a u16 from the user API.  As such, the key length will
never overflow.

In case (2), sca_keylength is computed based on the user key (1 short)
and 2 * key_vector (3 shorts) for a total of 7 * USHRT_MAX, which still
will not overflow.

In other words, this overflow check is not really necessary.  Just
make it more correct.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
Cc: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 net/sctp/auth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 865e68f..bf81204 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -82,7 +82,7 @@ static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)
 	struct sctp_auth_bytes *key;
 
 	/* Verify that we are not going to overflow INT_MAX */
-	if ((INT_MAX - key_len) < sizeof(struct sctp_auth_bytes))
+	if (key_len > (INT_MAX - sizeof(struct sctp_auth_bytes)))
 		return NULL;
 
 	/* Allocate the shared key */
-- 
1.7.5.4

Re: [PATCH v2] sctp: better integer overflow check in sctp_auth_create_key()

[David Miller]

From: Xi Wang <xi.wang@gmail.com>
Date: Tue, 29 Nov 2011 14:26:30 -0500

> The check from commit 30c2235c is incomplete and cannot prevent
> cases like key_len = 0x80000000 (INT_MAX + 1).  In that case, the
> left-hand side of the check (INT_MAX - key_len), which is unsigned,
> becomes 0xffffffff (UINT_MAX) and bypasses the check.
> 
> However this shouldn't be a security issue.  The function is called
> from the following two code paths:
> 
>  1) setsockopt()
> 
>  2) sctp_auth_asoc_set_secret()
> 
> In case (1), sca_keylength is never going to exceed 65535 since it's
> bounded by a u16 from the user API.  As such, the key length will
> never overflow.
> 
> In case (2), sca_keylength is computed based on the user key (1 short)
> and 2 * key_vector (3 shorts) for a total of 7 * USHRT_MAX, which still
> will not overflow.
> 
> In other words, this overflow check is not really necessary.  Just
> make it more correct.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Cc: Vlad Yasevich <vladislav.yasevich@hp.com>

I already applied your patch, you cannot just post a patch as if
it hasn't been applied to the tree, it doesn't work like that.

Once I've applied one of your patches, it is "cast in stone" and
cannot be reverted.  You must therefore develop relative to the
change.