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 4/4] [V2] zabbix_agentd: Add IPFire specific userparameters
Date: Thu, 15 Apr 2021 12:21:56 +0100
Message-ID: <3A671760-CEDE-44E6-8CAC-DFB89AB8CCD6@ipfire.org>
In-Reply-To: <76b0ca2a3d864a703e808811ebe982160924234b.camel@disroot.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0896220413011273478=="
List-Id: <development.lists.ipfire.org>

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

Hello,

> On 12 Apr 2021, at 23:16, Robin Roevens <robin.roevens(a)disroot.org> wrote:
>=20
> Hi Michael
>=20
>=20
> Michael Tremer schreef op ma 12-04-2021 om 11:36 [+0100]:
>> Hello,
>>=20
>>> On 7 Apr 2021, at 21:44, Robin Roevens <robin.roevens(a)disroot.org>
>>> wrote:
>>> ...
>>> ...
>>> diff --git a/config/zabbix_agentd/ipfire_services.pl
>>> b/config/zabbix_agentd/ipfire_services.pl
>>> new file mode 100755
>>> index 000000000..dbf8aec56
>>> --- /dev/null
>>> +++ b/config/zabbix_agentd/ipfire_services.pl
>>> @@ -0,0 +1,221 @@
>>> +#!/usr/bin/perl
>>> +####################################################################
>>> ###########
>>> +# ipfire_services.pl - Retrieves available IPFire services
>>> information and=20
>>> +#                      return this as a JSON array suitable for easy
>>> processing=20
>>> +#                      by Zabbix server
>>> +#
>>> +# Author: robin.roevens (at) disroot.org
>>> +# Version: 1.0
>>> +#
>>> +# Based on: services.cgi by IPFire Team
>>> +# Copyright (C) 2007-2021  IPFire Team  <info(a)ipfire.org>=20
>>> +#=20
>>> +# 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.
>>> +#=20
>>> +# 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=20
>>> +# GNU General Public License for more details.
>>> +#=20
>>> +# You should have received a copy of the GNU General Public License=20
>>> +# along with this program.  If not, see <
>>> http://www.gnu.org/licenses/>.
>>> +#=20
>>> +####################################################################
>>> ###########
>>> +
>>> +use strict;
>>> +
>>> +# enable only the following on debugging purpose
>>> +# use warnings;
>>> +
>>> +# Maps a nice printable name to the changing part of the pid file,
>>> which
>>> +# is also the name of the program
>>> +my %servicenames =3D(
>>> +        'DHCP Server' =3D> 'dhcpd',
>>> +        'Web Server' =3D> 'httpd',
>>> +        'CRON Server' =3D> 'fcron',
>>> +        'DNS Proxy Server' =3D> 'unbound',
>>> +        'Logging Server' =3D> 'syslogd',
>>> +        'Kernel Logging Server' =3D> 'klogd',
>>> +        'NTP Server' =3D> 'ntpd',
>>> +        'Secure Shell Server' =3D> 'sshd',
>>> +        'VPN' =3D> 'charon',
>>> +        'Web Proxy' =3D> 'squid',
>>> +        'Intrusion Detection System' =3D> 'suricata',
>>> +        'OpenVPN' =3D> 'openvpn'
>>> +);
>>=20
>> You have a hard-coded list with process names here. It technically is
>> missing add-ons but including everything would make this list very long
>> and a lot of work to maintain...
>>=20
> As explained in my previous mail, I don't currently know of a method to
> get a list of mandatory IPFire specific services. So this list is a
> straight copy out of services.cgi minus the translation.
> The add-on services are looked up below, exactly like it is done in
> services.cgi.

What list are you looking for? Just all processes that are running?

