* [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option [not found] <20080825164440.GA504@kernel.sg> @ 2008-08-25 20:25 ` Vlad Yasevich 2008-08-25 22:17 ` David Miller 2008-08-26 0:59 ` Eugene Teo 0 siblings, 2 replies; 8+ messages in thread From: Vlad Yasevich @ 2008-08-25 20:25 UTC (permalink / raw) To: davem; +Cc: netdev, linux-sctp, security, eteo, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option 2008-08-25 20:25 ` [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option Vlad Yasevich @ 2008-08-25 22:17 ` David Miller 2008-08-26 0:59 ` Eugene Teo 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2008-08-25 22:17 UTC (permalink / raw) To: vladislav.yasevich; +Cc: netdev, linux-sctp, security, eteo 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! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option 2008-08-25 20:25 ` [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option Vlad Yasevich 2008-08-25 22:17 ` David Miller @ 2008-08-26 0:59 ` Eugene Teo 2008-08-26 1:19 ` [Security] " Linus Torvalds 2008-08-26 1:24 ` Vlad Yasevich 1 sibling, 2 replies; 8+ 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] 8+ messages in thread
* Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option 2008-08-26 0:59 ` Eugene Teo @ 2008-08-26 1:19 ` Linus Torvalds 2008-08-26 1:28 ` Linus Torvalds 2008-08-26 1:24 ` Vlad Yasevich 1 sibling, 1 reply; 8+ 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] 8+ messages in thread
* Re: [Security] [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option 2008-08-26 1:19 ` [Security] " Linus Torvalds @ 2008-08-26 1:28 ` Linus Torvalds 0 siblings, 0 replies; 8+ 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] 8+ 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:19 ` [Security] " Linus Torvalds @ 2008-08-26 1:24 ` Vlad Yasevich 2008-08-26 3:01 ` Eugene Teo 1 sibling, 1 reply; 8+ 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] 8+ 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 2008-08-26 13:37 ` Vlad Yasevich 0 siblings, 1 reply; 8+ 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] 8+ 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 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
end of thread, other threads:[~2008-08-26 13:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20080825164440.GA504@kernel.sg>
2008-08-25 20:25 ` [PATCH] sctp: add verification checks to SCTP_AUTH_KEY option Vlad Yasevich
2008-08-25 22:17 ` David Miller
2008-08-26 0:59 ` Eugene Teo
2008-08-26 1:19 ` [Security] " Linus Torvalds
2008-08-26 1:28 ` Linus Torvalds
2008-08-26 1:24 ` Vlad Yasevich
2008-08-26 3:01 ` Eugene Teo
2008-08-26 13:37 ` Vlad Yasevich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).