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

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

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

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