From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] suricata: correct rule actions in IPS mode
Date: Sun, 02 Jun 2019 23:09:02 +0100
Message-ID: <3738A1F6-A55C-4E77-8A5A-EB34769D067A@ipfire.org>
In-Reply-To: <20190601160817.16358-1-ipfr@tfitzgeorge.me.uk>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0219541452837441884=="
List-Id: <development.lists.ipfire.org>

--===============0219541452837441884==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi Tim,

Thanks for working on this. It is a long patch=E2=80=A6

> On 1 Jun 2019, at 17:08, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
>=20
> 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.
>=20
> Fixes #12086
>=20
> 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
>=20
> 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;
>=20
> -	# 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 o=
f ".tar.gz".
> 	my $tmp =3D File::Temp->new( SUFFIX =3D> ".tar.gz", DIR =3D> "/var/tmp/", =
UNLINK =3D> 0 );
> 	my $tmpfile =3D $tmp->filename();
>=20
> @@ -293,6 +293,9 @@ sub downloadruleset {
> 	# Overwrite existing rules tarball with the new downloaded one.
> 	move("$tmpfile", "$rulestarball");
>=20
> +	# Set correct ownership for the rulesdir and files.
> +	set_ownership("$rulestarball=E2=80=9D);

Why is this necessary? Shouldn=E2=80=99t 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) =3D @_;
> +sub write_modify_sids_file($$) {
> +	my ($ruleaction,$rulefile) =3D @_;
>=20
> 	# Open modify sid's file for writing.
> 	open(FILE, ">$modify_sids_file") or die "Could not write to $modify_sids_f=
ile. $!\n";
> @@ -737,8 +740,39 @@ sub write_modify_sids_file($) {
>=20
> 	# 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 informatio=
n, such
> +		# as a specific type of file being downloaded, to other rulewhich then c=
heck 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 rulese=
t 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 cann=
ot 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 th=
at 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:noal=
ert;'
> +			#    This should give rules not in the policy a reasonable default if t=
he user
> +			#    manually enables them.
> +			# 3. Enable rules and set actions according to the meta-data strings.
> +
> +			my $policy =3D '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' unles=
s they
> +			# contain the string 'flowbits:noalert;'.
> +			print FILE <<END;
> +modifysid * "^(#?)(?:alert|drop)" | "\${1}drop"
> +modifysid * "^(#?)drop(.+flowbits:noalert;)" | "\${1}alert\${2}"
> +END
> +		}
> 	}
>=20
> 	# Close file handle.
> diff --git a/config/rootfiles/common/configroot b/config/rootfiles/common/c=
onfigroot
> 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/13=
3/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
>=20
> +# 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 =3D "drop";
> +
> +# Check if the traffic only should be monitored.
> +if ($idssettings{"MONITOR_TRAFFIC_ONLY"} eq "on") {
> +	# Switch IDS action to alert only.
> +	$IDS_action =3D "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 =3D "$Lang::tr{'could not download latest updates'} - $La=
ng::tr{'system is offline'}";
> 			}
>=20
> -			# 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 woul=
d have merged that right away).

> 			if(&IDS::checkdiskspace()) {
> 				$errormessage =3D "$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'}");
>=20
> +				&General::readhash("$IDS::ids_settings_file", \%idssettings);
> +
> +				# Temporary variable to set the ruleaction.
> +				# Default is "drop" to use suricata as IPS.
> +				my $ruleaction=3D"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=3D"alert";
> +				}

Would it not be easier to pass MONITOR_TRAFFIC_ONLY to the function instead o=
f having two places where $ruleaction is being determined (the other one is f=
urther 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 =3D $Lang::tr{'could not download latest updates'};
> @@ -609,8 +625,10 @@ if ($cgiparams{'RULESET'} eq $Lang::tr{'save'}) {
> 		$ruleaction=3D"alert";
> 	}
>=20
> +	&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'});
>=20
> 	# 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) :
>=20
> 	# 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/con=
vert-ids-modifysids-file
>=20
> 	# Add conntrack helper default settings
> 	for proto in FTP H323 IRC SIP TFTP; do \
> --=20
> 2.16.4

I am awaiting Stefan=E2=80=99s response because he is the suricata expert.

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

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

Best,
-Michael


--===============0219541452837441884==--