]> review.fuel-infra Code Review - openstack-build/cinder-build.git/commitdiff
Tidy up the SSH call to avoid injection attacks in storwize_svc
authorHaomai Wang <haomai@unitedstack.com>
Wed, 17 Jul 2013 13:36:55 +0000 (21:36 +0800)
committerHaomai Wang <haomai@unitedstack.com>
Wed, 31 Jul 2013 13:50:41 +0000 (21:50 +0800)
Let the command and arguments form up a list and avoid the extra arguments
attackers inserted to the command string

fix bug 1192971

Change-Id: I57b3fe60e64d9d0dc1ea9a18442c877be2ceece3

cinder/exception.py
cinder/tests/test_storwize_svc.py
cinder/utils.py
cinder/volume/drivers/san/san.py
cinder/volume/drivers/storwize_svc.py

index 777ec12838d5b729f66ba3045925b732295f2a63..f23c6fbe66017b960398344b468395550ae743d0 100644 (file)
@@ -606,3 +606,7 @@ class VolumeMigrationFailed(CinderException):
 
 class ProtocolNotSupported(CinderException):
     message = _("Connect to volume via protocol %(protocol)s not supported.")
+
+
+class SSHInjectionThreat(CinderException):
+    message = _("SSH command injection detected") + ": %(command)s"
index 02b1b59b2a2f045abcc3f3f136db99fff78d7ca5..9c2eea8e8fe37d09bc5ed928be014883f718b199 100644 (file)
@@ -181,8 +181,7 @@ class StorwizeSVCManagementSimulator:
         return True
 
     # Convert argument string to dictionary
-    def _cmd_to_dict(self, cmd):
-        arg_list = cmd.split()
+    def _cmd_to_dict(self, arg_list):
         no_param_args = [
             'autodelete',
             'autoexpand',
@@ -1156,7 +1155,6 @@ port_speed!N/A
 
         command = kwargs['cmd']
         del kwargs['cmd']
-        arg_list = cmd.split()
 
         if command == 'lsmdiskgrp':
             out, err = self._cmd_lsmdiskgrp(**kwargs)
index c263972d808bd7b666126c7ed6fb79a492d64ae7..d26943837bd193b11fc21c1d5bd4bb2f6035bf4a 100644 (file)
@@ -129,6 +129,30 @@ def trycmd(*args, **kwargs):
     return (stdout, stderr)
 
 
+def check_ssh_injection(cmd_list):
+    ssh_injection_pattern = ['`', '$', '|', '||', ';', '&', '&&', '>', '>>',
+                             '<']
+
+    # Check whether injection attacks exist
+    for arg in cmd_list:
+        arg = arg.strip()
+        # First, check no space in the middle of arg
+        arg_len = len(arg.split())
+        if arg_len > 1:
+            raise exception.SSHInjectionThreat(command=str(cmd_list))
+
+        # Second, check whether danger character in command. So the shell
+        # special operator must be a single argument.
+        for c in ssh_injection_pattern:
+            if arg == c:
+                continue
+
+            result = arg.find(c)
+            if not result == -1:
+                if result == 0 or not arg[result - 1] == '\\':
+                    raise exception.SSHInjectionThreat(command=cmd_list)
+
+
 def ssh_execute(ssh, cmd, process_input=None,
                 addl_env=None, check_exit_code=True):
     LOG.debug(_('Running cmd (SSH): %s'), cmd)
index ad532810eba662927c3733df081801d6c4308e17..bdf7767ed5941d5b73efd06ad1f56aa41b2798be 100644 (file)
@@ -100,7 +100,10 @@ class SanDriver(driver.VolumeDriver):
             command = ' '.join(cmd)
             return self._run_ssh(command, check_exit_code)
 
-    def _run_ssh(self, command, check_exit_code=True, attempts=1):
+    def _run_ssh(self, cmd_list, check_exit_code=True, attempts=1):
+        utils.check_ssh_injection(cmd_list)
+        command = ' '. join(cmd_list)
+
         if not self.sshpool:
             password = self.configuration.san_password
             privatekey = self.configuration.san_private_key
index 85ad8fbec1ed8ec8b24cc0d404d989fc04b36d5c..4e22aa652623639f48051611ccc52b4f2802ae6a 100755 (executable)
@@ -141,7 +141,7 @@ class StorwizeSVCDriver(san.SanDriver):
                                               for char in invalid_ch_in_host)
 
     def _get_iscsi_ip_addrs(self):
