suricata: correct rule actions in IPS mode

  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 <ipfr@tfitzgeorge.me.uk>
---
 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
  

Hi Tim,

Thanks for working on this. It is a long patch…

> On 1 Jun 2019, at 17:08, Tim FitzGeorge <ipfr@tfitzgeorge.me.uk> wrote:
> 
> 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 <ipfr@tfitzgeorge.me.uk>
> ---
> 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”);

Why is this necessary? Shouldn’t the downloader already be running as nobody or suricata?

> +
> 	# 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.

Great commenting :)

I am not so sure if it is a good idea to sort of hard-code the type of ruleset in the next bit.

What happens when we are adding more rulesets from other sources? How can we test this? We already have commercial rulesets listed right now which we cannot test at all. How do we know this works?

> +
> +		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 <<END;
> +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 <<END;
> +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 <info@ipfire.org>                #
> +#                                                                             #
> +# 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 <http://www.gnu.org/licenses/>.       #
> +#                                                                             #
> +###############################################################################
> +
> +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");
> +}

What is this script for? Do we only run this to update the systems?

Can we not just trigger a re-download of the rules?

> 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.

The typos could have been an extra patch to keep this one smaller (and I would have merged that right away).

> 			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";
> +				}

Would it not be easier to pass MONITOR_TRAFFIC_ONLY to the function instead of having two places where $ruleaction is being determined (the other one is further below)?

> +
> +				# 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 \
> -- 
> 2.16.4

I am awaiting Stefan’s response because he is the suricata expert.

Although the patch is large, I can imagine that this was a lot of work and took quite some time to figure out. Thanks for that!

I didn’t test it yet, but will deploy this into production as soon as Stefan gives his OK.

Best,
-Michael
  

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 <peter.mueller@ipfire.org>

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 <ipfr@tfitzgeorge.me.uk>
> ---
>  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 <<END;
> +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 <<END;
> +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 <info@ipfire.org>                #
> +#                                                                             #
> +# 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 <http://www.gnu.org/licenses/>.       #
> +#                                                                             #
> +###############################################################################
> +
> +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 \
>
  

Thanks for the feedback Peter.

Can we have Stefan reply to this and give us his thoughts?

I would like to close Core Update 133 by the end of the week and this patch must be in there. It is a very pressing bug.

-Michael

> On 4 Jun 2019, at 19:52, Peter Müller <peter.mueller@ipfire.org> wrote:
> 
> 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 <peter.mueller@ipfire.org>
> 
> 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 <ipfr@tfitzgeorge.me.uk>
>> ---
>> 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 <<END;
>> +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 <<END;
>> +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 <info@ipfire.org>                #
>> +#                                                                             #
>> +# 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 <http://www.gnu.org/licenses/>.       #
>> +#                                                                             #
>> +###############################################################################
>> +
>> +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