Re: [PATCH 0/5] ipblacklist: IP Address Blacklists

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 4947 bytes --]

Hello Tim,

Thank you for sending in this patchset.

I think as well that this functionality would be a great addition to the IPS that we have right now and we have talked about this on numerous occasions - including implementation details.

Now, you have done the whole thing. Well done.

Before we dive into the code, can I ask a couple of high-level questions?

Peter is always bringing up that downloading blacklists isn’t a good idea. It has actually become one of the biggest obstacles in our conversations and I am surprised he didn’t bring it up again :)

The automatic blacklist feature. What is the objective here? We saw value in having traffic even from malicious sources passed to the IPS so that it will examine it and log it. The idea was to have a better picture of the threats instead of just silencing them. Not sure what is best in the end.

I am unsure how users will deal with this and turn on “all the lists”(TM) and suddenly things do not work any more. How are they meant to figure out a good threshold? Should we not make that decision for them instead?

About the implementation: Your code is very very clean as always. There are a couple of things that I would like to see changed around how those iptables rules are being inserted into the existing chains. You are adding something into the POLICYIN chain with surgical precision which might break when the chain is being modified. This is potentially all minor stuff and can be fixed in minutes.

Well done!

-Michael

> On 25 Nov 2019, at 20:13, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
> 
> Implements downloading of IP address blacklists and implementing
> them as IPSets.  A separate IPSet is used for each blacklist; this
> simplifies handling of overlaps between different lists.  Traffic
> to or from the red0/ppp0 interface is checked against the IPSets.
> The check is placed before the IPS check as the IPSet check is
> much lighter on CPU use which means that overall CPU use is
> reduced.
> 
> The available lists are defined in a separate file.  A WUI page
> allows the desired lists to be enabled and the interval between
> checks for updates to be defined.  A minimum update check interval
> is defined for each blacklist in the definition file.
> 
> Optionally, an automatically updating blacklist can be enabled.
> This adds addresses to an IPSet if the rate of packets dropped by
> the default red0/ppp0 input policy exceeds a user defined threshold.
> The addresses are kept in the IPSet until a user defined period
> without packets from the blocked address has passed.
> 
> Tim FitzGeorge (5):
>  ipblacklist: Main script
>  ipblacklist: WUI and language file
>  ipblacklist: Ancillary files
>  ipblacklist: Modifications to system
>  ipblacklist: Build infrastructure
> 
> config/backup/backup.pl                     |    1 +
> config/backup/include                       |    2 +
> config/firewall/firewall-policy             |    5 +
> config/ipblacklist/sources                  |  151 +++
> config/logwatch/ipblacklist                 |  103 ++
> config/logwatch/ipblacklist.conf            |   34 +
> config/menu/50-firewall.menu                |    5 +
> config/rootfiles/common/aarch64/stage2      |    1 +
> config/rootfiles/common/configroot          |    2 +
> config/rootfiles/common/ipblacklist-sources |    1 +
> config/rootfiles/common/logwatch            |    2 +
> config/rootfiles/common/misc-progs          |    2 +
> config/rootfiles/common/stage2              |    1 +
> config/rootfiles/common/web-user-interface  |    1 +
> config/rootfiles/common/x86_64/stage2       |    1 +
> html/cgi-bin/ipblacklist.cgi                |  725 +++++++++++++
> html/cgi-bin/logs.cgi/log.dat               |    2 +
> langs/en/cgi-bin/en.pl                      |   31 +
> lfs/configroot                              |    4 +-
> lfs/ipblacklist-sources                     |   53 +
> lfs/logwatch                                |    2 +
> make.sh                                     |   11 +-
> src/initscripts/system/firewall             |   20 +
> src/misc-progs/Makefile                     |    2 +-
> src/misc-progs/getipsetstat.c               |   28 +
> src/misc-progs/ipblacklistctrl.c            |   52 +
> src/scripts/ipblacklist                     | 1558 +++++++++++++++++++++++++++
> 27 files changed, 2792 insertions(+), 8 deletions(-)
> create mode 100644 config/ipblacklist/sources
> create mode 100644 config/logwatch/ipblacklist
> create mode 100644 config/logwatch/ipblacklist.conf
> create mode 100644 config/rootfiles/common/ipblacklist-sources
> create mode 100644 html/cgi-bin/ipblacklist.cgi
> create mode 100644 lfs/ipblacklist-sources
> create mode 100644 src/misc-progs/getipsetstat.c
> create mode 100644 src/misc-progs/ipblacklistctrl.c
> create mode 100755 src/scripts/ipblacklist
> 
> -- 
> 2.16.4
>