Re: [PATCH] suricata: correct rule actions in IPS mode

["Peter Müller" <peter.mueller@ipfire.org>]

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

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(a)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(a)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(a)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