dccp: Reponsed with Reset when packet is received with invalid

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.

---
 net/dccp/input.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index dab4cc9..df0e671 100644
@@ -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

Em Wed, Aug 20, 2008 at 09:20:07AM +0800, Wei Yongjun escreveu:

God catch! Please also keep [email protected] on the CC list.

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.

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

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

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;
}