[PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Vlad Yasevich]

The structure used for SCTP_AUTH_KEY option contains a
length that needs to be verfied to prevent buffer overflow
conditions.  Spoted by Eugene Teo <eteo@redhat.com>.

Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 net/sctp/auth.c   |    4 ++++
 net/sctp/socket.c |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 675a5c3..1fcb4cf 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -80,6 +80,10 @@ 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))
+		return NULL;
+
 	/* Allocate the shared key */
 	key = kmalloc(sizeof(struct sctp_auth_bytes) + key_len, gfp);
 	if (!key)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index bb5c9ef..afa952e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3144,6 +3144,11 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
 		goto out;
 	}
 
+	if (authkey->sca_keylength > optlen) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	asoc = sctp_id2assoc(sk, authkey->sca_assoc_id);
 	if (!asoc && authkey->sca_assoc_id && sctp_style(sk, UDP)) {
 		ret = -EINVAL;
-- 
1.5.2.5

Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Mon, 25 Aug 2008 16:25:32 -0400

> The structure used for SCTP_AUTH_KEY option contains a
> length that needs to be verfied to prevent buffer overflow
> conditions.  Spoted by Eugene Teo <eteo@redhat.com>.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied and queued up for -stable, thanks!

Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Eugene Teo]

Vlad Yasevich wrote:
> The structure used for SCTP_AUTH_KEY option contains a
> length that needs to be verfied to prevent buffer overflow
> conditions.  Spoted by Eugene Teo <eteo@redhat.com>.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
>  net/sctp/auth.c   |    4 ++++
>  net/sctp/socket.c |    5 +++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 675a5c3..1fcb4cf 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -80,6 +80,10 @@ static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)

This should be __u16 key_len.

>  {
>  	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))
> +		return NULL;

Shouldn't this be UINT_MAX? But then if you are going to change
sctp_auth_create_key() to accept __u16 key_len, then it should be
USHORT_MAX.

>  	/* Allocate the shared key */
>  	key = kmalloc(sizeof(struct sctp_auth_bytes) + key_len, gfp);
>  	if (!key)
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bb5c9ef..afa952e 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3144,6 +3144,11 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
>  		goto out;
>  	}
>  
> +	if (authkey->sca_keylength > optlen) {
> +		ret = -EINVAL;
> +		goto out;

Is there a better upper bound check?

>  	asoc = sctp_id2assoc(sk, authkey->sca_assoc_id);
>  	if (!asoc && authkey->sca_assoc_id && sctp_style(sk, UDP)) {
>  		ret = -EINVAL;

Thanks,
Eugene

Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Linus Torvalds]

On Tue, 26 Aug 2008, Eugene Teo wrote:
>
> > @@ -80,6 +80,10 @@ static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)
> 
> This should be __u16 key_len.

Hmm? If it fits in a u16, then there is no worry about overflow.

> >  	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))
> > +		return NULL;
> 
> Shouldn't this be UINT_MAX? But then if you are going to change
> sctp_auth_create_key() to accept __u16 key_len, then it should be
> USHORT_MAX.

If it's ushort, then it shouldn't need any test at all from an overflow 
standpoint. The addition simply can't overflow, since it's always done in 
"size_t" due to the sizeof.

But if it can overflow, I actually think it makes more sense to test for 
something smaller than the "exact" overflow. A key can't reasonably be all 
that long _anyway_, so it's probably better to test for something _much_ 
smaller.

		Linus

Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Vlad Yasevich]

Eugene Teo wrote:
> Vlad Yasevich wrote:
>> The structure used for SCTP_AUTH_KEY option contains a
>> length that needs to be verfied to prevent buffer overflow
>> conditions.  Spoted by Eugene Teo <eteo@redhat.com>.
>>
>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>> ---
>>  net/sctp/auth.c   |    4 ++++
>>  net/sctp/socket.c |    5 +++++
>>  2 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
>> index 675a5c3..1fcb4cf 100644
>> --- a/net/sctp/auth.c
>> +++ b/net/sctp/auth.c
>> @@ -80,6 +80,10 @@ static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)
> 
> This should be __u16 key_len.
> 
>>  {
>>  	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))
>> +		return NULL;
> 
> Shouldn't this be UINT_MAX? But then if you are going to change
> sctp_auth_create_key() to accept __u16 key_len, then it should be
> USHORT_MAX.
> 

