search

Adamm00 / IPSet_ASUS

Skynet - Advanced IP Blocking For ASUS Routers Using IPSet.
https://www.snbforums.com/threads/release-skynet-router-firewall-security-enhancements.16798/
349 stars 61 forks source link

Skynet Stats not properly displaying Ban Reason. #129

Closed jumpsmm7 closed 1 year ago

jumpsmm7 commented 1 year ago
Brief Description Of Issue

So, I was playing with skynet stats today because I thought something was odd about the stats. I found it odd that in the Top 10 Blocks From (Outbound) it would list the reason "Imported: Apples" as the ban reason when the only lists I have imported using the comment "Apples" was whitelisted items.

Steps To Reproduce Issue

Import a whitelist: e.g. firewall import whitelist file.txt "Apples"

Make sure you only use the comment "Apples" for your whitelisted items.

Run skynet stats checking for a device which was previously known for having made it to the top 10 for being blocked trying access one of the newly whitelisted IP's which uses the comment "Apples" in whitelisted entries.

e.g. firewall stats search device 192.168.1.14

image

Expected Behaviour

Should not show Ban Reason as "Imported: Apples" since we know there are no Imported Blacklistings using the comment "Apples". This type of bad behavior could be giving bad statistics and false readings all around in skynet.

Output of ( sh /jffs/scripts/firewall debug info )

image

dave14305 commented 1 year ago

I think all occurrences of: cut -d '.' -f1-3)..*/ should be changed to: cut -d '.' -f1-3)\..*/ to make the dot literal.

jumpsmm7 commented 1 year ago

https://github.com/Adamm00/IPSet_ASUS/pull/130 I put in a pull request with the appropriate fixes. @dave14305 thanks for your help with this. I would not have had the patience to figure this out if you had not pointed me in the right place to investigate.

dave14305 commented 1 year ago

Why remove the -m1 from the grep? How are you ensuring only one match will be returned? And why such a complicated looking regex when Skynet-Whitelist alone would work just as well with the original -F syntax?

jumpsmm7 commented 1 year ago

Why remove the -m1 from the grep? How are you ensuring only one match will be returned? And why such a complicated looking regex when Skynet-Whitelist alone would work just as well with the original -F syntax?

The -m1 does nothing in this instance. Or are you trying to imply we should only remove one Skynet-Whitelist instance instead of all of them. The regex occurs no more penalty than simple -F in the eyes of using grep. It better expresses the intended logic.

dave14305 commented 1 year ago

The -m1 does nothing in this instance. Or are you trying to imply we should only remove one Skynet-Whitelist instance instead of all of them.

The grep is intended to say “print the first result that is not part of the whitelist”, since it uses -v. The new syntax as written now could return multiple lines if the first 3 octets matched more than one CIDR (i.e. a /25 to /31). A rare condition, but it should be considered a possibility since -m1 was already there.

jumpsmm7 commented 1 year ago

The -m1 does nothing in this instance. Or are you trying to imply we should only remove one Skynet-Whitelist instance instead of all of them.

The grep is intended to say “print the first result that is not part of the whitelist”, since it uses -v. The new syntax as written now could return multiple lines if the first 3 octets matched more than one CIDR (i.e. a /25 to /31). A rare condition, but it should be considered a possibility since -m1 was already there.

I am heeding your advice and submitting a request to put it back. I agree. I will close this once @Adamm00 approves the adjusted request.

https://github.com/Adamm00/IPSet_ASUS/pull/131

jumpsmm7 commented 1 year ago

Looks like @dave14305 has fixed some more code in this request: https://github.com/Adamm00/IPSet_ASUS/pull/132

The fixes look solid.