* [PATCH] dccp: handle invalid feature options length
@ 2011-05-06 13:27 ` Dan Rosenberg
0 siblings, 0 replies; 10+ messages in thread
From: Dan Rosenberg @ 2011-05-06 13:27 UTC (permalink / raw)
To: gerrit, davem; +Cc: dccp, netdev, linux-kernel, security
A length of zero (after subtracting two for the type and len fields) for
the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
the subtraction. The subsequent code may read past the end of the
options value buffer when parsing. I'm unsure of what the consequences
of this might be, but it's probably not good.
Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
Cc: stable@kernel.org
---
net/dccp/options.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/dccp/options.c b/net/dccp/options.c
index f06ffcf..4b2ab65 100644
--- a/net/dccp/options.c
+++ b/net/dccp/options.c
@@ -123,6 +123,8 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
case DCCPO_CHANGE_L ... DCCPO_CONFIRM_R:
if (pkt_type == DCCP_PKT_DATA) /* RFC 4340, 6 */
break;
+ if (len == 0)
+ goto out_invalid_option;
rc = dccp_feat_parse_options(sk, dreq, mandatory, opt,
*value, value + 1, len - 1);
if (rc)
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] dccp: handle invalid feature options length
2011-05-06 13:27 ` Dan Rosenberg
@ 2011-05-06 13:53 ` Gerrit Renker
-1 siblings, 0 replies; 10+ messages in thread
From: Gerrit Renker @ 2011-05-06 13:53 UTC (permalink / raw)
To: dccp
Quoting Dan Rosenberg:
| A length of zero (after subtracting two for the type and len fields) for
| the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
| the subtraction. The subsequent code may read past the end of the
| options value buffer when parsing. I'm unsure of what the consequences
| of this might be, but it's probably not good.
|
Can you please check again: did you experience this condition, to me it
seems the patch is based on reading this code.
In this case, please consider the condition before the switch statement:
/* Check if this isn't a single byte option */
if (opt > DCCPO_MAX_RESERVED) {
if (opt_ptr = opt_end)
goto out_nonsensical_length;
len = *opt_ptr++;
if (len < 2)
goto out_nonsensical_length;
The described range (DCCPO_CHANGE_L = 32 ... DCCPO_CONFIRM_R = 35) is after DCCPO_MAX_RESERVED = 31,
so that the above test applies.
Hence for these option types the len is always at least 1, so that (len - 1) >= 0.
| --- a/net/dccp/options.c
| +++ b/net/dccp/options.c
| @@ -123,6 +123,8 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
| case DCCPO_CHANGE_L ... DCCPO_CONFIRM_R:
| if (pkt_type = DCCP_PKT_DATA) /* RFC 4340, 6 */
| break;
| + if (len = 0)
| + goto out_invalid_option;
| rc = dccp_feat_parse_options(sk, dreq, mandatory, opt,
| *value, value + 1, len - 1);
| if (rc)
|
|
--
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] dccp: handle invalid feature options length
@ 2011-05-06 13:53 ` Gerrit Renker
0 siblings, 0 replies; 10+ messages in thread
From: Gerrit Renker @ 2011-05-06 13:53 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: davem, dccp, netdev, linux-kernel, security
Quoting Dan Rosenberg:
| A length of zero (after subtracting two for the type and len fields) for
| the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
| the subtraction. The subsequent code may read past the end of the
| options value buffer when parsing. I'm unsure of what the consequences
| of this might be, but it's probably not good.
|
Can you please check again: did you experience this condition, to me it
seems the patch is based on reading this code.
In this case, please consider the condition before the switch statement:
/* Check if this isn't a single byte option */
if (opt > DCCPO_MAX_RESERVED) {
if (opt_ptr == opt_end)
goto out_nonsensical_length;
len = *opt_ptr++;
if (len < 2)
goto out_nonsensical_length;
The described range (DCCPO_CHANGE_L = 32 ... DCCPO_CONFIRM_R = 35) is after DCCPO_MAX_RESERVED = 31,
so that the above test applies.
Hence for these option types the len is always at least 1, so that (len - 1) >= 0.
| --- a/net/dccp/options.c
| +++ b/net/dccp/options.c
| @@ -123,6 +123,8 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
| case DCCPO_CHANGE_L ... DCCPO_CONFIRM_R:
| if (pkt_type == DCCP_PKT_DATA) /* RFC 4340, 6 */
| break;
| + if (len == 0)
| + goto out_invalid_option;
| rc = dccp_feat_parse_options(sk, dreq, mandatory, opt,
| *value, value + 1, len - 1);
| if (rc)
|
|
--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dccp: handle invalid feature options length
2011-05-06 13:53 ` Gerrit Renker
@ 2011-05-06 14:43 ` Dan Rosenberg
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Rosenberg @ 2011-05-06 14:43 UTC (permalink / raw)
To: dccp
> Can you please check again: did you experience this condition, to me it
> seems the patch is based on reading this code.
>
I saw this while reading the code.
> In this case, please consider the condition before the switch statement:
>
> /* Check if this isn't a single byte option */
> if (opt > DCCPO_MAX_RESERVED) {
> if (opt_ptr = opt_end)
> goto out_nonsensical_length;
>
> len = *opt_ptr++;
> if (len < 2)
> goto out_nonsensical_length;
>
> The described range (DCCPO_CHANGE_L = 32 ... DCCPO_CONFIRM_R = 35) is after DCCPO_MAX_RESERVED = 31,
> so that the above test applies.
>
> Hence for these option types the len is always at least 1, so that (len - 1) >= 0.
>
You just missed the important part:
if (len < 2)
goto out_nonsensical_length;
/*
* Remove the type and len fields, leaving
* just the value size
*/
len -= 2;
If the len is 2, this check will pass, and the resulting len will be 0,
causing the underflow.
Regards,
Dan
> | --- a/net/dccp/options.c
> | +++ b/net/dccp/options.c
> | @@ -123,6 +123,8 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
> | case DCCPO_CHANGE_L ... DCCPO_CONFIRM_R:
> | if (pkt_type = DCCP_PKT_DATA) /* RFC 4340, 6 */
> | break;
> | + if (len = 0)
> | + goto out_invalid_option;
> | rc = dccp_feat_parse_options(sk, dreq, mandatory, opt,
> | *value, value + 1, len - 1);
> | if (rc)
> |
> |
>
> --
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] dccp: handle invalid feature options length
@ 2011-05-06 14:43 ` Dan Rosenberg
0 siblings, 0 replies; 10+ messages in thread
From: Dan Rosenberg @ 2011-05-06 14:43 UTC (permalink / raw)
To: Gerrit Renker; +Cc: davem, dccp, netdev, linux-kernel, security
> Can you please check again: did you experience this condition, to me it
> seems the patch is based on reading this code.
>
I saw this while reading the code.
> In this case, please consider the condition before the switch statement:
>
> /* Check if this isn't a single byte option */
> if (opt > DCCPO_MAX_RESERVED) {
> if (opt_ptr == opt_end)
> goto out_nonsensical_length;
>
> len = *opt_ptr++;
> if (len < 2)
> goto out_nonsensical_length;
>
> The described range (DCCPO_CHANGE_L = 32 ... DCCPO_CONFIRM_R = 35) is after DCCPO_MAX_RESERVED = 31,
> so that the above test applies.
>
> Hence for these option types the len is always at least 1, so that (len - 1) >= 0.
>
You just missed the important part:
if (len < 2)
goto out_nonsensical_length;
/*
* Remove the type and len fields, leaving
* just the value size
*/
len -= 2;
If the len is 2, this check will pass, and the resulting len will be 0,
causing the underflow.
Regards,
Dan
> | --- a/net/dccp/options.c
> | +++ b/net/dccp/options.c
> | @@ -123,6 +123,8 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
> | case DCCPO_CHANGE_L ... DCCPO_CONFIRM_R:
> | if (pkt_type == DCCP_PKT_DATA) /* RFC 4340, 6 */
> | break;
> | + if (len == 0)
> | + goto out_invalid_option;
> | rc = dccp_feat_parse_options(sk, dreq, mandatory, opt,
> | *value, value + 1, len - 1);
> | if (rc)
> |
> |
>
> --
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dccp: handle invalid feature options length
2011-05-06 13:27 ` Dan Rosenberg
@ 2011-05-06 19:57 ` Gerrit Renker
-1 siblings, 0 replies; 10+ messages in thread
From: Gerrit Renker @ 2011-05-06 19:57 UTC (permalink / raw)
To: dccp
Quoting Dan Rosenberg:
| A length of zero (after subtracting two for the type and len fields) for
| the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
| the subtraction. The subsequent code may read past the end of the
| options value buffer when parsing. I'm unsure of what the consequences
| of this might be, but it's probably not good.
|
Please disregard my earlier message, I erred.
Dan is right, his patch is correct and definitively valid.
A length of 0 would be cast to 0xff and then cause buffer overrun.
| Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
| Cc: stable@kernel.org
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] dccp: handle invalid feature options length
@ 2011-05-06 19:57 ` Gerrit Renker
0 siblings, 0 replies; 10+ messages in thread
From: Gerrit Renker @ 2011-05-06 19:57 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: davem, dccp, netdev, linux-kernel, security
Quoting Dan Rosenberg:
| A length of zero (after subtracting two for the type and len fields) for
| the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
| the subtraction. The subsequent code may read past the end of the
| options value buffer when parsing. I'm unsure of what the consequences
| of this might be, but it's probably not good.
|
Please disregard my earlier message, I erred.
Dan is right, his patch is correct and definitively valid.
A length of 0 would be cast to 0xff and then cause buffer overrun.
| Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
| Cc: stable@kernel.org
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dccp: handle invalid feature options length
2011-05-06 19:57 ` Gerrit Renker
@ 2011-05-06 20:04 ` David Miller
-1 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2011-05-06 20:04 UTC (permalink / raw)
To: dccp
From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Fri, 6 May 2011 21:57:33 +0200
> Quoting Dan Rosenberg:
> | A length of zero (after subtracting two for the type and len fields) for
> | the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
> | the subtraction. The subsequent code may read past the end of the
> | options value buffer when parsing. I'm unsure of what the consequences
> | of this might be, but it's probably not good.
> |
> Please disregard my earlier message, I erred.
> Dan is right, his patch is correct and definitively valid.
> A length of 0 would be cast to 0xff and then cause buffer overrun.
>
> | Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> | Cc: stable@kernel.org
> Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Great, I'll apply this, thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] dccp: handle invalid feature options length
@ 2011-05-06 20:04 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2011-05-06 20:04 UTC (permalink / raw)
To: gerrit; +Cc: drosenberg, dccp, netdev, linux-kernel, security
From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Fri, 6 May 2011 21:57:33 +0200
> Quoting Dan Rosenberg:
> | A length of zero (after subtracting two for the type and len fields) for
> | the DCCPO_{CHANGE,CONFIRM}_{L,R} options will cause an underflow due to
> | the subtraction. The subsequent code may read past the end of the
> | options value buffer when parsing. I'm unsure of what the consequences
> | of this might be, but it's probably not good.
> |
> Please disregard my earlier message, I erred.
> Dan is right, his patch is correct and definitively valid.
> A length of 0 would be cast to 0xff and then cause buffer overrun.
>
> | Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> | Cc: stable@kernel.org
> Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Great, I'll apply this, thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread