* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
@ 2008-08-26 0:59 ` Eugene Teo
0 siblings, 0 replies; 16+ messages in thread
From: Eugene Teo @ 2008-08-26 0:59 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: davem, netdev, linux-sctp, security
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY
2008-08-26 0:59 ` Eugene Teo
@ 2008-08-26 1:19 ` Linus Torvalds
-1 siblings, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2008-08-26 1:19 UTC (permalink / raw)
To: Eugene Teo; +Cc: Vlad Yasevich, netdev, linux-sctp, security, davem
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
@ 2008-08-26 1:19 ` Linus Torvalds
0 siblings, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2008-08-26 1:19 UTC (permalink / raw)
To: Eugene Teo; +Cc: Vlad Yasevich, netdev, linux-sctp, security, davem
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY
2008-08-26 1:19 ` [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option Linus Torvalds
@ 2008-08-26 1:28 ` Linus Torvalds
-1 siblings, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2008-08-26 1:28 UTC (permalink / raw)
To: Eugene Teo; +Cc: Vlad Yasevich, netdev, linux-sctp, security, davem
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
@ 2008-08-26 1:28 ` Linus Torvalds
0 siblings, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2008-08-26 1:28 UTC (permalink / raw)
To: Eugene Teo; +Cc: Vlad Yasevich, netdev, linux-sctp, security, davem
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
2008-08-26 0:59 ` Eugene Teo
@ 2008-08-26 1:24 ` Vlad Yasevich
-1 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2008-08-26 1:24 UTC (permalink / raw)
To: Eugene Teo; +Cc: davem, netdev, linux-sctp, security
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
@ 2008-08-26 1:24 ` Vlad Yasevich
0 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2008-08-26 1:24 UTC (permalink / raw)
To: Eugene Teo; +Cc: davem, netdev, linux-sctp, security
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
2008-08-26 1:24 ` Vlad Yasevich
@ 2008-08-26 3:01 ` Eugene Teo
-1 siblings, 0 replies; 16+ messages in thread
From: Eugene Teo @ 2008-08-26 3:01 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: davem, netdev, linux-sctp, security
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
@ 2008-08-26 3:01 ` Eugene Teo
0 siblings, 0 replies; 16+ messages in thread
From: Eugene Teo @ 2008-08-26 3:01 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: davem, netdev, linux-sctp, security
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
2008-08-26 3:01 ` Eugene Teo
@ 2008-08-26 13:37 ` Vlad Yasevich
-1 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2008-08-26 13:37 UTC (permalink / raw)
To: Eugene Teo; +Cc: davem, netdev, linux-sctp, security
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option
@ 2008-08-26 13:37 ` Vlad Yasevich
0 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2008-08-26 13:37 UTC (permalink / raw)
To: Eugene Teo; +Cc: davem, netdev, linux-sctp, security
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
^ permalink raw reply [flat|nested] 16+ messages in thread