Skip to content

Commit

Permalink
[device/celestica] Mitigation for command injection vulnerability (#1…
Browse files Browse the repository at this point in the history
…1740)

Signed-off-by: maipbui <maibui@microsoft.com>
Dependency: [PR (#12065)](#12065) needs to merge first.
#### Why I did it
1. `eval()` - not secure against maliciously constructed input, can be dangerous if used to evaluate dynamic content. This may be a code injection vulnerability.
2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection.
3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content.
4. `is` operator - string comparison should not be used with reference equality.
5. `globals()` - extremely dangerous because it may allow an attacker to execute arbitrary code on the system
#### How I did it
1. `eval()` - use `literal_eval()`
2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation)
3. `os` - use with `subprocess`
4. `is` - replace by `==` operator for value equality
5. `globals()` - avoid the use of globals()
  • Loading branch information
maipbui authored Dec 9, 2022
1 parent d9eec94 commit 51a1eb1
Show file tree
Hide file tree
Showing 22 changed files with 159 additions and 303 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
RESET_REGISTER = "0x112"
HOST_REBOOT_CAUSE_PATH = "/host/reboot-cause/previous-reboot-cause.txt"
PMON_REBOOT_CAUSE_PATH = "/usr/share/sonic/platform/api_files/reboot-cause/previous-reboot-cause.txt"
HOST_CHK_CMD = "docker > /dev/null 2>&1"
STATUS_LED_PATH = "/sys/devices/platform/e1031.smc/master_led"


Expand Down
125 changes: 19 additions & 106 deletions device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import os
import ast
import imp
import yaml
import subprocess

from sonic_py_common import device_info


Expand All @@ -24,7 +24,7 @@ class Common:

SET_METHOD_IPMI = 'ipmitool'
NULL_VAL = 'N/A'
HOST_CHK_CMD = "docker > /dev/null 2>&1"
HOST_CHK_CMD = ["docker"]
REF_KEY = '$ref:'

def __init__(self, conf=None):
Expand All @@ -46,8 +46,7 @@ def run_command(self, command):
status = False
output = ""
try:
p = subprocess.Popen(
command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
p = subprocess.Popen(command, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
raw_data, err = p.communicate()
if p.returncode == 0:
status, output = True, raw_data.strip()
Expand All @@ -67,7 +66,7 @@ def _clean_input(self, input, config):
cleaned_input = input_translator.get(input)

elif type(input_translator) is str:
cleaned_input = eval(input_translator.format(input))
cleaned_input = ast.literal_eval(input_translator.format(input))

return cleaned_input

Expand All @@ -77,19 +76,12 @@ def _clean_output(self, index, output, config):
if type(output_translator) is dict:
output = output_translator.get(output)
elif type(output_translator) is str:
output = eval(output_translator.format(output))
output = ast.literal_eval(output_translator.format(output))
elif type(output_translator) is list:
output = eval(output_translator[index].format(output))
output = ast.literal_eval(output_translator[index].format(output))

return output

def _ipmi_get(self, index, config):
argument = config.get('argument')
cmd = config['command'].format(
config['argument'][index]) if argument else config['command']
status, output = self.run_command(cmd)
return output if status else None

def _sysfs_read(self, index, config):
sysfs_path = config.get('sysfs_path')
argument = config.get('argument', '')
Expand Down Expand Up @@ -132,10 +124,6 @@ def _sysfs_write(self, index, config, input):
return False, output
return True, output

def _ipmi_set(self, index, config, input):
arg = config['argument'][index].format(input)
return self.run_command(config['command'].format(arg))

def _hex_ver_decode(self, hver, num_of_bits, num_of_points):
ver_list = []
c_bit = 0
Expand All @@ -159,14 +147,16 @@ def _get_class(self, config):
return class_

def get_reg(self, path, reg_addr):
cmd = "echo {1} > {0}; cat {0}".format(path, reg_addr)
status, output = self.run_command(cmd)
return output if status else None
with open(path, 'w') as file:
file.write(reg_addr + '\n')
with open(path, 'r') as file:
output = file.readline().strip()
return output

def set_reg(self, path, reg_addr, value):
cmd = "echo {0} {1} > {2}".format(reg_addr, value, path)
status, output = self.run_command(cmd)
return output if status else None
with open(path, 'w') as file:
file.write("{0} {1}\n".format(reg_addr, value))
return None

def read_txt_file(self, path):
try:
Expand Down Expand Up @@ -195,7 +185,11 @@ def write_txt_file(self, file_path, value):
return True

def is_host(self):
return os.system(self.HOST_CHK_CMD) == 0
try:
subprocess.call(self.HOST_CHK_CMD, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
except FileNotFoundError:
return False
return True

def load_json_file(self, path):
"""
Expand All @@ -221,87 +215,6 @@ def get_config_path(self, config_name):
"""
return os.path.join(self.DEVICE_PATH, self.platform, self.CONFIG_DIR, config_name) if self.is_host() else os.path.join(self.PMON_PLATFORM_PATH, self.CONFIG_DIR, config_name)

def get_output(self, index, config, default):
"""
Retrieves the output for each function base on config
Args:
index: An integer containing the index of device.
config: A dict object containing the configuration of specified function.
default: A string containing the default output of specified function.
Returns:
A string containing the output of specified function in config
"""
output_source = config.get('output_source')

if output_source == self.OUTPUT_SOURCE_IPMI:
output = self._ipmi_get(index, config)

elif output_source == self.OUTPUT_SOURCE_GIVEN_VALUE:
output = config["value"]

elif output_source == self.OUTPUT_SOURCE_GIVEN_CLASS:
output = self._get_class(config)

elif output_source == self.OUTPUT_SOURCE_GIVEN_LIST:
output = config["value_list"][index]

elif output_source == self.OUTPUT_SOURCE_SYSFS:
output = self._sysfs_read(index, config)

elif output_source == self.OUTPUT_SOURCE_FUNC:
func_conf = self._main_conf[config['function'][index]]
output = self.get_output(index, func_conf, default)

elif output_source == self.OUTPUT_SOURCE_GIVEN_TXT_FILE:
path = config.get('path')
output = self.read_txt_file(path)

elif output_source == self.OUTPUT_SOURCE_GIVEN_VER_HEX_FILE:
path = config.get('path')
hex_ver = self.read_txt_file(path)
output = self._hex_ver_decode(
hex_ver, config['num_of_bits'], config['num_of_points'])

elif output_source == self.OUTPUT_SOURCE_GIVEN_VER_HEX_ADDR:
path = config.get('path')
addr = config.get('reg_addr')
hex_ver = self.get_reg(path, addr)
output = self._hex_ver_decode(
hex_ver, config['num_of_bits'], config['num_of_points'])

else:
output = default

return self._clean_output(index, output, config) or default

def set_output(self, index, input, config):
"""
Sets the output of specified function on config
Args:
config: A dict object containing the configuration of specified function.
index: An integer containing the index of device.
input: A string containing the input of specified function.
Returns:
bool: True if set function is successfully, False if not
"""
cleaned_input = self._clean_input(input, config)
if not cleaned_input:
return False

set_method = config.get('set_method')
if set_method == self.SET_METHOD_IPMI:
output = self._ipmi_set(index, config, cleaned_input)[0]
elif set_method == self.OUTPUT_SOURCE_SYSFS:
output = self._sysfs_write(index, config, cleaned_input)[0]
else:
output = False

return output

def get_event(self, timeout, config, sfp_list):
"""
Returns a nested dictionary containing all devices which have
Expand Down
20 changes: 8 additions & 12 deletions device/celestica/x86_64-cel_e1031-r0/sonic_platform/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
try:
import os.path
import shutil
import shlex
import subprocess
from sonic_platform_base.component_base import ComponentBase
except ImportError as e:
Expand Down Expand Up @@ -39,8 +38,7 @@ def __init__(self, component_index):
def __run_command(self, command):
# Run bash command and print output to stdout
try:
process = subprocess.Popen(
shlex.split(command), universal_newlines=True, stdout=subprocess.PIPE)
process = subprocess.Popen(command, universal_newlines=True, stdout=subprocess.PIPE)
while True:
output = process.stdout.readline()
if output == '' and process.poll() is not None:
Expand All @@ -63,24 +61,22 @@ def __get_bios_version(self):

def get_register_value(self, register):
# Retrieves the cpld register value
cmd = "echo {1} > {0}; cat {0}".format(GETREG_PATH, register)
p = subprocess.Popen(
cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
raw_data, err = p.communicate()
if err is not '':
return None
with open(GETREG_PATH, 'w') as file:
file.write(register + '\n')
with open(GETREG_PATH, 'r') as file:
raw_data = file.readline()
return raw_data.strip()

def __get_cpld_version(self):
# Retrieves the CPLD firmware version
cpld_version = dict()
with open(SMC_CPLD_PATH, 'r') as fd:
smc_cpld_version = fd.read()
smc_cpld_version = 'None' if smc_cpld_version is 'None' else "{}.{}".format(
smc_cpld_version = 'None' if smc_cpld_version == 'None' else "{}.{}".format(
int(smc_cpld_version[2], 16), int(smc_cpld_version[3], 16))

mmc_cpld_version = self.get_register_value(MMC_CPLD_ADDR)
mmc_cpld_version = 'None' if mmc_cpld_version is 'None' else "{}.{}".format(
mmc_cpld_version = 'None' if mmc_cpld_version == 'None' else "{}.{}".format(
int(mmc_cpld_version[2], 16), int(mmc_cpld_version[3], 16))

cpld_version["SMC_CPLD"] = smc_cpld_version
Expand Down Expand Up @@ -159,7 +155,7 @@ def install_firmware(self, image_path):
ext = ".vme" if ext == "" else ext
new_image_path = os.path.join("/tmp", (root.lower() + ext))
shutil.copy(image_path, new_image_path)
install_command = "ispvm %s" % new_image_path
install_command = ["ispvm", str(new_image_path)]
# elif self.name == "BIOS":
# install_command = "afulnx_64 %s /p /b /n /x /r" % image_path
return self.__run_command(install_command)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,5 @@ def execute(self, thermal_info_dict):
thermal_overload_position = Common().read_txt_file(
thermal_overload_position_path)

cmd = 'bash /usr/share/sonic/platform/thermal_overload_control.sh {}'.format(
thermal_overload_position)
cmd = ['bash', '/usr/share/sonic/platform/thermal_overload_control.sh', thermal_overload_position]
Common().run_command(cmd)
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@


class ThermalManager(ThermalManagerBase):
FSC_ALGORITHM_CMD = ' supervisorctl {} fancontrol'
FSC_ALGORITHM_CMD = ['supervisorctl', '', 'fancontrol']

@classmethod
def start_thermal_control_algorithm(cls):
Expand Down Expand Up @@ -43,5 +43,5 @@ def _enable_fancontrol_service(cls, enable):
Returns:
bool: True if set success, False if fail.
"""
cmd = 'start' if enable else 'stop'
return Common().run_command(cls.FSC_ALGORITHM_CMD.format(cmd))
cls.FSC_ALGORITHM_CMD[1] = 'start' if enable else 'stop'
return Common().run_command(cls.FSC_ALGORITHM_CMD)
2 changes: 0 additions & 2 deletions device/celestica/x86_64-cel_midstone-r0/plugins/psuutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
#
#############################################################################

import os.path
import subprocess

try:
from sonic_psu.psu_base import PsuBase
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
REBOOT_CAUSE_FILE = "reboot-cause.txt"
PREV_REBOOT_CAUSE_FILE = "previous-reboot-cause.txt"
GETREG_PATH = "/sys/devices/platform/dx010_cpld/getreg"
HOST_CHK_CMD = "docker > /dev/null 2>&1"
STATUS_LED_PATH = "/sys/devices/platform/leds_dx010/leds/dx010:green:stat/brightness"


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import os.path
import shutil
import subprocess

try:
from sonic_platform_base.component_base import ComponentBase
Expand Down Expand Up @@ -52,12 +51,10 @@ def __get_bios_version(self):

def get_register_value(self, register):
# Retrieves the cpld register value
cmd = "echo {1} > {0}; cat {0}".format(GETREG_PATH, register)
p = subprocess.Popen(
cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
raw_data, err = p.communicate()
if err is not '':
return None
with open(GETREG_PATH, 'w') as file:
file.write(register + '\n')
with open(GETREG_PATH, 'r') as file:
raw_data = file.readline()
return raw_data.strip()

def __get_cpld_version(self):
Expand Down Expand Up @@ -146,11 +143,11 @@ def install_firmware(self, image_path):
ext = ".vme" if ext == "" else ext
new_image_path = os.path.join("/tmp", (root.lower() + ext))
shutil.copy(image_path, new_image_path)
install_command = "ispvm %s" % new_image_path
install_command = ["ispvm", str(new_image_path)]
# elif self.name == "BIOS":
# install_command = "afulnx_64 %s /p /b /n /x /r" % image_path

return self.__run_command(install_command)
return self._api_helper.run_command(install_command)


def update_firmware(self, image_path):
Expand Down
Loading

0 comments on commit 51a1eb1

Please sign in to comment.