| Message ID | 1304688438.29544.16.camel@dan |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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)
|
|
> 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) > | > | > > -- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html