BruteForce Detector changes #44
Conversation
This reverts commit 2fc9ccea
…work # Conflicts: # brute_force_detector/brute_force_detector.cpp # brute_force_detector/host.cpp # brute_force_detector/host.h # brute_force_detector/record.h # brute_force_detector/sender.h
|
Changes to evaluating attacks ( SSHHost/RDPHost/TELNETHost::checkForAttack ) should solve the issue with "OR evaluation" in incoming/outgoing direction. (mentioned by @vaclavbartos ) |
|
I briefly looked at the changes and noticed there are TABs used for indentation somewhere - please, replace them with 4 spaces to keep it consistent with the rest of the code and our codig style. |
cejkato2
left a comment
cejkato2
left a comment
There was a problem hiding this comment.
I'm glad for revision of this code.
However, we still need to solve the main task: to identify (correctly) who is attacking whom and provide such information in an alert...
The algorithm of SSH bf detection is not very complicated but it's principle of deciding about bruteforce is still quite unclear to me. I agree that one attacker can perform bf attack against multiple victims but I hesitate we can decide about attackers based on all their connections... From my personal point of view (based on rather intuition), we should try to recognize bf attacks per each pairs of IPs separately: one attacker can communicate legitimately and attack one victim at the same time...
If we improved the identification of real attacks, alerts would be much more clear to operators - an alert would identify an attacker and one or more victims.
| last report separated by a dash | ||
|
|
||
| * `NOTE` : This field contains (comma is used as separator): | ||
| "I: (list of incoming attacker IPs), O: (list of outgoing attacked IPs)" |
There was a problem hiding this comment.
It is great that this description was updated.
However, it still seems strange... even though the description is more accurate now, who is an attacker and who is under attack? We expected that SRC_IP is always an attacker. When there is something on the "list of incoming attacker IPs" , is SRC_IP a victim for these IPs?
I think the output of the module should describe who is an attacker and victim more clearly...
| return 3; | ||
| while ((opt = TRAP_GETOPT(argc, argv, module_getopt_string, long_options)) != -1) { | ||
| switch (opt) { | ||
| case 'c': // config |
There was a problem hiding this comment.
This change is not compliant with coding style: https://nemea.liberouter.org/developer/coding-style-c#statements
| ur_time_t timeOfLastReport; | ||
| ur_time_t timeOfLastReceivedRecord; | ||
| RecordList<T> recordListIncoming; //incoming direction to victim (attacker -> victim) | ||
| RecordList<T> recordListIncoming; // direction to victim (attacker -> victim) |
There was a problem hiding this comment.
It is a challenge for me to understand, why "incoming" represents traffic that is generated by attacker :-)
|
I agree the naming of directions as |
This reverts commit 38c115f
changing void pointers to MatchStructure objects
|
What is the current state of this PR? Is there anything we want to add or fix? Based on commit messages, it seems to me that changes were just about refactoring and minor improvements without affecting functionality, right? We'd need to improve this detector to identify both source and target of an attack, which is probably not covered in this PR... |
|
@petrmiculek This PR is quite old... Can you please resolve conflicts so we can merge it (if it is finished)? |
3 participants
Default config changes:
At least 90/100 suspicious flows needed to trigger an attack. (up from 30/50)
(90% of flows must be suspicious when there is more than that, this was not changed)
Host record list now holds only 500 (down from 1000) records,
but a record now has a timeout of 60 minutes (up from 30min).
Refactoring changes
Bugfixes