[PATCH] dccp: Reponsed with Reset when packet is received with invalid

[PATCH] dccp: Reponsed with Reset when packet is received with invalid

[Wei Yongjun]

RFC4340 said that if a packet is received with invalid option(such as Mandatory
Option as the last byte of the option list), endpoint should reponsed with
Reset. In LISTIN state and RESPOND state, the endpoint reponsed with reset
correctly, but in REQUEST state and OPEN state, the endpoint just ignored
the packet. The packet sequence is as the following:

Case 1:

  Endpoint A                           Endpoint B
  (CLOSED)                             (CLOSED)

               <----------------       REQUEST

  RESPONSE     ----------------->      (*1)
  (with invalid option)
               <----------------       RESET
                                       (with Reset Code 5, "Option Error")

  (*1) it just be ignored currently, no reset is sent

Case 2:

  Endpoint A                           Endpoint B
  (OPEN)                               (OPEN)

  DATA-ACK     ----------------->      (*2)
  (with invalid option)
               <----------------       RESET
                                       (with Reset Code 5, "Option Error")

  (*2) it just be ignored currently, no reset is sent

This patch fixed the problem by reponsed with Reset instead of ignore packet.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/dccp/input.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/dccp/input.c b/net/dccp/input.c
index dab4cc9..df0e671 100644
--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -370,7 +370,7 @@ int dccp_rcv_established(struct sock *sk, struct sk_buff *skb,
 		goto discard;
 
 	if (dccp_parse_options(sk, NULL, skb))
-		goto discard;
+		return 1;
 
 	dccp_handle_ackvec_processing(sk, skb);
 	dccp_deliver_input_to_ccids(sk, skb);
@@ -631,7 +631,7 @@ int dccp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 
 	/*  Step 8: Process options */
 	if (dccp_parse_options(sk, NULL, skb))
-		goto discard;
+		return 1;
 
 	/*
 	 *  Step 9: Process Reset
-- 
1.5.3.8

Re: [PATCH] dccp: Reponsed with Reset when packet is received with

[Arnaldo Carvalho de Melo]

Em Wed, Aug 20, 2008 at 09:20:07AM +0800, Wei Yongjun escreveu:
> RFC4340 said that if a packet is received with invalid option(such as Mandatory
> Option as the last byte of the option list), endpoint should reponsed with
> Reset. In LISTIN state and RESPOND state, the endpoint reponsed with reset
> correctly, but in REQUEST state and OPEN state, the endpoint just ignored
> the packet. The packet sequence is as the following:
>
> Case 1:
>
>  Endpoint A                           Endpoint B
>  (CLOSED)                             (CLOSED)
>
>               <----------------       REQUEST
>
>  RESPONSE     ----------------->      (*1)
>  (with invalid option)
>               <----------------       RESET
>                                       (with Reset Code 5, "Option Error")
>
>  (*1) it just be ignored currently, no reset is sent
>
> Case 2:
>
>  Endpoint A                           Endpoint B
>  (OPEN)                               (OPEN)
>
>  DATA-ACK     ----------------->      (*2)
>  (with invalid option)
>               <----------------       RESET
>                                       (with Reset Code 5, "Option Error")
>
>  (*2) it just be ignored currently, no reset is sent
>
> This patch fixed the problem by reponsed with Reset instead of ignore packet.
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

God catch! Please also keep netdev@vger.kernel.org on the CC list.

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

> ---
> net/dccp/input.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/dccp/input.c b/net/dccp/input.c
> index dab4cc9..df0e671 100644
> --- a/net/dccp/input.c
> +++ b/net/dccp/input.c
> @@ -370,7 +370,7 @@ int dccp_rcv_established(struct sock *sk, struct sk_buff *skb,
> 		goto discard;
>
> 	if (dccp_parse_options(sk, NULL, skb))
> -		goto discard;
> +		return 1;
>
> 	dccp_handle_ackvec_processing(sk, skb);
> 	dccp_deliver_input_to_ccids(sk, skb);
> @@ -631,7 +631,7 @@ int dccp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>
> 	/*  Step 8: Process options */
> 	if (dccp_parse_options(sk, NULL, skb))
> -		goto discard;
> +		return 1;
>
> 	/*
> 	 *  Step 9: Process Reset
> -- 
> 1.5.3.8
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dccp: Reponsed with Reset when packet is received with invalid option

[Arnaldo Carvalho de Melo]

Em Wed, Aug 20, 2008 at 09:20:07AM +0800, Wei Yongjun escreveu:
> RFC4340 said that if a packet is received with invalid option(such as Mandatory
> Option as the last byte of the option list), endpoint should reponsed with
> Reset. In LISTIN state and RESPOND state, the endpoint reponsed with reset
> correctly, but in REQUEST state and OPEN state, the endpoint just ignored
> the packet. The packet sequence is as the following:
>
> Case 1:
>
>  Endpoint A                           Endpoint B
>  (CLOSED)                             (CLOSED)
>
>               <----------------       REQUEST
>
>  RESPONSE     ----------------->      (*1)
>  (with invalid option)
>               <----------------       RESET
>                                       (with Reset Code 5, "Option Error")
>
>  (*1) it just be ignored currently, no reset is sent
>
> Case 2:
>
>  Endpoint A                           Endpoint B
>  (OPEN)                               (OPEN)
>
>  DATA-ACK     ----------------->      (*2)
>  (with invalid option)
>               <----------------       RESET
>                                       (with Reset Code 5, "Option Error")
>
>  (*2) it just be ignored currently, no reset is sent
>
> This patch fixed the problem by reponsed with Reset instead of ignore packet.
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

God catch! Please also keep netdev@vger.kernel.org on the CC list.

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

> ---
> net/dccp/input.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/dccp/input.c b/net/dccp/input.c
> index dab4cc9..df0e671 100644
> --- a/net/dccp/input.c
> +++ b/net/dccp/input.c
> @@ -370,7 +370,7 @@ int dccp_rcv_established(struct sock *sk, struct sk_buff *skb,
> 		goto discard;
>
> 	if (dccp_parse_options(sk, NULL, skb))
> -		goto discard;
> +		return 1;
>
> 	dccp_handle_ackvec_processing(sk, skb);
> 	dccp_deliver_input_to_ccids(sk, skb);
> @@ -631,7 +631,7 @@ int dccp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>
> 	/*  Step 8: Process options */
> 	if (dccp_parse_options(sk, NULL, skb))
> -		goto discard;
> +		return 1;
>
> 	/*
> 	 *  Step 9: Process Reset
> -- 
> 1.5.3.8
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dccp: Reponsed with Reset when packet is received with

[Gerrit Renker]

Thanks a lot for this patch -- I will integrate it into the test tree
and upload the updated version (include Acked-by's) tomorrow morning.

> RFC4340 said that if a packet is received with invalid option(such as Mandatory
> Option as the last byte of the option list),
While looking at dccp_parse_options, it seems to me that this case
(Mandatory as the last option, or a one or more OK-options followed by
 a faulty one) may use the wrong Reset code, since the `rc' variable
is assigned different values while looping through the header options.
Did you notice such a case?

In any case, I will be adding 

--- a/net/dccp/options.c
+++ b/net/dccp/options.c
@@ -243,6 +243,7 @@ ignore_option:
 	}
 
 	/* mandatory was the last byte in option list -> reset connection */
+	rc = DCCP_RESET_CODE_OPTION_ERROR;
 	if (mandatory)
 		goto out_invalid_option;
 
to make sure that previous assignments to `rc' do not override the reset code.

Gerrit

Re: [PATCH] dccp: Reponsed with Reset when packet is received with

[Wei Yongjun]

Gerrit Renker wrote:
> Thanks a lot for this patch -- I will integrate it into the test tree
> and upload the updated version (include Acked-by's) tomorrow morning.
>
>   
>> RFC4340 said that if a packet is received with invalid option(such as Mandatory
>> Option as the last byte of the option list),
>>     
> While looking at dccp_parse_options, it seems to me that this case
> (Mandatory as the last option, or a one or more OK-options followed by
>  a faulty one) may use the wrong Reset code, since the `rc' variable
> is assigned different values while looping through the header options.
> Did you notice such a case?
>
> In any case, I will be adding 
>
> --- a/net/dccp/options.c
> +++ b/net/dccp/options.c
> @@ -243,6 +243,7 @@ ignore_option:
>  	}
>  
>  	/* mandatory was the last byte in option list -> reset connection */
> +	rc = DCCP_RESET_CODE_OPTION_ERROR;
>  	if (mandatory)
>  		goto out_invalid_option;
>   

