[PATCH] dccp: handle invalid feature options length

[PATCH] dccp: handle invalid feature options length

[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.

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)

[PATCH] dccp: handle invalid feature options length

[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.

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)

Re: [PATCH] dccp: handle invalid feature options length

[Gerrit Renker]

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)
| 
| 

-- 

Re: [PATCH] dccp: handle invalid feature options length

[Gerrit Renker]

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)
| 
| 

-- 

Re: [PATCH] dccp: handle invalid feature options length

[Dan Rosenberg]

> 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)
> | 
> | 
> 
> -- 

Re: [PATCH] dccp: handle invalid feature options length

[Dan Rosenberg]

> 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)
> | 
> | 
> 
> -- 

Re: [PATCH] dccp: handle invalid feature options length

[Gerrit Renker]

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>

Re: [PATCH] dccp: handle invalid feature options length

[Gerrit Renker]

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>

Re: [PATCH] dccp: handle invalid feature options length

[David Miller]

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!

Re: [PATCH] dccp: handle invalid feature options length

[David Miller]

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!