[Oisf-devel] RFC: stream: handle extra different SYN/ACK

[Peter Manev]

This was a very nice catch and work Victor.
Thanks

On Fri, Apr 19, 2013 at 9:13 AM, Victor Julien <victor at inliniac.net> wrote:
> This has now been pushed into master.
>
> On 04/08/2013 06:27 PM, Victor Julien wrote:
>> Handling of multiple different SYN/ACKs
>>
>> When processing the TCP 3 way handshake (3whs), Suricata's TCP stream
>> engine will closely follow the setup of a TCP connection to make sure
>> the rest of the session can be tracked and reassembled properly.
>> Retransmissions of SYN/ACKs are silently accepted, unless they are
>> different somehow. If the SEQ or ACK values are different they are
>> considered wrong and events are set. The stream events rules will match
>> on this.
>>
>> I ran into some cases where not the initial SYN/ACK was used by the
>> client, but instead a later one. Suricata however, had accepted the
>> initial SYN/ACK. The result was that every packet from that point was
>> rejected by the stream engine. A 67 packet pcap resulting in 64 stream
>> events.
>>
>> If people have the stream events enabled _and_ pay attention to them, a
>> noisy session like this should certainly get their attention. However,
>> many people disable the stream events, or choose to ignore them, so a
>> better solution is necessary.
>>
>>
>> *Analysis*
>>
>> In this case the curious thing is that the extra SYN/ACK(s) have
>> different properties: the sequence number is different. As the SYN/ACKs
>> sequence number is used as "initial sequence number" (ISN) in the "to
>> client" direction, it's crucial to track it correctly. Failing to do so,
>> Suricata will loose track of the stream, causing reassembly to fail.
>> This could lead to missed alerts.
>>
>> Whats happening on the wire:
>>
>> TCP SSN 1:
>>> SYN: SEQ 10
>> < SYN/ACK 1: ACK 11, SEQ 100
>> < SYN/ACK 2: ACK 11, SEQ 1000
>>> ACK: SEQ 11, ACK 101
>>
>> TCP SSN 2:
>>> SYN: SEQ 10
>> < SYN/ACK 1: ACK 11, SEQ 100
>>> SYN/ACK 2: ACK 11, SEQ 1000
>> < ACK: SEQ 11, ACK 1001
>>
>> It's clear that in SSN 1 the client ACKs the first SYN/ACK while in SSN
>> 2 the 2nd SYN/ACK is ACK'd. It's likely that the first SYN/ACK was lost
>> before it reached the client. Suricata accepts the first though, and
>> rejects any others that are not the same.
>>
>>
>> *Solution*
>>
>> The solution I've been working on is to delay judgement on the extra
>> SYN/ACKs until Suricata sees the ACK that completes the 3whs. At that
>> point Suricata knows what the client accepted, and which SYN/ACKs were
>> either ignored, or never received.
>>
>> Logic in pseudo code:
>>
>> Normal SYN/ACK coming in:
>>
>>     UpdateState(p);
>>     ssn->state = TCP_SYN_RECV;
>>
>> Extra SYN/ACK packets:
>>
>>     if (p != ssn) {
>>         QueueState(p);
>>
>> On receiving the ACK that completes the 3whs:
>>
>>     if (ssn->queue_len) {
>>         q = QueueFindState(p);
>>         if (q)
>>             UpdateState(q);
>>     }
>>     UpdateState(p);
>>     ssn->state = TCP_ESTABLISHED;
>>
>> So when receiving the ACK, Suricata first searches for the proper
>> SYN/ACK on the list. If it's not found, the ACK will be processed
>> normally, which means it's checked against the original SYN/ACK. If
>> Suricata did have a queued state, it will first apply it to the SSN.
>> Then the ACK will be processed normally, so that is can complete the
>> 3whs and move the state to ESTABLISHED.
>>
>>
>> *Limitations*
>>
>> Queuing these states takes some memory, and for this reason there is a
>> limit to the number each SSN will accept. This is configurable through a
>> new stream option:
>>
>> stream:
>>   max-synack-queued: 5
>>
>> It defaults to 5. I've seen a few (valid) hits against a few terrabytes
>> of traffic, so I think the default is reasonably safe. An event is being
>> set if the limit is exceeded. It can be matched using a stream-event rule:
>>
>>   alert tcp any any -> any any (msg:"SURICATA STREAM 3way handshake
>> excessive different SYN/ACKs"; stream-event:3whs_synack_flood;
>> sid:2210055; rev:1;)
>>
>>
>> *Performance*
>>
>> This functionality doesn't affect the regular "fast path" except for a
>> small check to see if we have queued states. However, if the queue list
>> is being used Suricata enters a slow path. Currently this involves an
>> memory allocation per stored queue. It may be interesting to consider
>> using pools here, although a single global pool might be ineffecient. In
>> such a case a lock would have to be used and this might lead to
>> contention, especially in a case where Suricata would be flooded. Per
>> thread pools [https://redmine.openinfosecfoundation.org/issues/519,
>> https://redmine.openinfosecfoundation.org/issues/520,
>> https://redmine.openinfosecfoundation.org/issues/521] may be best here.
>>
>> *IPS mode*
>>
>> SYN/ACKs that exceed the limit are dropped if stream.inline is enabled
>> as is the case with all packets that are considered to be bad in some way.
>>
>>
>> *Code*
>>
>> Please review:
>> https://github.com/inliniac/suricata/commit/61315e92619e32565c7e439666acad8431972f9e
>>
>
>
> --
> ---------------------------------------------
> Victor Julien
> http://www.inliniac.net/
> PGP: http://www.inliniac.net/victorjulien.asc
> ---------------------------------------------
>
> _______________________________________________
> Suricata IDS Devel mailing list: oisf-devel at openinfosecfoundation.org
> Site: http://suricata-ids.org | Participate: http://suricata-ids.org/participate/
> List: https://lists.openinfosecfoundation.org/mailman/listinfo/oisf-devel
> Redmine: https://redmine.openinfosecfoundation.org/



-- 
Regards,
Peter Manev