This patch seems not correctly. The patch may look like this :
    while (opt_ptr != opt_end) {         
         rc = DCCP_RESET_CODE_OPTION_ERROR;
    ...
    }

rc may be override by process change option, if is OK, rc become 0. So 
the code "goto out_invalid_option;" in while statements will get reset 
code 0.

Regards
Wei Yongjun

Re: [PATCH] dccp: Reponsed with Reset when packet is received with

[Gerrit Renker]

>> In any case, I will be adding 
>>
>> --- a/net/dccp/options.c
>> +++ b/net/dccp/options.c
>> @@ -243,6 +243,7 @@ ignore_option:
>>  	}
>>   	/* mandatory was the last byte in option list -> reset connection */
>> +	rc = DCCP_RESET_CODE_OPTION_ERROR;
>>  	if (mandatory)
>>  		goto out_invalid_option;
>>   
>
> This patch seems not correctly. The patch may look like this :
>    while (opt_ptr != opt_end) {                 rc = 
> DCCP_RESET_CODE_OPTION_ERROR;
>    ...
>    }
>
> rc may be override by process change option, if is OK, rc become 0. So  
> the code "goto out_invalid_option;" in while statements will get reset  
> code 0.
>
You are right. In addition the above was not a good idea and had lead to
revising the test tree patch which set the reset code. The update resulted
in the abbreviated control flow shown below, with the cases:

 1. Header options with nonsensical lengths (-> out_nonsensical_length)
    These are now ignored as per 5.8 (page 30), as well as any options
    following the first erratic option.
 2. Invalid header options
    These cover mainly cases from 5.8, 5.8.2, and 6.6.8, in particular:
    - Mandatory option follows Mandatory option;
    - Mandatory was the last option in the list;
    - invalid per-option length (defined for each option individually);
    - failure of ccid_hc_{rx,tx}_parse_options()
      => at the moment, only ccid3_hc_tx_parse_options() is defined,
         and this also fails only if the per-option length is invalid.
 3. Invalid feature-negotiation option
    The feature-negotiation options have special semantics, as defined in
    6.6.7, 6.6.8, and 6.6.9. This is why the `rc' code is set by the
    feature-negotiation code. In all other current cases, "Option Error"
    is returned.

While at it, I noticed that there was an error in the third patch I had
submitted a few days ago. So thanks as always for looking through the
code, will submit/upload a revised version.

Gerrit

/* ------------------> updated control flow <------------------------- */
int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
{
	int rc;

	// ...
	switch ()  
		// ...
		case DCCPO_CHANGE_L ... DCCPO_CONFIRM_R:
			if (pkt_type = DCCP_PKT_DATA)      /* RFC 4340, 6 */
				break;
			rc = dccp_feat_parse_options(sk, dreq, mandatory, opt,
						    *value, value + 1, len - 1);
			if (rc)
				goto out_invalid_option;
			break;
	// ...			

	/* mandatory was the last byte in option list -> reset connection */
	if (mandatory)
		goto out_invalid_option;

out_nonsensical_length:
	/* RFC 4340, 5.8: ignore option and all remaining option space */
	return 0;

out_invalid_option:
	DCCP_INC_STATS_BH(DCCP_MIB_INVALIDOPT);
	rc = DCCP_RESET_CODE_OPTION_ERROR;
out_featneg_failed:
	DCCP_WARN("DCCP(%p): Option %d (len=%d) error=%u\n", sk, opt, len, rc);
	DCCP_SKB_CB(skb)->dccpd_reset_code = rc;
	DCCP_SKB_CB(skb)->dccpd_reset_data[0] = opt;
	DCCP_SKB_CB(skb)->dccpd_reset_data[1] = len > 0 ? value[0] : 0;
	DCCP_SKB_CB(skb)->dccpd_reset_data[2] = len > 1 ? value[1] : 0;
	return -1;
}