-        generator = self._port_conf_generator('svcinfo lsportip')
+        generator = self._port_conf_generator(['svcinfo', 'lsportip'])
         header = next(generator, None)
         if not header:
             return
@@ -166,7 +166,7 @@ class StorwizeSVCDriver(san.SanDriver):
     def _get_fc_wwpns(self):
         for key in self._storage_nodes:
             node = self._storage_nodes[key]
-            ssh_cmd = 'svcinfo lsnode -delim ! %s' % node['id']
+            ssh_cmd = ['svcinfo', 'lsnode', '-delim', '!', node['id']]
             raw = self._run_ssh(ssh_cmd)
             resp = CLIResponse(raw, delim='!', with_header=False)
             wwpns = set(node['WWPN'])
@@ -184,7 +184,7 @@ class StorwizeSVCDriver(san.SanDriver):
         self._context = ctxt
 
         # Validate that the pool exists
-        ssh_cmd = 'svcinfo lsmdiskgrp -delim ! -nohdr'
+        ssh_cmd = ['svcinfo', 'lsmdiskgrp', '-delim', '!', '-nohdr']
         out, err = self._run_ssh(ssh_cmd)
         self._assert_ssh_return(len(out.strip()), 'do_setup',
                                 ssh_cmd, out, err)
@@ -197,7 +197,7 @@ class StorwizeSVCDriver(san.SanDriver):
         # Check if compression is supported
         self._compression_enabled = False
         try:
-            ssh_cmd = 'svcinfo lslicense -delim !'
+            ssh_cmd = ['svcinfo', 'lslicense', '-delim', '!']
             out, err = self._run_ssh(ssh_cmd)
             license_lines = out.strip().split('\n')
             for license_line in license_lines:
@@ -210,7 +210,7 @@ class StorwizeSVCDriver(san.SanDriver):
             LOG.exception(_('Failed to get license information.'))
 
         # Get the iSCSI and FC names of the Storwize/SVC nodes
-        ssh_cmd = 'svcinfo lsnode -delim !'
+        ssh_cmd = ['svcinfo', 'lsnode', '-delim', '!']
         out, err = self._run_ssh(ssh_cmd)
         self._assert_ssh_return(len(out.strip()), 'do_setup',
                                 ssh_cmd, out, err)
@@ -346,8 +346,7 @@ class StorwizeSVCDriver(san.SanDriver):
         """Generate and store a randomly-generated CHAP secret for the host."""
 
         chap_secret = utils.generate_password()
