Fix False SYNFLOOD alerts #8740
Conversation
| (isTCPFlagSet(dst2src_tcp_flags,TH_FIN)))) { | ||
| updateSynFloodAlerts(false); | ||
| } | ||
|
|
There was a problem hiding this comment.
The code wirtten in this way it's not very clean and hard to understand in the future when debugging. It would have been cleaner simply running a switch case at the end of the calculateConnectionState() function taking account for all the possible connection states and based on the value simply increase or decrease the counter.
There was a problem hiding this comment.
That makes sense! However, I am a bit confused by the states we have defined as I feel they do not cover all possible connection states, such as when a connection is cancelled after a the server sends the SYNACK packet. Am I missing something or would it be ok to define another connection state?
| void Flow::updateSynFloodAlerts(bool connection_opened){ | ||
| if((current_c_state != S0 && !connection_opened) || | ||
| (current_c_state !=MINOR_NO_STATE && connection_opened)) | ||
| return; |
There was a problem hiding this comment.
If handled correctly in the calculateConnectionState() function, i don't think this check is needed
There was a problem hiding this comment.
These checks are mainly for two cases:
- Every data packet sent after the handshake trigger the checkS1ConnState(),
leading to unnecessarily decrementing the count. - If SYN packets are being resent from a client we will increment the count unnecessarily.
This logic could be in the calculateConnectionState() function but I thought it would be cleaner here.
|
|
||
| void AlertCounter::dec(){ | ||
| current_hits--; | ||
| } |
There was a problem hiding this comment.
Here you have to check that current_hits is higher than 0.
It can happen in many ways that the first packet of a captured flow does not starts as a normal connection (for instance the packet is forged and starts with a SYN and FIN set or simply the flows in captured half-way during the real connection) and this might lead to a counter overflow issue.
There was a problem hiding this comment.
The check in the updateSynFLoodAlerts() function should ensure that we only
decrement the count if the connection has previously been opened. So half captured flows and spoofed packets shouldn't be able to trigger this and cause an overflow I think.
Please sign (check) the below before submitting the Pull Request:
Link to the related issue:
Describe changes:
Keeping count of half opened TCP connections instead of SYN packets to keep track of SYN-FLOOD attacks.