I'd rather keep it a u32 since this function is not only for userspace,
but for creating a generated key.

Yes, UINT_MAX makes more sense.

>>  	/* Allocate the shared key */
>>  	key = kmalloc(sizeof(struct sctp_auth_bytes) + key_len, gfp);
>>  	if (!key)
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index bb5c9ef..afa952e 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -3144,6 +3144,11 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
>>  		goto out;
>>  	}
>>  
>> +	if (authkey->sca_keylength > optlen) {
>> +		ret = -EINVAL;
>> +		goto out;
> 
> Is there a better upper bound check?

Hm...  optlen - sizeof(struct sctp_authkey)  is more accurate.

There is really no other bound.

-vlad

> 
>>  	asoc = sctp_id2assoc(sk, authkey->sca_assoc_id);
>>  	if (!asoc && authkey->sca_assoc_id && sctp_style(sk, UDP)) {
>>  		ret = -EINVAL;
> 
> Thanks,
> Eugene
> 

Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Linus Torvalds]

On Mon, 25 Aug 2008, Linus Torvalds wrote:
> 
> But if it can overflow, I actually think it makes more sense to test for 
> something smaller than the "exact" overflow. A key can't reasonably be all 
> that long _anyway_, so it's probably better to test for something _much_ 
> smaller.

IOW, I think it would make _more_ sense to just declare some "key size 
max", and do something like

	#define SCTP_AUTH_KEY_SIZE (65536)

	.. 
	if (key_len > SCTP_AUTH_KEY_SIZE)
		return -EINVAL;

kind of thing.

		Linus

Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Eugene Teo]

Vlad Yasevich wrote:
> Eugene Teo wrote:
>> Vlad Yasevich wrote:
[...]
>>> +	if (authkey->sca_keylength > optlen) {
>>> +		ret = -EINVAL;
>>> +		goto out;
>> Is there a better upper bound check?
> 
> Hm...  optlen - sizeof(struct sctp_authkey)  is more accurate.
> 
> There is really no other bound.

Linus suggested that it is better to declare an upper bound for key_len.
I think it makes a lot of sense as a key shouldn't be as long as the
boundary limit of its declared data type.

Thanks,
Eugene

Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option

[Vlad Yasevich]

Eugene Teo wrote:
> Vlad Yasevich wrote:
>> Eugene Teo wrote:
>>> Vlad Yasevich wrote:
> [...]
>>>> +	if (authkey->sca_keylength > optlen) {
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>> Is there a better upper bound check?
>> Hm...  optlen - sizeof(struct sctp_authkey)  is more accurate.
>>
>> There is really no other bound.
> 
> Linus suggested that it is better to declare an upper bound for key_len.
> I think it makes a lot of sense as a key shouldn't be as long as the
> boundary limit of its declared data type.
> 
>

I am starting to think that this might be completely unnecessary.

The sctp_auth_set_key() function is be called from two places:

  1) setsockopt() code path

  2) sctp_auth_asoc_set_secret() code path


In case (1), sca_keylength is never going to exceed 65536 since it's
bounded by a u16 from the user api.  As such, the malloc() will never
overflow.  This is the case we were really concerned about since the user
can supply bogus values.

In case (2), it is possible for the length to exceed 65536 since it's
trying to create a generated key and the algorithm takes raw pieces of the
SCTP handshake packets to do that.  However, there is no way that they can
exceed another 132K bytes because max IP datagram size is 65K and both ends
have to exchange security data.  Using IPv6 Jumbo extension I guess they can
go higher but we are probably going to fail memory allocations at that point.

That makes the theoretical maximum for the key to be 192K, but that discounts
the Jumbo usage.  So, I think using the current INT_MAX is sufficient to catch
possible overflows caused by case (2).   I think restricting the size further is
pointless and could end up being too restrictive.

-vlad