From c09e36927f74f955a325381a7d611ea270d04b65 Mon Sep 17 00:00:00 2001 From: Jake Hunsaker 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 --- 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