From c09e36927f74f955a325381a7d611ea270d04b65 Mon Sep 17 00:00:00 2001

From: Jake Hunsaker <jhunsake@redhat.com>

Date: Tue, 13 Nov 2018 12:30:45 -0500

Subject: [PATCH] [global] Quote options locally everywhere

An earlier commit passed cluster option values through pipes.quote() in

an effort to prevent command injection on the remote nodes, however this

did not capture every avenue through which a maliciously designed option

value or sos command could take.

Now, quote the option values at the time of use, and also quote any sos

option that isn't an on/off toggle.

Additionally, re-work how the ovirt/rhv database query is quoted and

filter out cluster/datacenter values that are likely to contain SQL

code, since we cannot rely on the driver to do this since we don't have

an actual connection to the database.

Original discovery of issues and patch work thanks to Riccardo Schirone.

Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>

---

 soscollector/clusters/kubernetes.py |  3 ++-

 soscollector/clusters/ovirt.py      | 32 ++++++++++++++++++++++-------

 soscollector/configuration.py       |  2 +-

 soscollector/sos_collector.py       | 19 +++++++++--------

 soscollector/sosnode.py             | 13 ++++++------

 5 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/soscollector/clusters/kubernetes.py b/soscollector/clusters/kubernetes.py

index 6c519f3..c5c2094 100644

--- a/soscollector/clusters/kubernetes.py

+++ b/soscollector/clusters/kubernetes.py

@@ -13,6 +13,7 @@

 # with this program; if not, write to the Free Software Foundation, Inc.,

 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

+from pipes import quote

 from soscollector.clusters import Cluster

@@ -32,7 +33,7 @@ class kubernetes(Cluster):

     def get_nodes(self):

         self.cmd += ' get nodes'

         if self.get_option('label'):

-            self.cmd += ' -l %s ' % self.get_option('label')

+            self.cmd += ' -l %s ' % quote(self.get_option('label'))

         res = self.exec_master_cmd(self.cmd)

         if res['status'] == 0:

             nodes = []

diff --git a/soscollector/clusters/ovirt.py b/soscollector/clusters/ovirt.py

index 1c44c97..0a074ca 100644

--- a/soscollector/clusters/ovirt.py

+++ b/soscollector/clusters/ovirt.py

@@ -16,6 +16,7 @@

 import os

 import fnmatch

+from pipes import quote

 from soscollector.clusters import Cluster

 from getpass import getpass

@@ -31,6 +32,22 @@ class ovirt(Cluster):

         ('no-hypervisors', False, 'Do not collect from hypervisors')

     ]

+    def _sql_scrub(self, val):

+        '''

+        Manually sanitize SQL queries since we can't leave this up to the

+        driver since we do not have an actual DB connection

+        '''

+        if not val:

+            return '%'

+

+        invalid_chars = ['\x00', '\\', '\n', '\r', '\032', '"', '\'']

+        if any(x in invalid_chars for x in val):

+            self.log_warn("WARNING: Cluster option \'%s\' contains invalid "

+                          "characters. Using '%%' instead." % val)

+            return '%'

+

+        return val

+

     def setup(self):

         self.pg_pass = False

         if not self.get_option('no-database'):

@@ -38,13 +55,14 @@ class ovirt(Cluster):

         self.format_db_cmd()

     def format_db_cmd(self):

-        cluster = self.get_option('cluster') or '%'

-        datacenter = self.get_option('datacenter') or '%'

-        self.dbcmd = '/usr/share/ovirt-engine/dbscripts/engine-psql.sh -c \"'

-        self.dbcmd += ("select host_name from vds_static where cluster_id in "

-                       "(select cluster_id from cluster where name like \'%s\'"

-                       " and storage_pool_id in (select id from storage_pool "

-                       "where name like \'%s\'))\"" % (cluster, datacenter))

+        cluster = self._sql_scrub(self.get_option('cluster'))

+        datacenter = self._sql_scrub(self.get_option('datacenter'))

+        query = ("select host_name from vds_static where cluster_id in "

+                 "(select cluster_id from cluster where name like '%s'"

+                 " and storage_pool_id in (select id from storage_pool "

+                 "where name like '%s'))" % (cluster, datacenter))

+        self.dbcmd = ('/usr/share/ovirt-engine/dbscripts/engine-psql.sh '

+                      '-c {}'.format(quote(query)))

         self.log_debug('Query command for ovirt DB set to: %s' % self.dbcmd)

     def get_nodes(self):

diff --git a/soscollector/configuration.py b/soscollector/configuration.py

index ea33835..7f2c3c7 100644

--- a/soscollector/configuration.py

+++ b/soscollector/configuration.py

@@ -138,7 +138,7 @@ class Configuration(dict):

                 try:

                     # there are no instances currently where any cluster option

                     # should contain a legitimate space.

-                    value = pipes.quote(option.split('=')[1].split()[0])

+                    value = option.split('=')[1].split()[0]

                 except IndexError:

                     # conversion to boolean is handled during validation

                     value = 'True'

diff --git a/soscollector/sos_collector.py b/soscollector/sos_collector.py

index 70a3dc8..7331f63 100644

--- a/soscollector/sos_collector.py

+++ b/soscollector/sos_collector.py

