Hello Tim, thank you for submitting this patch. I can confirm it solves side effects as described in #12086 (primarily tested with DNS and SMTP traffic, but there may be other issues I am not aware of). Tested-by: Peter Müller Thanks, and best regards, Peter Müller > In IPS mode rule actions need to be have the action 'drop' for the > protection to work, however this is not appropriate for all rules. > Modify the generator for oinkmaster-modify-sids.conf to leave > rules with the action 'alert' here this is appropriate. Also add > a script to be run on update to correct existing downloaded rules. > > Fixes #12086 > > Signed-off-by: Tim FitzGeorge > --- > config/cfgroot/ids-functions.pl | 44 +++++++++++++-- > config/rootfiles/common/configroot | 1 + > config/rootfiles/core/133/update.sh | 3 ++ > config/suricata/convert-ids-modifysids-file | 84 +++++++++++++++++++++++++++++ > html/cgi-bin/ids.cgi | 22 +++++++- > lfs/configroot | 1 + > 6 files changed, 148 insertions(+), 7 deletions(-) > create mode 100644 config/suricata/convert-ids-modifysids-file > > diff --git a/config/cfgroot/ids-functions.pl b/config/cfgroot/ids-functions.pl > index 88734a3ca..d7f659d69 100644 > --- a/config/cfgroot/ids-functions.pl > +++ b/config/cfgroot/ids-functions.pl > @@ -243,7 +243,7 @@ sub downloadruleset { > # Load perl module to deal with temporary files. > use File::Temp; > > - # Generate temporay file name, located in "/var/tmp" and with a suffix of ".tar.gz". > + # Generate temporary file name, located in "/var/tmp" and with a suffix of ".tar.gz". > my $tmp = File::Temp->new( SUFFIX => ".tar.gz", DIR => "/var/tmp/", UNLINK => 0 ); > my $tmpfile = $tmp->filename(); > > @@ -293,6 +293,9 @@ sub downloadruleset { > # Overwrite existing rules tarball with the new downloaded one. > move("$tmpfile", "$rulestarball"); > > + # Set correct ownership for the rulesdir and files. > + set_ownership("$rulestarball"); > + > # If we got here, everything worked fine. Return nothing. > return; > } > @@ -726,8 +729,8 @@ sub write_used_rulefiles_file(@) { > # > ## Function to generate and write the file for modify the ruleset. > # > -sub write_modify_sids_file($) { > - my ($ruleaction) = @_; > +sub write_modify_sids_file($$) { > + my ($ruleaction,$rulefile) = @_; > > # Open modify sid's file for writing. > open(FILE, ">$modify_sids_file") or die "Could not write to $modify_sids_file. $!\n"; > @@ -737,8 +740,39 @@ sub write_modify_sids_file($) { > > # Check if the traffic only should be monitored. > unless($ruleaction eq "alert") { > - # Tell oinkmaster to switch all rules from alert to drop. > - print FILE "modifysid \* \"alert\" \| \"drop\"\n"; > + # Suricata is in IPS mode, which means that the rule actions have to be changed > + # from 'alert' to 'drop', however not all rules should be changed. Some rules > + # exist purely to set a flowbit which is used to convey other information, such > + # as a specific type of file being downloaded, to other rulewhich then check for > + # malware in that file. Rules which fall into the first category should stay as > + # alert since not all flows of that type contain malware. > + > + if($rulefile eq 'registered' or $rulefile eq 'subscripted' or $rulefile eq 'community') { > + # These types of rulesfiles contain meta-data which gives the action that should > + # be used when in IPS mode. Do the following: > + # > + # 1. Disable all rules and set the action to 'drop' > + # 2. Set the action back to 'alert' if the rule contains 'flowbits:noalert;' > + # This should give rules not in the policy a reasonable default if the user > + # manually enables them. > + # 3. Enable rules and set actions according to the meta-data strings. > + > + my $policy = 'balanced'; # Placeholder to allow policy to be changed. > + > + print FILE < +modifysid * "^#?(?:alert|drop)" | "#drop" > +modifysid * "^#drop(.+flowbits:noalert;)" | "#alert\${1}" > +modifysid * "^#(?:alert|drop)(.+policy $policy-ips alert)" | "alert\${1}" > +modifysid * "^#(?:alert|drop)(.+policy $policy-ips drop)" | "drop\${1}" > +END > + } else { > + # These rulefiles don't have the metadata, so set rules to 'drop' unless they > + # contain the string 'flowbits:noalert;'. > + print FILE < +modifysid * "^(#?)(?:alert|drop)" | "\${1}drop" > +modifysid * "^(#?)drop(.+flowbits:noalert;)" | "\${1}alert\${2}" > +END > + } > } > > # Close file handle. > diff --git a/config/rootfiles/common/configroot b/config/rootfiles/common/configroot > index a7f27fe55..56b0257bc 100644 > --- a/config/rootfiles/common/configroot > +++ b/config/rootfiles/common/configroot > @@ -3,6 +3,7 @@ usr/sbin/convert-outgoingfw > usr/sbin/convert-portfw > usr/sbin/convert-snort > usr/sbin/convert-xtaccess > +usr/sbin/convert-ids-modifysids-file > usr/sbin/firewall-policy > #var/ipfire > var/ipfire/addon-lang > diff --git a/config/rootfiles/core/133/update.sh b/config/rootfiles/core/133/update.sh > index 3b769f42e..c2fe5db04 100644 > --- a/config/rootfiles/core/133/update.sh > +++ b/config/rootfiles/core/133/update.sh > @@ -59,6 +59,9 @@ telinit u > # Update Language cache > /usr/local/bin/update-lang-cache > > +# Modify suricata modify-sids file > +/usr/sbin/convert-ids-modifysids-file > + > # Start services > /usr/local/bin/ipsecctrl S > /etc/init.d/suricata restart > diff --git a/config/suricata/convert-ids-modifysids-file b/config/suricata/convert-ids-modifysids-file > new file mode 100644 > index 000000000..8b70aa0fc > --- /dev/null > +++ b/config/suricata/convert-ids-modifysids-file > @@ -0,0 +1,84 @@ > +#!/usr/bin/perl > +############################################################################### > +# # > +# IPFire.org - A linux based firewall # > +# Copyright (C) 2019 IPFire Development Team # > +# # > +# This program is free software: you can redistribute it and/or modify # > +# it under the terms of the GNU General Public License as published by # > +# the Free Software Foundation, either version 3 of the License, or # > +# (at your option) any later version. # > +# # > +# This program is distributed in the hope that it will be useful, # > +# but WITHOUT ANY WARRANTY; without even the implied warranty of # > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # > +# GNU General Public License for more details. # > +# # > +# You should have received a copy of the GNU General Public License # > +# along with this program. If not, see . # > +# # > +############################################################################### > + > +use strict; > + > +require '/var/ipfire/general-functions.pl'; > +require "${General::swroot}/ids-functions.pl"; > + > +# Hash which contains the IDS (suricata) settings. > +my %idssettings; > + > +# Hash which contains the RULES settings. > +my %rulessettings; > + > +# > +## Step 1: Read IDS and rules settings. > +# > + > +exit unless(-f $IDS::ids_settings_file and -f $IDS::rules_settings_file); > + > +# Read IDS settings. > +&General::readhash("$IDS::ids_settings_file", \%idssettings); > + > +# Read rules settings. > +&General::readhash("$IDS::rules_settings_file", \%rulessettings); > + > +# > +## Step 2: Generate and write the file to modify the ruleset. > +# > + > +my $IDS_action = "drop"; > + > +# Check if the traffic only should be monitored. > +if ($idssettings{"MONITOR_TRAFFIC_ONLY"} eq "on") { > + # Switch IDS action to alert only. > + $IDS_action = "alert"; > +} > + > +# Call subfunction and pass the desired IDS action. > +&IDS::write_modify_sids_file($IDS_action, $rulessettings{RULES}); > + > +# Set correct ownership. > +&IDS::set_ownership("$IDS::modify_sids_file"); > + > +# > +## Step 3: Call oinkmaster to extract and setup the rules structures. > +# > + > +# Check if a rulestarball is present. > +if (-f $IDS::rulestarball) { > + # Launch oinkmaster by calling the subfunction. > + &IDS::oinkmaster(); > + > + # Set correct ownership for the rulesdir and files. > + &IDS::set_ownership("$IDS::rulespath"); > +} > + > +# > +## Step 4: Start the IDS if enabled. > +# > + > +# Check if the IDS should be started. > +if($idssettings{"ENABLE_IDS"} eq "on") { > + # Call suricatactrl and reload the rules. > + &IDS::call_suricatactrl("reload"); > +} > diff --git a/html/cgi-bin/ids.cgi b/html/cgi-bin/ids.cgi > index 00db6a0c3..1791e9beb 100644 > --- a/html/cgi-bin/ids.cgi > +++ b/html/cgi-bin/ids.cgi > @@ -359,7 +359,7 @@ if ($cgiparams{'RULESET'} eq $Lang::tr{'save'}) { > $errormessage = "$Lang::tr{'could not download latest updates'} - $Lang::tr{'system is offline'}"; > } > > - # Check if enought free disk space is availabe. > + # Check if enough free disk space is availabe. > if(&IDS::checkdiskspace()) { > $errormessage = "$Lang::tr{'not enough disk space'}"; > } > @@ -370,6 +370,22 @@ if ($cgiparams{'RULESET'} eq $Lang::tr{'save'}) { > # a new ruleset. > &working_notice("$Lang::tr{'ids working'}"); > > + &General::readhash("$IDS::ids_settings_file", \%idssettings); > + > + # Temporary variable to set the ruleaction. > + # Default is "drop" to use suricata as IPS. > + my $ruleaction="drop"; > + > + # Check if the traffic only should be monitored. > + if($idssettings{'MONITOR_TRAFFIC_ONLY'} eq 'on') { > + # Switch the ruleaction to "alert". > + # Suricata acts as an IDS only. > + $ruleaction="alert"; > + } > + > + # Write the modify sid's file and pass the taken ruleaction. > + &IDS::write_modify_sids_file($ruleaction, $cgiparams{'RULES'}); > + > # Call subfunction to download the ruleset. > if(&IDS::downloadruleset()) { > $errormessage = $Lang::tr{'could not download latest updates'}; > @@ -609,8 +625,10 @@ if ($cgiparams{'RULESET'} eq $Lang::tr{'save'}) { > $ruleaction="alert"; > } > > + &General::readhash("$IDS::rules_settings_file", \%rulessettings); > + > # Write the modify sid's file and pass the taken ruleaction. > - &IDS::write_modify_sids_file($ruleaction); > + &IDS::write_modify_sids_file($ruleaction, $rulessettings{'RULES'}); > > # Check if "MONITOR_TRAFFIC_ONLY" has been changed. > if($cgiparams{'MONITOR_TRAFFIC_ONLY'} ne $oldidssettings{'MONITOR_TRAFFIC_ONLY'}) { > diff --git a/lfs/configroot b/lfs/configroot > index d4eb545f0..227d09239 100644 > --- a/lfs/configroot > +++ b/lfs/configroot > @@ -135,6 +135,7 @@ $(TARGET) : > > # Install snort to suricata converter. > cp $(DIR_SRC)/config/suricata/convert-snort /usr/sbin/convert-snort > + cp $(DIR_SRC)/config/suricata/convert-ids-modifysids-file /usr/sbin/convert-ids-modifysids-file > > # Add conntrack helper default settings > for proto in FTP H323 IRC SIP TFTP; do \ > -- The road to Hades is easy to travel. -- Bion of Borysthenes