Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix False SYNFLOOD alerts #8740

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
+91 −13
Open

Fix False SYNFLOOD alerts #8740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/AlertCounter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class AlertCounter {
public:
AlertCounter();
void inc(time_t when, AlertableEntity *alertable);
void inc_no_time_window();
void dec();
u_int16_t hits() const;
void reset_hits();
};
Expand Down
1 change: 1 addition & 0 deletions include/Flow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,7 @@ inline float get_goodput_bytes_thpt() const { return (goodput_bytes_thpt); };
inline MinorConnectionStates getCurrentConnectionState() { return(current_c_state); };
bool checkS1ConnState();
bool isTCPFlagSet(u_int8_t flags, int flag_to_check);
void updateSynFloodAlerts(bool connection_opened);
MinorConnectionStates calculateConnectionState(bool is_cumulative);
MajorConnectionStates getMajorConnState();
inline u_int32_t getPreNATSrcIp() { return ntohl(src_ip_addr_pre_nat); };
Expand Down
1 change: 1 addition & 0 deletions include/Host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ class Host : public GenericHashEntry,
bool addIfMatching(lua_State *vm, AddressTree *ptree, char *key);
bool addIfMatching(lua_State *vm, u_int8_t *mac);
void updateSynAlertsCounter(time_t when, bool syn_sent);
void updateSynFloodAlertsCounter(bool attacker, bool connection_opened);
void updateFinAlertsCounter(time_t when, bool fin_sent);
void updateRstAlertsCounter(time_t when, bool rst_sent);
void updateICMPAlertsCounter(time_t when, bool icmp_sent);
Expand Down
1 change: 1 addition & 0 deletions include/NetworkStats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ class NetworkStats : public InterfaceMemberAlertableEntity,
virtual void updateStats(const struct timeval *tv);

void updateSynAlertsCounter(time_t when, bool syn_sent);
void updateSynFloodAlertsCounter(bool connection_opened);
void updateSynAckAlertsCounter(time_t when, bool synack_sent);
void updateRoundTripTime(u_int32_t rtt_msecs);
void incNumFlows(time_t t, bool as_client);
Expand Down
4 changes: 2 additions & 2 deletions scripts/locales/en.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,8 +1083,8 @@ local lang = {
["subject_quota_exceeded"] = "Host pool <a href=\"%{url}\">%{pool}</a> exceeded %{subject} quota [%{value} &gt; %{quota}]",
["suspicious_dga_domain_http"] = "Possible risky DGA Domain name detected [url: %{href}]",
["suspicious_dga_domain_other"] = "Possible risky DGA Domain name detected [domain: %{href}]",
["syn_flood_attacker"] = "%{entity} is a SYN flooder [%{value} &gt; %{threshold} SYN/sec sent for ~3 sec]",
["syn_flood_victim"] = "%{entity} is under SYN flood attack [%{value} &gt; %{threshold} SYN/sec received for ~3 sec]",
["syn_flood_attacker"] = "%{entity} is a SYN flooder [%{value} &gt; %{threshold} half-opened TCP Connections]",
["syn_flood_victim"] = "%{entity} is under SYN flood attack [%{value} &gt; %{threshold} half-opened TCP Connections]",
["syn_scan_attacker"] = "%{entity} is a SYN scan attacker [%{value} &gt; %{threshold} SYN sent]",
["syn_scan_victim"] = "%{entity} is under a SYN scan [%{value} &gt; %{threshold} SYN received]",
["system_error"] = "System error detected in ntopng, please report it. Error message: %{system_error_msg}",
Expand Down
10 changes: 9 additions & 1 deletion src/AlertCounter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,15 @@ void AlertCounter::inc(time_t when, AlertableEntity *alertable) {
);
#endif
}
/* *************************************** */

void AlertCounter::inc_no_time_window(){
current_hits++;
}

void AlertCounter::dec(){
current_hits--;
}
Copy link
Member

@MatteoBiscosi MatteoBiscosi Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/* *************************************** */