>>> +
>>> +# Hash to overwrite the process name of a process if it differs from
>>> the launch command.
>>> +my %overwrite_exename_hash =3D (
>>> +        "suricata" =3D> "Suricata-Main"
>>> +);
>>> +
>>> +my $first =3D 1;
>>> +
>>> +print "[";
>>> +
>>> +# Built-in services
>>> +my $key =3D '';
>>> +foreach $key (sort keys %servicenames){
>>> +       print "," if not $first;
>>> +       $first =3D 0;
>>> +
>>> +       print "{";
>>> +       print "\"service\":\"$key\",";
>>> +
>>> +       my $shortname =3D $servicenames{$key};
>>> +       print &servicestats($shortname);
>>> +      =20
>>> +       print "}";
>>> +}
>>> +
>>> +# Generate list of installed addon pak's
>>> +my @pak =3D `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -
>>> d"-" -f2`;
>>> +foreach (@pak){
>>> +       chomp($_);
>>> +
>>> +       # Check which of the paks are services
>>> +       my @svc =3D `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>> +       foreach (@svc){
>>> +               # blacklist some packages
>>> +               #
>>> +               # alsa has trouble with the volume saving and was not
>>> really stopped
>>> +               # mdadm should not stopped with webif because this
>>> could crash the system
>>> +               #
>>> +               chomp($_);
>>> +               if ( $_ eq 'squid' ) {
>>> +                       next;
>>> +               }
>>> +               if ( ($_ ne "alsa") && ($_ ne "mdadm") ) {
>>=20
>> Another hardcoded list due to code duplication. Not your fault, but not
>> pretty.
>>=20
>=20
> Indeed, as is probably clear by now, I too would prefer a more clean
> way of generating a list of services.
>=20
> Currently seeing this piece of code again.. I think this blacklist in
> services.cgi only exists so the user can't stop/start the service using
> the webgui.. But there is no such functionality in this script.. so I
> think I can skip this blacklist, and also provide service state of alsa
> and mdadm.. (not sure why squid is here, is there an addon that also
> provides squid, maybe squid-accounting?)

The code seemed to have some trouble with hyphenated package names, that is w=
hy those checks were introduced.

It is pretty bad code and we should rework it some time, but definitely not c=
opy it if we can avoid it.

>>> +                       print ",";
>>> +                       print "{";
>>> +
>>> +                       print "\"service\":\"Addon: $_\",";
>>> +                       print "\"servicename\":\"$_\",";
>>> +
>>> +                       my $onboot =3D isautorun($_);
>>> +                       print "\"onboot\":$onboot,";
>>> +
>>> +                       print &addonservicestats($_);
>>> +
>>> +                       print "}";
>>> +               }
>>> +       }
>>> +}     =20
>>> +
>>> +print "]";
>>> +
>>> +sub servicestats{
>>> +       my $cmd =3D $_[0];
>>> +       my $status =3D "\"servicename\":\"$cmd\",\"state\":\"0\"";
>>> +       my $pid =3D '';
>>> +       my $testcmd =3D '';
>>> +        my $exename;
>>> +        my $memory;
>>> +
>>> +
>>> +       $cmd =3D~ /(^[a-z]+)/;
>>> +      =20
>>> +       # Check if the exename needs to be overwritten.
>>> +        # This happens if the expected process name string
>>> +        # differs from the real one. This may happened if
>>> +        # a service uses multiple processes or threads.
>>> +        if (exists($overwrite_exename_hash{$cmd})) {
>>> +                # Grab the string which will be reported by
>>> +                # the process from the corresponding hash.
>>> +                $exename =3D $overwrite_exename_hash{$1};
>>> +        } else {
>>> +                # Directly expect the launched command as
>>> +                # process name.
>>> +                $exename =3D $1;
>>> +        }
>>> +      =20
>>> +       if (open(FILE, "/var/run/${cmd}.pid")){
>>> +                $pid =3D <FILE>; chomp $pid;
>>> +                close FILE;
>>> +                if (open(FILE, "/proc/${pid}/status")){
>>> +                        while (<FILE>){
>>> +                                if (/^Name:\W+(.*)/) {
>>> +                                        $testcmd =3D $1;
>>> +                                }
>>> +                        }
>>> +                        close FILE;
>>> +                }
>>> +                if (open(FILE, "/proc/${pid}/status")) {
>>> +                        while (<FILE>) {
>>> +                                my ($key, $val) =3D split(":", $_, 2);
>>> +                                if ($key eq 'VmRSS') {
>>> +                                       $val =3D~ /\s*([0-9]*)\s+kB/;
>>> +                                       # Convert kB to B
>>> +                                        $memory =3D $1*1024;
>>> +                                        last;
>>> +                                }
>>> +                        }
>>> +                        close(FILE);
>>> +                }
>>> +                if ($testcmd =3D~ /$exename/){
>>> +                       $status =3D
>>> "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$memory
>>> ";
>>> +               }
>>> +        }
>>> +        return $status;
>>> +}
>>> +
>>> +sub isautorun{
>>> +        my $cmd =3D $_[0];
>>> +        my $status =3D "0";
>>> +        my $init =3D `find /etc/rc.d/rc3.d/S??${cmd} 2>/dev/null`;
>>> +        chomp ($init);
>>> +        if ($init ne ''){
>>> +                $status =3D "1";
>>> +        }
>>> +        $init =3D `find /etc/rc.d/rc3.d/off/S??${cmd} 2>/dev/null`;
>>> +        chomp ($init);
>>> +        if ($init ne ''){
>>> +                $status =3D "0";
>>> +        }
>>> +
>>> +        return $status;
>>> +}
>>> +
>>> +sub addonservicestats{
>>> +        my $cmd =3D $_[0];
>>> +        my $status =3D "0";
>>> +        my $pid =3D '';
>>> +        my $testcmd =3D '';
>>> +        my $exename;
>>> +        my @memory =3D (0);
>>> +
>>> +        $testcmd =3D `sudo /usr/local/bin/addonctrl $_ status
>>> 2>/dev/null`;
>>> +
>>> +        if ( $testcmd =3D~ /is\ running/ && $testcmd !~ /is\ not\
>>> running/){
>>> +                $status =3D "\"state\":1";
>>> +
>>> +                $testcmd =3D~ s/.* //gi;
>>> +                $testcmd =3D~ s/[a-z_]//gi;
>>> +                $testcmd =3D~ s/\[[0-1]\;[0-9]+//gi;
>>> +                $testcmd =3D~ s/[\(\)\.]//gi;
>>> +                $testcmd =3D~ s/  //gi;
>>> +                $testcmd =3D~ s/=1B//gi;
>>> +
>>> +                my @pid =3D split(/\s/,$testcmd);
>>> +                $status .=3D",\"pid\":\"$pid[0]\"";
>>> +
>>> +                my $memory =3D 0;
>>> +
>>> +                foreach (@pid){
>>> +                        chomp($_);
>>> +                        if (open(FILE, "/proc/$_/statm")){
>>> +                                my $temp =3D <FILE>;
>>> +                                @memory =3D split(/ /,$temp);
>>> +                        }
>>> +                        $memory+=3D$memory[0];
>>> +                }
>>> +               $memory*=3D1024;
>>> +                $status .=3D",\"memory\":$memory";
>>> +        }else{
>>> +                $status =3D "\"state\":0";
>>> +        }
>>> +        return $status;
>>> +}=20
>>> diff --git a/config/zabbix_agentd/sudoers
>>> b/config/zabbix_agentd/sudoers
>>> index 1b362a4fd..340bb8e66 100644
>>> --- a/config/zabbix_agentd/sudoers
>>> +++ b/config/zabbix_agentd/sudoers
>>> @@ -14,4 +14,4 @@
>>> # Append / edit the following list of commands to fit your needs:
>>> #
>>> Defaults:zabbix !requiretty
>>> -zabbix ALL=3D(ALL) NOPASSWD: /opt/pakfire/pakfire status
>>> +zabbix ALL=3D(ALL) NOPASSWD: /opt/pakfire/pakfire status,
>>> /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping
>>=20
>> This is absolutely impossible to give the zabbix user control to
>> start/stop any add-on and to read and change the entire iptables
>> ruleset.
>>=20
>> Zabbix is listening on the network and does not have any kind of
>> authentication. Any security vulnerabilities in Zabbix would allow to
>> alter the firewall rules and DoS any add-ons. This is dangerous.
> There is some level of security:=20

I agree that it is *some*, but is it enough?

> - the agent will only respond on requests from the configured Zabbix
> server (which the user has to configure in the zabbix agentd conf-file,
> and defaults to 127.0.0.1 as we can't possibly know the ip of the
> user's server, so by default the agent responds to no one)

Host-based ACLs are not really a thing. On the local network faking a source =
IP address is one of the easiest jobs.

> - By default remote commands (as in requests to the agent to execute
> any command given) are disabled so the only way to make the agent call
> those binaries is to request a predefined userparameter that in turn
> will execute such a command with predefined params (see the additional
> template_...conf-files)
> and on top of that, the user can add additional security by configuring
> encryption in the agent main configfile

Encryption is not authentication.

And the predefined parameters might be a thing, but my view is that zabbix ca=
n execute quite serious commands if it is being exploited.

The Zabbix Agent had exactly one of these vulnerabilities in 2009 where an at=
tacker could execute arbitrary commands:

  https://www.cvedetails.com/cve/CVE-2009-4502/

> But as you point out, a possible vulnerability of some sort may indeed
> still pose risks this way.

I just find this risk insanely large. I am not going to tell people what soft=
ware they can use and what not, but I want to avoid that the default configur=
ation is adding major security issues to the entire system. And adding those =
sudo rules by default does exactly this.

What would a monitoring tool do with a dump of the iptables ruleset? I do not=
 even understand where the use of this is.

>> Are there any alternative ways to realise this functionality?
> As sudo is not able to allow commands with variable parameters together
> with fixed parameters, a popular solution is using wrapper scripts, not
> writable by the user, denying the user direct access to the commands in
> question.
>=20
> /usr/local/bin/addonctrl is only called from within the
> ipfire_services.pl script I provide, in the form of "addonctrl
> <service> status".=20
> So I could add /etc/zabbix_agentd/scripts/ipfire_services.pl to the
> sudo list instead so that the zabbix user won't be able to call
> addonctrl directly. (making sure zabbix user has no write permission on
> that script)

> The same goes for iptables. This is only called by the predefined
> userparameter "ipfire.net.fw.hits[*]" defined in
> template_module_ipfire_network_stats.conf in the form "/sbin/iptables -
> vnxL $1 | grep "\/\* $2 \*\/" | awk '{ print $$2 }';" where $1 (chain)
> and $2 (rule) are parameters that have to be supplied to the agent when
> requesting that item.=20
> Here I can also supply a wrapper-script to be called  by sudo
> /etc/zabbix_agentd/scripts/firewall_hits.sh that accepts those
> parameters, and checks them for validity and against abuse.
>=20
> Regards
> Robin

-Michael

>>=20
>>> diff --git
>>> a/config/zabbix_agentd/template_module_ipfire_network_stats.conf
>>> b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
>>> new file mode 100644
>>> index 000000000..f1658ed07
>>> --- /dev/null
>>> +++ b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
>>> @@ -0,0 +1,4 @@
>>> +### Parameters for monitoring IPFire network statistics=20
>>> +UserParameter=3Dipfire.net.gateway.pingtime,sudo /usr/sbin/fping -c 3
>>> gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
>>> +UserParameter=3Dipfire.net.gateway.ping,sudo /usr/sbin/fping -q -r 3
>>> gateway; [ ! $? ]; echo $?
>>> +UserParameter=3Dipfire.net.fw.hits[*],sudo /sbin/iptables -vnxL $1 |
>>> grep "\/\* $2 \*\/" | awk '{ print $$2 }';
>>> diff --git
>>> a/config/zabbix_agentd/template_module_ipfire_services.conf
>>> b/config/zabbix_agentd/template_module_ipfire_services.conf
>>> new file mode 100644
>>> index 000000000..5f95218e3
>>> --- /dev/null
>>> +++ b/config/zabbix_agentd/template_module_ipfire_services.conf
>>> @@ -0,0 +1,2 @@
>>> +### Parameter for monitoring IPFire services
>>> +UserParameter=3Dipfire.services,/etc/zabbix_agentd/scripts/ipfire_serv
>>> ices.pl
>>> diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
>>> index 73e08d20a..c0d28d51f 100644
>>> --- a/lfs/zabbix_agentd
>>> +++ b/lfs/zabbix_agentd
>>> @@ -33,7 +33,7 @@ DIR_APP    =3D $(DIR_SRC)/$(THISAPP)
>>> TARGET     =3D $(DIR_INFO)/$(THISAPP)
>>> PROG       =3D zabbix_agentd
>>> PAK_VER    =3D 5
>>> -DEPS       =3D
>>> +DEPS       =3D "fping"
>>>=20
>>> #####################################################################
>>> ##########
>>> # Top-level Rules
>>> @@ -97,6 +97,12 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>>>                 /etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
>>>         install -v -m 644
>>> $(DIR_SRC)/config/zabbix_agentd/template_app_pakfire.conf \
>>>                 /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfi
>>> re.conf.ipfirenew
>>> +       install -v -m 644
>>> $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_network_stats.
>>> conf \
>>> +               /etc/zabbix_agentd/zabbix_agentd.d/template_module_ip
>>> fire_network_stats.conf.ipfirenew
>>> +       install -v -m 644
>>> $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_services.conf
>>> \
>>> +               /etc/zabbix_agentd/zabbix_agentd.d/template_module_ip
>>> fire_services.conf.ipfirenew
>>> +       install -v -m 755
>>> $(DIR_SRC)/config/zabbix_agentd/ipfire_services.pl \
>>> +               /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfiren
>>> ew
>>>=20
>>>         # Create directory for additional agent modules
>>>         -mkdir -pv /usr/lib/zabbix
>>> diff --git a/src/paks/zabbix_agentd/install.sh
>>> b/src/paks/zabbix_agentd/install.sh
>>> index 4248a7ec1..ced915c81 100644
>>> --- a/src/paks/zabbix_agentd/install.sh
>>> +++ b/src/paks/zabbix_agentd/install.sh
>>> @@ -66,8 +66,13 @@ restore_backup ${NAME}
>>> # Put zabbix configfiles in place
>>> setup_configfile /etc/zabbix_agentd/zabbix_agentd.conf
>>> setup_configfile
>>> /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf
>>> +setup_configfile
>>> /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_network_sta
>>> ts.conf
>>> +setup_configfile
>>> /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_services.co
>>> nf
>>> setup_configfile /etc/sudoers.d/zabbix
>>>=20
>>> +# Overwrite script if it exists as user should not modify it but it
>>> is included in backup
>>> +mv /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
>>> /etc/zabbix_agentd/scripts/ipfire_services.pl
>>> +
>>> if $review_required; then
>>>         echo "WARNING: New versions of some configfile(s) where
>>> provided as .ipfirenew-files."
>>>         echo "         They may need manual review in order to take
>>> advantage of new features"
>>> diff --git a/src/paks/zabbix_agentd/uninstall.sh
>>> b/src/paks/zabbix_agentd/uninstall.sh
>>> index 7a13880c5..ccbc8f7cf 100644
>>> --- a/src/paks/zabbix_agentd/uninstall.sh
>>> +++ b/src/paks/zabbix_agentd/uninstall.sh
>>> @@ -26,6 +26,8 @@ stop_service ${NAME}
>>>=20
>>> # Remove .ipfirenew files in advance so they won't be included in
>>> backup
>>> rm -rfv /etc/zabbix_agentd/*.ipfirenew
>>> /etc/zabbix_agentd/*/*.ipfirenew
>>> +# Remove script-file as it should not have been modified by user
>>> +rm -fv /etc/zabbix_agentd/scripts/ipfire_services.pl
>>>=20
>>> make_backup ${NAME}
>>> remove_files
>>> --=20
>>> 2.30.2
>>>=20
>>>=20
>>> --=20
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>>=20
>>=20
>> -Michael
>>=20
>>=20
>=20
>=20
> --=20
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.


--===============0896220413011273478==--