Re: [PATCH 4/4] [V2] zabbix_agentd: Add IPFire specific userparameters

[Robin Roevens <robin.roevens@disroot.org>]

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

Hi Michael


Michael Tremer schreef op ma 12-04-2021 om 11:36 [+0100]:
> Hello,
> 
> > 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 
> > +#                      return this as a JSON array suitable for easy
> > processing 
> > +#                      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> 
> > +# 
> > +# 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;
> > +
> > +# 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 =(
> > +        'DHCP Server' => 'dhcpd',
> > +        'Web Server' => 'httpd',
> > +        'CRON Server' => 'fcron',
> > +        'DNS Proxy Server' => 'unbound',
> > +        'Logging Server' => 'syslogd',
> > +        'Kernel Logging Server' => 'klogd',
> > +        'NTP Server' => 'ntpd',
> > +        'Secure Shell Server' => 'sshd',
> > +        'VPN' => 'charon',
> > +        'Web Proxy' => 'squid',
> > +        'Intrusion Detection System' => 'suricata',
> > +        'OpenVPN' => 'openvpn'
> > +);
> 
> 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...
> 
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.

> > +
> > +# Hash to overwrite the process name of a process if it differs from
> > the launch command.
> > +my %overwrite_exename_hash = (
> > +        "suricata" => "Suricata-Main"
> > +);
> > +
> > +my $first = 1;
> > +
> > +print "[";
> > +
> > +# Built-in services
> > +my $key = '';
> > +foreach $key (sort keys %servicenames){
> > +       print "," if not $first;
> > +       $first = 0;
> > +
> > +       print "{";
> > +       print "\"service\":\"$key\",";
> > +
> > +       my $shortname = $servicenames{$key};
> > +       print &servicestats($shortname);
> > +       
> > +       print "}";
> > +}
> > +
> > +# Generate list of installed addon pak's
> > +my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -
> > d"-" -f2`;
> > +foreach (@pak){
> > +       chomp($_);
> > +
> > +       # Check which of the paks are services
> > +       my @svc = `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") ) {
> 
> Another hardcoded list due to code duplication. Not your fault, but not
> pretty.
> 

Indeed, as is probably clear by now, I too would prefer a more clean
way of generating a list of services.

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?)

> > +                       print ",";
> > +                       print "{";
> > +
> > +                       print "\"service\":\"Addon: $_\",";
> > +                       print "\"servicename\":\"$_\",";
> > +
> > +                       my $onboot = isautorun($_);
> > +                       print "\"onboot\":$onboot,";
> > +
> > +                       print &addonservicestats($_);
> > +
> > +                       print "}";
> > +               }
> > +       }
> > +}      
> > +
> > +print "]";
> > +
> > +sub servicestats{
> > +       my $cmd = $_[0];
> > +       my $status = "\"servicename\":\"$cmd\",\"state\":\"0\"";
> > +       my $pid = '';
> > +       my $testcmd = '';
> > +        my $exename;
> > +        my $memory;
> > +
> > +
> > +       $cmd =~ /(^[a-z]+)/;
> > +       
> > +       # 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 = $overwrite_exename_hash{$1};
> > +        } else {
> > +                # Directly expect the launched command as
> > +                # process name.
> > +                $exename = $1;
> > +        }
> > +       
> > +       if (open(FILE, "/var/run/${cmd}.pid")){
> > +                $pid = <FILE>; chomp $pid;
> > +                close FILE;
> > +                if (open(FILE, "/proc/${pid}/status")){
> > +                        while (<FILE>){
> > +                                if (/^Name:\W+(.*)/) {
> > +                                        $testcmd = $1;
> > +                                }
> > +                        }
> > +                        close FILE;
> > +                }
> > +                if (open(FILE, "/proc/${pid}/status")) {
> > +                        while (<FILE>) {
> > +                                my ($key, $val) = split(":", $_, 2);
> > +                                if ($key eq 'VmRSS') {
> > +                                       $val =~ /\s*([0-9]*)\s+kB/;
> > +                                       # Convert kB to B
> > +                                        $memory = $1*1024;
> > +                                        last;
> > +                                }
> > +                        }
> > +                        close(FILE);
> > +                }
> > +                if ($testcmd =~ /$exename/){
> > +                       $status =
> > "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$memory
> > ";
> > +               }
> > +        }
> > +        return $status;
> > +}
> > +
> > +sub isautorun{
> > +        my $cmd = $_[0];
> > +        my $status = "0";
> > +        my $init = `find /etc/rc.d/rc3.d/S??${cmd} 2>/dev/null`;
> > +        chomp ($init);
> > +        if ($init ne ''){
> > +                $status = "1";
> > +        }
> > +        $init = `find /etc/rc.d/rc3.d/off/S??${cmd} 2>/dev/null`;
> > +        chomp ($init);
> > +        if ($init ne ''){
> > +                $status = "0";
> > +        }
> > +
> > +        return $status;
> > +}
> > +
> > +sub addonservicestats{
> > +        my $cmd = $_[0];
> > +        my $status = "0";
> > +        my $pid = '';
> > +        my $testcmd = '';
> > +        my $exename;
> > +        my @memory = (0);
> > +
> > +        $testcmd = `sudo /usr/local/bin/addonctrl $_ status
> > 2>/dev/null`;
> > +
> > +        if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\
> > running/){
> > +                $status = "\"state\":1";
> > +
> > +                $testcmd =~ s/.* //gi;
> > +                $testcmd =~ s/[a-z_]//gi;
> > +                $testcmd =~ s/\[[0-1]\;[0-9]+//gi;
> > +                $testcmd =~ s/[\(\)\.]//gi;
> > +                $testcmd =~ s/  //gi;
> > +                $testcmd =~ s/^[//gi;
> > +
> > +                my @pid = split(/\s/,$testcmd);
> > +                $status .=",\"pid\":\"$pid[0]\"";
> > +
> > +                my $memory = 0;
> > +
> > +                foreach (@pid){
> > +                        chomp($_);
> > +                        if (open(FILE, "/proc/$_/statm")){
> > +                                my $temp = <FILE>;
> > +                                @memory = split(/ /,$temp);
> > +                        }
> > +                        $memory+=$memory[0];
> > +                }
> > +               $memory*=1024;
> > +                $status .=",\"memory\":$memory";
> > +        }else{
> > +                $status = "\"state\":0";
> > +        }
> > +        return $status;
> > +} 
> > 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=(ALL) NOPASSWD: /opt/pakfire/pakfire status
> > +zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status,
> > /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping
> 
> 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.
> 
> 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: 
- 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)
- 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

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

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

/usr/local/bin/addonctrl is only called from within the
ipfire_services.pl script I provide, in the form of "addonctrl
<service> status". 
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. 
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.

Regards
Robin

> 
> > 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 
> > +UserParameter=ipfire.net.gateway.pingtime,sudo /usr/sbin/fping -c 3
> > gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
> > +UserParameter=ipfire.net.gateway.ping,sudo /usr/sbin/fping -q -r 3
> > gateway; [ ! $? ]; echo $?
> > +UserParameter=ipfire.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=ipfire.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    = $(DIR_SRC)/$(THISAPP)
> > TARGET     = $(DIR_INFO)/$(THISAPP)
> > PROG       = zabbix_agentd
> > PAK_VER    = 5
> > -DEPS       =
> > +DEPS       = "fping"
> > 
> > #####################################################################
> > ##########
> > # 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
> > 
> >         # 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
> > 
> > +# 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}
> > 
> > # 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
> > 
> > make_backup ${NAME}
> > remove_files
> > -- 
> > 2.30.2
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
> -Michael
> 
> 


-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.