u_int16_t AlertCounter::hits() const {
Expand All @@ -81,4 +89,4 @@ u_int16_t AlertCounter::hits() const {

/* *************************************** */

void AlertCounter::reset_hits() { hits_reset_req = true; }
void AlertCounter::reset_hits() { hits_reset_req = true; }
54 changes: 52 additions & 2 deletions src/Flow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,13 @@ Flow::~Flow() {
constructor (viewed interfaces) */
delete srv_ip_addr;

/* Clean up SYN Flood Counts if the we have an open conection and the flow is being purged*/

if( getCurrentConnectionState() == S0 ){
updateSynFloodAlerts(false);
}


/*
Finish deleting other flow data structures
*/
Expand Down Expand Up @@ -8733,6 +8740,27 @@ bool Flow::checkS1ConnState() {
!(isTCPFlagSet(src2dst_tcp_flags,TH_RST)) && !(isTCPFlagSet(dst2src_tcp_flags,TH_RST)) /* NO RST */
));
}
/* **************************************************** */

void Flow::updateSynFloodAlerts(bool connection_opened){
if((current_c_state != S0 && !connection_opened) ||
(current_c_state !=MINOR_NO_STATE && connection_opened))
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If handled correctly in the calculateConnectionState() function, i don't think this check is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks are mainly for two cases:

  1. Every data packet sent after the handshake trigger the checkS1ConnState(),
    leading to unnecessarily decrementing the count.
  2. 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.



NetworkStats *cli_network_stats = NULL, *srv_network_stats = NULL;
if (srv_host)
srv_network_stats =
srv_host->getNetworkStats(srv_host->get_local_network_id());

if (cli_host)
cli_host->updateSynFloodAlertsCounter(true,connection_opened);
if (srv_host)
srv_host->updateSynFloodAlertsCounter(false,connection_opened);

if (srv_network_stats)
srv_network_stats->updateSynFloodAlertsCounter(connection_opened);
}

/* **************************************************** */

Expand All @@ -8745,19 +8773,39 @@ MinorConnectionStates Flow::calculateConnectionState(bool is_cumulative) {
!(isTCPFlagSet(dst2src_tcp_flags,TH_SYN))) {
if (!(isTCPFlagSet(dst2src_tcp_flags,TH_RST))) {
if ((isTCPFlagSet(src2dst_tcp_flags,TH_RST))) {
updateSynFloodAlerts(false);
return(setCurrentConnectionState(RSTOS0));
} else {
if ((isTCPFlagSet(src2dst_tcp_flags,TH_FIN))) {
updateSynFloodAlerts(false);
return(setCurrentConnectionState(SH));
} else {
return(setCurrentConnectionState(S0));
/* We are assuming that the TCP connection is established on
* server side when it sees the clients SYN
* this helps us detect SYN flood victims even if all the SYN
* packets are rejected or dropped*/
updateSynFloodAlerts(true);
return(setCurrentConnectionState(S0));
}
}
} else {
updateSynFloodAlerts(false);
return(setCurrentConnectionState(REJ));
}
}

/* Account for connection being closed after SYNACK*/
if ((isTCPFlagSet(src2dst_tcp_flags,TH_SYN)) &&
(isTCPFlagSet(dst2src_tcp_flags,TH_SYN)) &&
(isTCPFlagSet(dst2src_tcp_flags,TH_ACK)) &&
!(isTCPFlagSet(src2dst_tcp_flags,TH_ACK))&&
((isTCPFlagSet(dst2src_tcp_flags,TH_RST))||
(isTCPFlagSet(src2dst_tcp_flags,TH_RST)) ||
(isTCPFlagSet(src2dst_tcp_flags,TH_FIN)) ||
(isTCPFlagSet(dst2src_tcp_flags,TH_FIN)))) {
updateSynFloodAlerts(false);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/* Check RSTRH */
if ((isTCPFlagSet(dst2src_tcp_flags,TH_SYN)) &&
(isTCPFlagSet(dst2src_tcp_flags,TH_ACK)) &&
Expand Down Expand Up @@ -8810,8 +8858,10 @@ MinorConnectionStates Flow::calculateConnectionState(bool is_cumulative) {
return(setCurrentConnectionState(RSTR));

/* Check S1 */
if (checkS1ConnState())
if (checkS1ConnState()){
updateSynFloodAlerts(false);
return(setCurrentConnectionState(S1));
}

if (getCurrentConnectionState() != MINOR_NO_STATE)
return(getCurrentConnectionState());
Expand Down
14 changes: 10 additions & 4 deletions src/Host.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,6 @@ u_int16_t Host::decScoreValue(u_int16_t score_decr,
/* *************************************** */

void Host::updateSynAlertsCounter(time_t when, bool syn_sent) {
AlertCounter *counter = syn_sent ? syn_flood.attacker_counter : syn_flood.victim_counter;

counter->inc(when, this);

if (syn_sent)
syn_scan.syn_sent_last_min++;
else
Expand All @@ -189,6 +185,16 @@ void Host::updateSynAlertsCounter(time_t when, bool syn_sent) {

/* *************************************** */

void Host::updateSynFloodAlertsCounter(bool attacker, bool connection_opened) {
AlertCounter *counter = attacker ? syn_flood.attacker_counter : syn_flood.victim_counter;
if(connection_opened)
counter->inc_no_time_window();
else
counter->dec();
}

/* *************************************** */

void Host::updateFinAlertsCounter(time_t when, bool fin_sent) {
fin_sent ? fin_scan.fin_sent_last_min++ : fin_scan.fin_recvd_last_min++;
}
Expand Down
13 changes: 11 additions & 2 deletions src/NetworkStats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ void NetworkStats::updateRoundTripTime(u_int32_t rtt_msecs) {
void NetworkStats::housekeepAlerts(ScriptPeriodicity p) {
switch (p) {
case minute_script:
flow_flood_victim_alert.reset_hits(), syn_flood_victim_alert.reset_hits();
flow_flood_victim_alert.reset_hits(); /*,syn_flood_victim_alert.reset_hits()*/
syn_recvd_last_min = synack_sent_last_min = 0;
break;
default:
Expand All @@ -208,10 +208,19 @@ void NetworkStats::housekeepAlerts(ScriptPeriodicity p) {

void NetworkStats::updateSynAlertsCounter(time_t when, bool syn_sent) {
if (!syn_sent) {
syn_flood_victim_alert.inc(when, this);
syn_recvd_last_min++;
}
}
/* *************************************** */

void NetworkStats::updateSynFloodAlertsCounter( bool connection_opened) {
if (connection_opened) {
syn_flood_victim_alert.inc_no_time_window();
}
else{
syn_flood_victim_alert.dec();
}
}

/* *************************************** */

Expand Down
2 changes: 1 addition & 1 deletion src/host_checks/SYNFlood.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ void SYNFlood::periodicUpdate(Host *h, HostAlert *engaged_alert) {
CLIENT_NO_RISK_PERCENTAGE);

/* Reset counters once done */
h->reset_syn_flood_hits();
//h->reset_syn_flood_hits();
}

/* ***************************************************** */
2 changes: 1 addition & 1 deletion tests/e2e
Open in desktop
Submodule e2e updated 4 files
+1 −1 rest/result/v2/get_alert_type_consts_01.out
+1 −1 rest/result/v2/get_checks_config_01.out
+1 −1 rest/result/v2/get_l7_application_consts_01.out
+1 −1 rest/result/v2/get_l7_category_consts_01.out
Loading