@@ -32,6 +32,7 @@ from concurrent.futures import ThreadPoolExecutor

 from .sosnode import SosNode

 from distutils.sysconfig import get_python_lib

 from getpass import getpass

+from pipes import quote

 from six.moves import input

 from textwrap import fill

 from soscollector import __version__

@@ -360,32 +361,34 @@ this utility or remote systems that it connects to.

     def configure_sos_cmd(self):

         '''Configures the sosreport command that is run on the nodes'''

         if self.config['sos_opt_line']:

-            filt = ['&', '|', '>', '<']

+            filt = ['&', '|', '>', '<', ';']

             if any(f in self.config['sos_opt_line'] for f in filt):

                 self.log_warn('Possible shell script found in provided sos '

                               'command. Ignoring --sos-cmd option entirely.')

                 self.config['sos_opt_line'] = None

             else:

                 self.config['sos_cmd'] = '%s %s' % (

-                    self.config['sos_cmd'], self.config['sos_opt_line'])

+                    self.config['sos_cmd'], quote(self.config['sos_opt_line']))

                 self.log_debug("User specified manual sosreport command. "

                                "Command set to %s" % self.config['sos_cmd'])

                 return True

         if self.config['case_id']:

-            self.config['sos_cmd'] += ' --case-id=%s' % self.config['case_id']

+            self.config['sos_cmd'] += ' --case-id=%s' % (

+                quote(self.config['case_id']))

         if self.config['alloptions']:

             self.config['sos_cmd'] += ' --alloptions'

         if self.config['verify']:

             self.config['sos_cmd'] += ' --verify'

         if self.config['log_size']:

             self.config['sos_cmd'] += (' --log-size=%s'

-                                       % self.config['log_size'])

+                                       % quote(self.config['log_size']))

         if self.config['sysroot']:

-            self.config['sos_cmd'] += ' -s %s' % self.config['sysroot']

+            self.config['sos_cmd'] += ' -s %s' % quote(self.config['sysroot'])

         if self.config['chroot']:

-            self.config['sos_cmd'] += ' -c %s' % self.config['chroot']

+            self.config['sos_cmd'] += ' -c %s' % quote(self.config['chroot'])

         if self.config['compression']:

-            self.config['sos_cmd'] += ' -z %s' % self.config['compression']

+            self.config['sos_cmd'] += ' -z %s' % (

+                quote(self.config['compression']))

         self.log_debug('Initial sos cmd set to %s' % self.config['sos_cmd'])

     def connect_to_master(self):

@@ -408,7 +411,7 @@ this utility or remote systems that it connects to.

         can still be run if the user sets a --cluster-type manually

         '''

         checks = list(self.clusters.values())

-        for cluster in checks:

+        for cluster in self.clusters.values():

             checks.remove(cluster)

             cluster.master = self.master

             if cluster.check_enabled():

diff --git a/soscollector/sosnode.py b/soscollector/sosnode.py

index fde1e6a..3790433 100644

--- a/soscollector/sosnode.py

+++ b/soscollector/sosnode.py

@@ -26,6 +26,7 @@ import six

 import time

 from distutils.version import LooseVersion

+from pipes import quote

 from subprocess import Popen, PIPE

@@ -450,7 +451,7 @@ class SosNode():

         label = self.determine_sos_label()

         if label:

-            self.sos_cmd = ' %s %s' % (self.sos_cmd, label)

+            self.sos_cmd = ' %s %s' % (self.sos_cmd, quote(label))

         if self.config['sos_opt_line']:

             return True

@@ -464,7 +465,7 @@ class SosNode():

                                'enabled but do not exist' % not_only)

             only = self._fmt_sos_opt_list(self.config['only_plugins'])

             if only:

-                self.sos_cmd += ' --only-plugins=%s' % only

+                self.sos_cmd += ' --only-plugins=%s' % quote(only)

             return True

         if self.config['skip_plugins']:

@@ -477,7 +478,7 @@ class SosNode():

                                'already not enabled' % not_skip)

             skipln = self._fmt_sos_opt_list(skip)

             if skipln:

-                self.sos_cmd += ' --skip-plugins=%s' % skipln

+                self.sos_cmd += ' --skip-plugins=%s' % quote(skipln)

         if self.config['enable_plugins']:

             # only run enable for plugins that are disabled

@@ -490,18 +491,18 @@ class SosNode():

                                'are already enabled or do not exist' % not_on)

             enable = self._fmt_sos_opt_list(opts)

             if enable:

-                self.sos_cmd += ' --enable-plugins=%s' % enable

+                self.sos_cmd += ' --enable-plugins=%s' % quote(enable)

         if self.config['plugin_options']:

             opts = [o for o in self.config['plugin_options']

                     if self._plugin_exists(o.split('.')[0])

                     and self._plugin_option_exists(o.split('=')[0])]

             if opts:

-                self.sos_cmd += ' -k %s' % ','.join(o for o in opts)

+                self.sos_cmd += ' -k %s' % quote(','.join(o for o in opts))

         if self.config['preset']:

             if self._preset_exists(self.config['preset']):

-                self.sos_cmd += ' --preset=%s' % self.config['preset']

+                self.sos_cmd += ' --preset=%s' % quote(self.config['preset'])

             else:

                 self.log_debug('Requested to enable preset %s but preset does '

                                'not exist on node' % self.config['preset'])

-- 

2.17.2