| Message ID | 1237423493.32009.31.camel@Maple |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Mar 19, 2009 at 12:44:53AM +0000, John Dykstra wrote: > RFC793 says (Section 3.9) that not only should such segments be > discarded, but that an ACK should be sent to the peer. I can't see what > that accomplishes, and it seems to badly interact with fast > retransmit--under some conditions with crafted packets you can get the > two stacks ACKing each other forever. So I left that out of this patch: I think part of the original intentions for the response ack is to generate the "ack storm". In certain cases of tcp hijacking where the attacker is trying to resynchronize a tcp session after injecting a packet into the stream, an ack storm raises alerts in intrusion detection systems. Most of the times, built-in measures reset the tcp session given an unusual large number of acks (I'm not sure how or if Linux does this). This was partially the original reason I was looking into this. I noticed that Windows does not send an ack back if the received ack has a higher than expected ack number *and* higher than expected sequence number. For some well crafted tcp hijacking cases, this increases the attack success rate substantially. It's beyond my knowledge of other implications such a response ack would cause. Cheers, Oliver -- 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: John Dykstra <john.dykstra1@gmail.com> Date: Thu, 19 Mar 2009 00:44:53 +0000 > [PATCH net-next-2.6] tcp: Discard segments that ack data not yet sent > > Discard incoming packets whose ack field iincludes data not yet sent. > This is consistent with RFC 793 Section 3.9. > > Change tcp_ack() to distinguish between too-small and too-large ack > field values. Keep segments with too-large ack fields out of the fast > path, and change slow path to discard them. > > Reported-by: Oliver Zheng <mailinglists+netdev@oliverzheng.com> > Signed-off-by: John Dykstra <john.dykstra1@gmail.com> I've been mulling over this patch for more than a week :-) Let's put this into net-next-2.6 and let it cook there for a while. It is possible I'll backport this into -stable after some time. 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
> I've been mulling over this patch for more than a week :-) > > Let's put this into net-next-2.6 and let it cook there for > a while. It is possible I'll backport this into -stable > after some time. One thing that might be useful for the testing period would be explicit printk when such a new discard happens? Otherwise it would be difficult to find out if something really goes wrong. -Andi
From: Andi Kleen <andi@firstfloor.org> Date: Mon, 23 Mar 2009 13:28:06 +0100 > > I've been mulling over this patch for more than a week :-) > > > > Let's put this into net-next-2.6 and let it cook there for > > a while. It is possible I'll backport this into -stable > > after some time. > > One thing that might be useful for the testing period would > be explicit printk when such a new discard happens? Otherwise > it would be difficult to find out if something really goes wrong. No, 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/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index fae78e3..01544cd 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3587,16 +3587,19 @@ static int tcp_ack(struct sock *sk, struct sk_buff *skb, int flag) u32 prior_fackets; int prior_packets; int frto_cwnd = 0; - - /* If the ack is newer than sent or older than previous acks + + /* If the ack is older than previous acks * then we can probably ignore it. */ - if (after(ack, tp->snd_nxt)) - goto uninteresting_ack; - if (before(ack, prior_snd_una)) goto old_ack; + /* If the ack includes data we haven't sent yet, discard + * this segment (RFC793 Section 3.9). + */ + if (after(ack, tp->snd_nxt)) + goto invalid_ack; + if (after(ack, prior_snd_una)) flag |= FLAG_SND_UNA_ADVANCED; @@ -3686,6 +3689,10 @@ no_queue: tcp_ack_probe(sk); return 1; +invalid_ack: + SOCK_DEBUG(sk, "Ack %u after %u:%u\n", ack, tp->snd_una, tp->snd_nxt); + return -1; + old_ack: if (TCP_SKB_CB(skb)->sacked) { tcp_sacktag_write_queue(sk, skb, prior_snd_una); @@ -3693,9 +3700,8 @@ old_ack: tcp_try_keep_open(sk); } -uninteresting_ack: - SOCK_DEBUG(sk, "Ack %u out of %u:%u\n", ack, tp->snd_una, tp->snd_nxt); - return 0; + SOCK_DEBUG(sk, "Ack %u before %u:%u\n", ack, tp->snd_una, tp->snd_nxt); + return 0; } /* Look for tcp options. Normally only called on SYN and SYNACK packets. @@ -5158,7 +5164,8 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb, */ if ((tcp_flag_word(th) & TCP_HP_BITS) == tp->pred_flags && - TCP_SKB_CB(skb)->seq == tp->rcv_nxt) { + TCP_SKB_CB(skb)->seq == tp->rcv_nxt && + !after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) { int tcp_header_len = tp->tcp_header_len; /* Timestamp header prediction: tcp_header_len @@ -5311,8 +5318,8 @@ slow_path: return -res; step5: - if (th->ack) - tcp_ack(sk, skb, FLAG_SLOWPATH); + if (th->ack && tcp_ack(sk, skb, FLAG_SLOWPATH) < 0) + goto discard; tcp_rcv_rtt_measure_ts(sk, skb); @@ -5649,7 +5656,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb, /* step 5: check the ACK field */ if (th->ack) { - int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH); + int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH) > 0; switch (sk->sk_state) { case TCP_SYN_RECV: