Skip to contentSkip to conversation

sctp: add verification checks to SCTP_AUTH_KEY option

PATCH In review Vlad Yasevich 4 participants 8 messages Aug 25, 2008 – Aug 26, 2008 1 message not in the archive

Subsystems (3)
Networking Network drivers SCTP protocol
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Vlad Yasevich

author
#1 lore raw

PATCH 1/1 sctp: add verification checks to SCTP_AUTH_KEY option

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 <[email protected]>.

Signed-off-by Vlad Yasevich <[email protected]>
---
 net/sctp/auth.c   |    4 ++++
 net/sctp/socket.c |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)
net/sctp/auth.c +4 -0
index 675a5c3..1fcb4cf 100644
@@ -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)
net/sctp/socket.c +5 -0
index bb5c9ef..afa952e 100644
@@ -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;
Signature · 1 line
1.5.2.5
#3 lore raw
In reply to Vlad Yasevich

net/sctp/auth.c static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)

Quoted 15 lines · 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 <[email protected]>.

Signed-off-by Vlad Yasevich <[email protected]>
---
 net/sctp/auth.c   |    4 ++++
 net/sctp/socket.c |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)
net/sctp/auth.c +0 -0
index 675a5c3..1fcb4cf 100644
@@ -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.

net/sctp/auth.c static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp) · line 85

Quoted 6 lines
net/sctp/auth.c +3 -0
 {
 	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.

Quoted 8 lines
net/sctp/auth.c +0 -0
 	/* Allocate the shared key */
 	key = kmalloc(sizeof(struct sctp_auth_bytes) + key_len, gfp);
 	if (!key)
net/sctp/socket.c +0 -0
index bb5c9ef..afa952e 100644

net/sctp/socket.c static int sctp_setsockopt_auth_key(struct sock *sk, · line 3149

Quoted 6 lines
net/sctp/socket.c +3 -0
@@ -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?

net/sctp/socket.c static int sctp_setsockopt_auth_key(struct sock *sk, · line 3154

Quoted 3 lines
net/sctp/socket.c +0 -0
 	asoc = sctp_id2assoc(sk, authkey->sca_assoc_id);
 	if (!asoc && authkey->sca_assoc_id && sctp_style(sk, UDP)) {
 		ret = -EINVAL;

Thanks,
Eugene

#4 lore raw
Quoted 4 lines · Eugene Teo

Review static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)

Quoted 2 lines
(quoted diff) +0 -0
@@ -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.

Quoted 9 lines

net/sctp/auth.c static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp) · line 85

Quoted 5 lines
net/sctp/auth.c +3 -0
 	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

#5 lore raw
Quoted 5 lines · Linus Torvalds

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

author
#6 lore raw
In reply to Eugene Teo
Quoted 22 lines · Eugene Teo

net/sctp/auth.c static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp)

Quoted 15 lines · 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 <[email protected]>.

Signed-off-by Vlad Yasevich <[email protected]>
---
 net/sctp/auth.c   |    4 ++++
 net/sctp/socket.c |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)
net/sctp/auth.c +0 -0
index 675a5c3..1fcb4cf 100644
@@ -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.

Quoted 3 lines
net/sctp/auth.c +0 -0
 {
 	struct sctp_auth_bytes *key;
Quoted 8 lines

net/sctp/auth.c static struct sctp_auth_bytes *sctp_auth_create_key(__u32 key_len, gfp_t gfp) · line 85

Quoted 3 lines
net/sctp/auth.c +3 -0
+	/* 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.

Quoted 11 lines
Quoted 11 lines
net/sctp/auth.c +0 -0
 	/* Allocate the shared key */
 	key = kmalloc(sizeof(struct sctp_auth_bytes) + key_len, gfp);
 	if (!key)
net/sctp/socket.c +0 -0
index bb5c9ef..afa952e 100644
@@ -3144,6 +3144,11 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
 		goto out;
 	}
Quoted 5 lines

net/sctp/socket.c static int sctp_setsockopt_auth_key(struct sock *sk, · line 3149

Quoted 3 lines
net/sctp/socket.c +3 -0
+	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

Quoted 8 lines

net/sctp/socket.c static int sctp_setsockopt_auth_key(struct sock *sk, · line 3154

Quoted 4 lines
net/sctp/socket.c +0 -0
 	asoc = sctp_id2assoc(sk, authkey->sca_assoc_id);
 	if (!asoc && authkey->sca_assoc_id && sctp_style(sk, UDP)) {
 		ret = -EINVAL;

Thanks,
Eugene

#7 lore raw
Quoted 2 lines · Vlad Yasevich
Quoted 1 line · Eugene Teo

Vlad Yasevich wrote:

[...]

Quoted 8 lines
Quoted 4 lines

net/sctp/socket.c static int sctp_setsockopt_auth_key(struct sock *sk, · line 3149

Quoted 3 lines
net/sctp/socket.c +3 -0
+	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

author
#8 lore raw
Quoted 8 lines · Eugene Teo
Quoted 2 lines · Vlad Yasevich
Quoted 1 line · Eugene Teo

Vlad Yasevich wrote:

[...]

Quoted 4 lines
Quoted 4 lines

net/sctp/socket.c static int sctp_setsockopt_auth_key(struct sock *sk, · line 3149

Quoted 3 lines
net/sctp/socket.c +3 -0
+	if (authkey->sca_keylength > optlen) {
+		ret = -EINVAL;
+		goto out;

Is there a better upper bound check?

Quoted 9 lines
Quoted 3 lines

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