-        ssh_cmd = ('svctask chhost -chapsecret "%(chap_secret)s" %(host_name)s'
-                   % {'chap_secret': chap_secret, 'host_name': host_name})
+        ssh_cmd = ['svctask', 'chhost', '-chapsecret', chap_secret, host_name]
         out, err = self._run_ssh(ssh_cmd)
         # No output should be returned from chhost
         self._assert_ssh_return(len(out.strip()) == 0,
@@ -360,7 +359,7 @@ class StorwizeSVCDriver(san.SanDriver):
         LOG.debug(_('enter: _get_chap_secret_for_host: host name %s')
                   % host_name)
 
-        ssh_cmd = 'svcinfo lsiscsiauth -delim !'
+        ssh_cmd = ['svcinfo', 'lsiscsiauth', '-delim', '!']
         out, err = self._run_ssh(ssh_cmd)
 
         if not len(out.strip()):
@@ -426,7 +425,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
     def _find_host_from_wwpn(self, connector):
         for wwpn in connector['wwpns']:
-            ssh_cmd = 'svcinfo lsfabric -wwpn %s -delim !' % wwpn
+            ssh_cmd = ['svcinfo', 'lsfabric', '-wwpn', wwpn, '-delim', '!']
             out, err = self._run_ssh(ssh_cmd)
 
             if not len(out.strip()):
@@ -456,7 +455,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
     def _find_host_exhaustive(self, connector, hosts):
         for host in hosts:
-            ssh_cmd = 'svcinfo lshost -delim ! %s' % host
+            ssh_cmd = ['svcinfo', 'lshost', '-delim', '!', host]
             out, err = self._run_ssh(ssh_cmd)
             self._assert_ssh_return(len(out.strip()),
                                     '_find_host_exhaustive',
@@ -487,7 +486,7 @@ class StorwizeSVCDriver(san.SanDriver):
         LOG.debug(_('enter: _get_host_from_connector: prefix %s') % prefix)
 
         # Get list of host in the storage
-        ssh_cmd = 'svcinfo lshost -delim !'
+        ssh_cmd = ['svcinfo', 'lshost', '-delim', '!']
         out, err = self._run_ssh(ssh_cmd)
 
         if not len(out.strip()):
@@ -541,15 +540,18 @@ class StorwizeSVCDriver(san.SanDriver):
         # When creating a host, we need one port
         self._driver_assert(len(ports), _('_create_host: No connector ports'))
         port1 = ports.pop(0)
-        ssh_cmd = ('svctask mkhost -force %(port1)s -name "%(host_name)s"' %
-                   {'port1': port1, 'host_name': host_name})
+        arg_name, arg_val = port1.split()
+        ssh_cmd = ['svctask', 'mkhost', '-force', arg_name, arg_val, '-name',
+                   '"%s"' % host_name]
         out, err = self._run_ssh(ssh_cmd)
         self._assert_ssh_return('successfully created' in out,
                                 '_create_host', ssh_cmd, out, err)
 
         # Add any additional ports to the host
         for port in ports:
-            ssh_cmd = ('svctask addhostport -force %s %s' % (port, host_name))
+            arg_name, arg_val = port.split()
+            ssh_cmd = ['svctask', 'addhostport', '-force', arg_name, arg_val,
+                       host_name]
             out, err = self._run_ssh(ssh_cmd)
 
         LOG.debug(_('leave: _create_host: host %(host)s - %(host_name)s') %
@@ -560,7 +562,7 @@ class StorwizeSVCDriver(san.SanDriver):
         """Return the defined storage mappings for a host."""
 
         return_data = {}
-        ssh_cmd = 'svcinfo lshostvdiskmap -delim ! %s' % host_name
+        ssh_cmd = ['svcinfo', 'lshostvdiskmap', '-delim', '!', host_name]
         out, err = self._run_ssh(ssh_cmd)
 
         mappings = out.strip().split('\n')
@@ -600,11 +602,8 @@ class StorwizeSVCDriver(san.SanDriver):
 
         # Volume is not mapped to host, create a new LUN
         if not mapped_flag:
-            ssh_cmd = ('svctask mkvdiskhostmap -host %(host_name)s -scsi '
-                       '%(result_lun)s %(volume_name)s' %
-                       {'host_name': host_name,
-                        'result_lun': result_lun,
-                        'volume_name': volume_name})
+            ssh_cmd = ['svctask', 'mkvdiskhostmap', '-host', host_name,
+                       '-scsi', result_lun, volume_name]
             out, err = self._run_ssh(ssh_cmd, check_exit_code=False)
             if err and err.startswith('CMMVC6071E'):
                 if not self.configuration.storwize_svc_multihostmap_enabled:
@@ -614,8 +613,11 @@ class StorwizeSVCDriver(san.SanDriver):
                                     'was not created because the VDisk is '\
                                     'already mapped to a host.\n"'
                     raise exception.CinderException(data=exception_msg)
-                ssh_cmd = ssh_cmd.replace('mkvdiskhostmap',
-                                          'mkvdiskhostmap -force')
+
+                for i in range(len(ssh_cmd)):
+                    if ssh_cmd[i] == 'mkvdiskhostmap':
+                        ssh_cmd.insert(i + 1, '-force')
+
                 # try to map one volume to multiple hosts
                 out, err = self._run_ssh(ssh_cmd)
                 LOG.warn(_('volume %s mapping to multi host') % volume_name)
@@ -636,7 +638,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
         LOG.debug(_('enter: _delete_host: host %s ') % host_name)
 
-        ssh_cmd = 'svctask rmhost %s ' % host_name
+        ssh_cmd = ['svctask', 'rmhost', host_name]
         out, err = self._run_ssh(ssh_cmd)
         # No output should be returned from rmhost
         self._assert_ssh_return(len(out.strip()) == 0,
@@ -646,7 +648,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
     def _get_conn_fc_wwpns(self, host_name):
         wwpns = []
-        cmd = 'svcinfo lsfabric -host %s' % host_name
+        cmd = ['svcinfo', 'lsfabric', '-host', host_name]
         generator = self._port_conf_generator(cmd)
         header = next(generator, None)
         if not header:
@@ -820,8 +822,8 @@ class StorwizeSVCDriver(san.SanDriver):
         # Check if vdisk-host mapping exists, remove if it does
         mapping_data = self._get_hostvdisk_mappings(host_name)
         if vol_name in mapping_data:
-            ssh_cmd = 'svctask rmvdiskhostmap -host %s %s' % \
-                (host_name, vol_name)
+            ssh_cmd = ['svctask', 'rmvdiskhostmap', '-host', host_name,
+                       vol_name]
             out, err = self._run_ssh(ssh_cmd)
             # Verify CLI behaviour - no output is returned from
             # rmvdiskhostmap
@@ -852,13 +854,13 @@ class StorwizeSVCDriver(san.SanDriver):
         parsed/matched to a single vdisk.
         """
 
-        ssh_cmd = 'svcinfo lsvdisk -bytes -delim ! %s ' % vdisk_name
+        ssh_cmd = ['svcinfo', 'lsvdisk', '-bytes', '-delim', '!', vdisk_name]
         return self._execute_command_and_parse_attributes(ssh_cmd)
 
     def _get_vdisk_fc_mappings(self, vdisk_name):
         """Return FlashCopy mappings that this vdisk is associated with."""
 
-        ssh_cmd = 'svcinfo lsvdiskfcmappings -nohdr %s' % vdisk_name
+        ssh_cmd = ['svcinfo', 'lsvdiskfcmappings', '-nohdr', vdisk_name]
         out, err = self._run_ssh(ssh_cmd)
 
         mapping_ids = []
@@ -921,31 +923,27 @@ class StorwizeSVCDriver(san.SanDriver):
         LOG.debug(_('enter: _create_vdisk: vdisk %s ') % name)
 
         model_update = None
-        autoex = '-autoexpand' if opts['autoexpand'] else ''
-        easytier = '-easytier on' if opts['easytier'] else '-easytier off'
+        easytier = 'on' if opts['easytier'] else 'off'
 
         # Set space-efficient options
         if opts['rsize'] == -1:
-            ssh_cmd_se_opt = ''
+            ssh_cmd_se_opt = []
         else:
-            ssh_cmd_se_opt = (
-                '-rsize %(rsize)d%% %(autoex)s -warning %(warn)d%%' %
-                {'rsize': opts['rsize'],
-                 'autoex': autoex,
-                 'warn': opts['warning']})
+            ssh_cmd_se_opt = ['-rsize', '%s%%' % str(opts['rsize']),
+                              '-autoexpand', '-warning',
+                              '%s%%' % str(opts['warning'])]
+            if not opts['autoexpand']:
+                ssh_cmd_se_opt.remove('-autoexpand')
+
             if opts['compression']:
-                ssh_cmd_se_opt = ssh_cmd_se_opt + ' -compressed'
+                ssh_cmd_se_opt.append('-compressed')
             else:
-                ssh_cmd_se_opt = ssh_cmd_se_opt + (
-                    ' -grainsize %d' % opts['grainsize'])
-
-        ssh_cmd = ('svctask mkvdisk -name %(name)s -mdiskgrp %(mdiskgrp)s '
-                   '-iogrp 0 -size %(size)s -unit '
-                   '%(unit)s %(easytier)s %(ssh_cmd_se_opt)s'
-                   % {'name': name,
-                   'mdiskgrp': self.configuration.storwize_svc_volpool_name,
-                   'size': size, 'unit': units, 'easytier': easytier,
-                   'ssh_cmd_se_opt': ssh_cmd_se_opt})
+                ssh_cmd_se_opt.extend(['-grainsize', str(opts['grainsize'])])
+
+        ssh_cmd = ['svctask', 'mkvdisk', '-name', name, '-mdiskgrp',
+                   self.configuration.storwize_svc_volpool_name,
+                   '-iogrp', '0', '-size', size, '-unit',
+                   units, '-easytier', easytier] + ssh_cmd_se_opt
         out, err = self._run_ssh(ssh_cmd)
         self._assert_ssh_return(len(out.strip()), '_create_vdisk',
                                 ssh_cmd, out, err)
@@ -964,12 +962,10 @@ class StorwizeSVCDriver(san.SanDriver):
         LOG.debug(_('leave: _create_vdisk: volume %s ') % name)
 
     def _make_fc_map(self, source, target, full_copy):
-        copyflag = '' if full_copy else '-copyrate 0'
-        fc_map_cli_cmd = ('svctask mkfcmap -source %(src)s -target %(tgt)s '
-                          '-autodelete %(copyflag)s' %
-                          {'src': source,
-                           'tgt': target,
-                           'copyflag': copyflag})
+        fc_map_cli_cmd = ['svctask', 'mkfcmap', '-source', source, '-target',
+                          target, '-autodelete']
+        if not full_copy:
+            fc_map_cli_cmd.extend(['-copyrate', '0'])
         out, err = self._run_ssh(fc_map_cli_cmd)
         self._driver_assert(
             len(out.strip()),
@@ -1020,7 +1016,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
     def _call_prepare_fc_map(self, fc_map_id, source, target):
         try:
-            out, err = self._run_ssh('svctask prestartfcmap %s' % fc_map_id)
+            out, err = self._run_ssh(['svctask', 'prestartfcmap', fc_map_id])
         except exception.ProcessExecutionError as e:
             with excutils.save_and_reraise_exception():
                 LOG.error(_('_prepare_fc_map: Failed to prepare FlashCopy '
@@ -1077,7 +1073,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
     def _start_fc_map(self, fc_map_id, source, target):
         try:
-            out, err = self._run_ssh('svctask startfcmap %s' % fc_map_id)
+            out, err = self._run_ssh(['svctask', 'startfcmap', fc_map_id])
         except exception.ProcessExecutionError as e:
             with excutils.save_and_reraise_exception():
                 LOG.error(_('_start_fc_map: Failed to start FlashCopy '
@@ -1148,8 +1144,8 @@ class StorwizeSVCDriver(san.SanDriver):
         LOG.debug(_('enter: _get_flashcopy_mapping_attributes: mapping %s')
                   % fc_map_id)
 
-        fc_ls_map_cmd = 'svcinfo lsfcmap -filtervalue id=%s -delim !' % \
-            fc_map_id
+        fc_ls_map_cmd = ['svcinfo', 'lsfcmap', '-filtervalue',
+                         'id=%s' % fc_map_id, '-delim', '!']
         out, err = self._run_ssh(fc_ls_map_cmd)
         if not len(out.strip()):
             return None
@@ -1204,8 +1200,8 @@ class StorwizeSVCDriver(san.SanDriver):
                     if source == name:
                         if not allow_snaps:
                             return False
-                        ssh_cmd = ('svctask chfcmap -copyrate 50 '
-                                   '-autodelete on %s' % map_id)
+                        ssh_cmd = ['svctask', 'chfcmap', '-copyrate', '50',
+                                   '-autodelete', 'on', map_id]
                         out, err = self._run_ssh(ssh_cmd)
                         wait_for_copy = True
                     # Case #3: A snapshot
@@ -1215,19 +1211,20 @@ class StorwizeSVCDriver(san.SanDriver):
                                {'name': name, 'src': source, 'tgt': target})
                         self._driver_assert(target == name, msg)
                         if status in ['copying', 'prepared']:
-                            self._run_ssh('svctask stopfcmap %s' % map_id)
+                            self._run_ssh(['svctask', 'stopfcmap', map_id])
                         elif status in ['stopping', 'preparing']:
                             wait_for_copy = True
                         else:
-                            self._run_ssh('svctask rmfcmap -force %s' % map_id)
+                            self._run_ssh(['svctask', 'rmfcmap', '-force',
+                                           map_id])
                 # Case 4: Copy in progress - wait and will autodelete
                 else:
                     if status == 'prepared':
-                        self._run_ssh('svctask stopfcmap %s' % map_id)
-                        self._run_ssh('svctask rmfcmap -force %s' % map_id)
+                        self._run_ssh(['svctask', 'stopfcmap', map_id])
+                        self._run_ssh(['svctask', 'rmfcmap', '-force', map_id])
                     elif status == 'idle_or_copied':
                         # Prepare failed
-                        self._run_ssh('svctask rmfcmap -force %s' % map_id)
+                        self._run_ssh(['svctask', 'rmfcmap', '-force', map_id])
                     else:
                         wait_for_copy = True
             if wait_for_copy:
@@ -1266,9 +1263,9 @@ class StorwizeSVCDriver(san.SanDriver):
 
         self._ensure_vdisk_no_fc_mappings(name)
 
-        forceflag = '-force' if force else ''
-        cmd_params = {'frc': forceflag, 'name': name}
-        ssh_cmd = 'svctask rmvdisk %(frc)s %(name)s' % cmd_params
+        ssh_cmd = ['svctask', 'rmvdisk', '-force', name]
+        if not force:
+            ssh_cmd.remove('-force')
         out, err = self._run_ssh(ssh_cmd)
         # No output should be returned from rmvdisk
         self._assert_ssh_return(len(out.strip()) == 0,
@@ -1336,8 +1333,8 @@ class StorwizeSVCDriver(san.SanDriver):
             raise exception.VolumeBackendAPIException(data=exception_message)
 
         extend_amt = int(new_size) - volume['size']
-        ssh_cmd = ('svctask expandvdisksize -size %(amt)d -unit gb %(name)s'
-                   % {'amt': extend_amt, 'name': volume['name']})
+        ssh_cmd = (['svctask', 'expandvdisksize', '-size', str(extend_amt),
+                    '-unit', 'gb', volume['name']])
         out, err = self._run_ssh(ssh_cmd)
         # No output should be returned from expandvdisksize
         self._assert_ssh_return(len(out.strip()) == 0, 'extend_volume',
@@ -1376,7 +1373,7 @@ class StorwizeSVCDriver(san.SanDriver):
 
         pool = self.configuration.storwize_svc_volpool_name
         #Get storage system name
-        ssh_cmd = 'svcinfo lssystem -delim !'
+        ssh_cmd = ['svcinfo', 'lssystem', '-delim', '!']
         attributes = self._execute_command_and_parse_attributes(ssh_cmd)
         if not attributes or not attributes['name']:
             exception_message = (_('_update_volume_stats: '
@@ -1388,7 +1385,7 @@ class StorwizeSVCDriver(san.SanDriver):
             backend_name = '%s_%s' % (attributes['name'], pool)
         data['volume_backend_name'] = backend_name
 
-        ssh_cmd = 'svcinfo lsmdiskgrp -bytes -delim ! %s' % pool
+        ssh_cmd = ['svcinfo', 'lsmdiskgrp', '-bytes', '-delim', '!', pool]
         attributes = self._execute_command_and_parse_attributes(ssh_cmd)
         if not attributes:
             LOG.error(_('Could not get pool data from the storage'))
@@ -1406,7 +1403,7 @@ class StorwizeSVCDriver(san.SanDriver):
         self._stats = data
 
     def _port_conf_generator(self, cmd):
-        ssh_cmd = '%s -delim !' % cmd
+        ssh_cmd = cmd + ['-delim', '!']
         out, err = self._run_ssh(ssh_cmd)
 
         if not len(out.strip()):
@@ -1483,7 +1480,7 @@ class StorwizeSVCDriver(san.SanDriver):
         """
 
         LOG.debug(_('enter: _execute_command_and_parse_attributes: '
-                    ' command %s') % ssh_cmd)
+                    ' command %s') % str(ssh_cmd))
 
         try:
             out, err = self._run_ssh(ssh_cmd)
@@ -1509,7 +1506,7 @@ class StorwizeSVCDriver(san.SanDriver):
         LOG.debug(_('leave: _execute_command_and_parse_attributes:\n'
                     'command: %(cmd)s\n'
                     'attributes: %(attr)s')
-                  % {'cmd': ssh_cmd,
+                  % {'cmd': str(ssh_cmd),
                      'attr': str(attributes)})
 
         return attributes