Skip to content

Commit

Permalink
[sonic_sku_create] remove shell=True, replace exit() with sys.exit() (#…
Browse files Browse the repository at this point in the history 
…2816)

#### What I did
`subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection.
`sys.exit` is better than `exit`, considered good to use in production code.
Ref:
https://stackoverflow.com/questions/6501121/difference-between-exit-and-sys-exit-in-python
https://stackoverflow.com/questions/19747371/python-exit-commands-why-so-many-and-when-should-each-be-used
#### How I did it
`subprocess()` - use `shell=False` instead, use list of strings Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation)
Replace `exit()` by `sys.exit()`
#### How to verify it
Add UT
  • Loading branch information
@maipbui
maipbui authored May 9, 2023
1 parent 71ef4f1 commit a5091bb
Show file tree
Hide file tree
Showing 4 changed files with 644 additions and 30 deletions.
60 changes: 30 additions & 30 deletions scripts/sonic_sku_create.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
from tabulate import tabulate
from lxml import etree as ET
from lxml.etree import QName

from sonic_py_common.general import check_output_pipe

minigraph_ns = "Microsoft.Search.Autopilot.Evolution"
minigraph_ns1 = "http://schemas.datacontract.org/2004/07/Microsoft.Search.Autopilot.Evolution"
Expand Down Expand Up @@ -105,10 +105,10 @@ def sku_def_parser(self, sku_def):
# Parsing XML sku definition file to extract Interface speed and InterfaceName(alias) <etp<#><a/b/c/d>|<Ethernet<#>/<#> to be used to analyze split configuration
# Rest of the fields are used as placeholders for portconfig_dict [name,lanes,SPEED,ALIAS,index]
try:
f = open(str(sku_def),"r")
f = open(str(sku_def), "r")
except IOError:
print("Couldn't open file: " + str(sku_def), file=sys.stderr)
exit(1)
sys.exit(1)
element = ET.parse(f)

root = element.getroot()
Expand Down Expand Up @@ -184,7 +184,7 @@ def check_json_lanes_with_bko(self, data, port_idx):
int_port_speed = int(port_speed)
else:
print(port_str, "does not contain speed key, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
for i in range(1,self.base_lanes):
curr_port_str = "Ethernet{:d}".format(port_idx+i)
if curr_port_str in data['PORT']:
Expand All @@ -193,20 +193,20 @@ def check_json_lanes_with_bko(self, data, port_idx):
curr_speed = curr_port_dict.get("speed")
else:
print(curr_port_str, "does not contain speed key, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
if port_speed != curr_speed:
print(curr_port_str, "speed is different from that of ",port_str,", Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
if "alias" in curr_port_dict:
curr_alias = curr_port_dict.get("alias")
else:
print(curr_port_str, "does not contain alias key, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
if "lanes" in curr_port_dict:
curr_lanes = curr_port_dict.get("lanes")
else:
print(curr_port_str, "does not contain lanes key, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
port_bmp |= (1<<i)

for entry in self.bko_dict:
Expand Down Expand Up @@ -263,8 +263,8 @@ def json_file_parser(self, json_file):
pattern = '^Ethernet([0-9]{1,})'
m = re.match(pattern,key)
if m is None:
print("Port Name ",port_name, " is not valid, Exiting...", file=sys.stderr)
exit(1)
print("Port Name ", key, " is not valid, Exiting...", file=sys.stderr)
sys.exit(1)
port_idx = int(m.group(1))

if port_idx%self.base_lanes == 0:
Expand Down Expand Up @@ -300,7 +300,7 @@ def parse_platform_from_config_db_file(self, config_file):
m = re.match(pattern,platform)
if m is None:
print("Platform Name ", platform, " is not valid, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
self.platform = platform


Expand Down Expand Up @@ -351,7 +351,7 @@ def form_port_config_dict_from_ini(self, ini_file):
idx += 1
else:
print("port_config.ini file does not contain all fields, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)

f_in.close()

Expand All @@ -362,26 +362,26 @@ def break_in_ini(self, ini_file, port_name, port_split):
m = re.match(pattern,port_split)
if m is None:
print("Port split format ",port_split, " is not valid, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
if port_split in self.bko_dict:
step = self.bko_dict[port_split]["step"]
speed = self.bko_dict[port_split]["speed"]
base_lanes = self.bko_dict[port_split]["lanes"]
bko = self.bko_dict[port_split]["bko"]
else:
print("Port split ",port_split, " is undefined for this platform, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)

port_found = False
pattern = '^Ethernet([0-9]{1,})'
m = re.match(pattern,port_name)
if m is None:
print("Port Name ",port_name, " is not valid, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
port_idx = int(m.group(1))
if port_idx % base_lanes != 0:
print(port_name, " is not base port, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)

bak_file = ini_file + ".bak"
shutil.copy(ini_file, bak_file)
Expand Down Expand Up @@ -528,7 +528,7 @@ def break_a_port(self, port_name, port_split):
shutil.copy(new_file,self.ini_file)
if lanes_str_result is None:
print("break_in_ini function returned empty lanes string, Exiting...", file=sys.stderr)
exit(1)
sys.exit(1)
self.break_in_cfg(self.cfg_file,port_name,port_split,lanes_str_result)

def _parse_interface_alias(self, alias):
Expand Down Expand Up @@ -573,7 +573,7 @@ def get_default_lanes(self):

except IOError:
print("Could not open file "+ self.base_file_path, file=sys.stderr)
exit(1)
sys.exit(1)

def set_lanes(self):
#set lanes and index per interfaces based on split
Expand All @@ -590,7 +590,7 @@ def set_lanes(self):
lanes_count = len(lanes)
if lanes_count % splt != 0:
print("Lanes(%s) could not be evenly splitted by %d." % (self.default_lanes_per_port[fp - 1], splt))
exit(1)
sys.exit(1)

# split the lanes
it = iter(lanes)
Expand Down Expand Up @@ -639,13 +639,13 @@ def create_port_config(self):
#create a port_config.ini file based on the sku definition
if not os.path.exists(self.new_sku_dir):
print("Error - path:", self.new_sku_dir, " doesn't exist",file=sys.stderr)
exit(1)
sys.exit(1)

try:
f = open(self.new_sku_dir+"port_config.ini","w+")
except IOError:
print("Could not open file "+ self.new_sku_dir+"port_config.ini", file=sys.stderr)
exit(1)
sys.exit(1)
header = PORTCONFIG_HEADER # ["name", "lanes", "alias", "index"]
port_config = []
for line in self.portconfig_dict.values():
Expand All @@ -670,7 +670,7 @@ def create_sku_dir(self):
# create a new SKU directory based on the base SKU
if (os.path.exists(self.new_sku_dir)):
print("SKU directory: "+self.new_sku_dir+ " already exists\n Please use -r flag to remove the SKU dir first", file=sys.stderr)
exit(1)
sys.exit(1)
try:
shutil.copytree(self.base_sku_dir, self.new_sku_dir)
except OSError as e:
Expand All @@ -680,7 +680,7 @@ def remove_sku_dir(self):
# remove SKU directory
if (self.new_sku_dir == self.base_sku_dir):
print("Removing the base SKU" + self.new_sku_dir + " is not allowed", file=sys.stderr)
exit(1)
sys.exit(1)
try:
if not os.path.exists(self.new_sku_dir):
print("Trying to remove a SKU "+ self.new_sku_dir + " that doesn't exists, Ignoring -r command")
Expand Down Expand Up @@ -726,7 +726,7 @@ def msn2700_specific(self):
raise ValueError()
except ValueError:
print("Error - Illegal split by 4 ", file=sys.stderr)
exit(1)
sys.exit(1)


def main(argv):
Expand Down Expand Up @@ -759,18 +759,18 @@ def main(argv):
sku.default_sku_path = args.default_sku_path[0]
else:
try:
sku.platform = subprocess.check_output("sonic-cfggen -H -v DEVICE_METADATA.localhost.platform",shell=True, text=True) #self.metadata['platform']
sku.platform = subprocess.check_output(["sonic-cfggen", "-H", "-v", "DEVICE_METADATA.localhost.platform"], text=True) #self.metadata['platform']
sku.platform = sku.platform.rstrip()
except KeyError:
print("Couldn't find platform info in CONFIG_DB DEVICE_METADATA", file=sys.stderr)
exit(1)
sys.exit(1)
sku.default_sku_path = '/usr/share/sonic/device/' + sku.platform

if args.base:
sku.base_sku_name = args.base
else:
f=open(sku.default_sku_path + '/' + "default_sku","r")
sku.base_sku_name=f.read().split()[0]
f = open(sku.default_sku_path + '/' + "default_sku", "r")
sku.base_sku_name = f.read().split()[0]

sku.base_sku_dir = sku.default_sku_path + '/' + sku.base_sku_name + '/'
sku.base_file_path = sku.base_sku_dir + "port_config.ini"
Expand Down Expand Up @@ -803,10 +803,10 @@ def main(argv):
sku.parse_platform_from_config_db_file(sku.cfg_file)
else:
try:
sku_name = subprocess.check_output("show platform summary | grep HwSKU ",shell=True).rstrip().split()[1]
sku_name = check_output_pipe(["show", "platform", "summary"], ["grep", "HwSKU"]).rstrip().split()[1]
except KeyError:
print("Couldn't find HwSku info in Platform summary", file=sys.stderr)
exit(1)
sys.exit(1)
sku.ini_file = sku.default_sku_path + "/" + sku_name + "/port_config.ini"
sku.cfg_file = "/etc/sonic/config_db.json"

Expand Down
12 changes: 12 additions & 0 deletions tests/sku_create_input/2700_files/config_db_incorrect_platform.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"DEVICE_METADATA": {
"localhost": {
"hwsku": "Mellanox-SN2700-C28D8",
"hostname": "sonic",
"platform": "x86_64-mlnx_2700-r0",
"mac": "7c:fe:90:f8:06:80",
"bgp_asn": "65100",
"type": "LeafRouter"
}
}
}
Loading

0 comments on commit a5091bb

Please sign in to comment.