Skip to content

Commit 991373c

Browse files
GREENRAT-K405GREENRAT-K405
committed
complete shell escaping
1 parent 83193b5 commit 991373c

1 file changed

Lines changed: 52 additions & 21 deletions

File tree

‎mkconcore.py‎

Lines changed: 52 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,8 @@ def safe_name(value, context):
8181
"""
8282
if not value:
8383
raise ValueError(f"{context} cannot be empty")
84-
#blocks path traversal (/, \\) and shell metacharacters (*, ?, <, >, |, ;, &, $, `)
85-
if re.search(r'[\\/:*?"<>|;&`$]', value):
84+
# blocks path traversal (/, \), control characters, and shell metacharacters (*, ?, <, >, |, ;, &, $, `, ', ", (, ))
85+
if re.search(r'[\x00-\x1F\x7F\\/:*?"<>|;&`$\'()]', value):
8686
raise ValueError(f"Unsafe {context}: '{value}' contains illegal characters.")
8787
return value
8888

@@ -130,6 +130,10 @@ def safe_name(value, context):
130130
prefixedgenode = ""
131131
sourcedir = sys.argv[2]
132132
outdir = sys.argv[3]
133+
134+
# Validate outdir argument
135+
safe_name(outdir, "Output directory argument")
136+
133137
if not os.path.isdir(sourcedir):
134138
logging.error(f"{sourcedir} does not exist")
135139
quit()
@@ -223,6 +227,8 @@ def safe_name(value, context):
223227
safe_name(source_part, f"Node source file '{source_part}'")
224228
else:
225229
safe_name(node_label, f"Node label '{node_label}'")
230+
# Explicitly reject incorrect format to prevent later crashes and ambiguity
231+
raise ValueError(f"Invalid node label '{node_label}': expected format 'container:source' with a ':' separator.")
226232

227233
nodes_dict[node['id']] = node_label
228234
node_id_to_label_map[node['id']] = node_label.split(':')[0]
@@ -722,7 +728,9 @@ def safe_name(value, context):
722728
dockername = sourcecode.split(".")[0] #3/28/21
723729
writeedges = volswr[i]
724730
while writeedges.find(":") != -1:
725-
fclear.write(DOCKEREXE+' volume rm ' +writeedges.split(":")[0].split("-v")[1].strip()+"\n") # Added strip()
731+
#scape volume path using shlex.quote for Docker commands (defense-in-depth)
732+
volume_path = writeedges.split(":")[0].split("-v")[1].strip()
733+
fclear.write(DOCKEREXE+' volume rm ' + shlex.quote(volume_path) +"\n") # Added strip() and quote
726734
writeedges = writeedges[writeedges.find(":")+1:]
727735
i=i+1
728736
fclear.close()
@@ -741,8 +749,10 @@ def safe_name(value, context):
741749
writeedges = volswr[i]
742750
while writeedges.find(":") != -1:
743751
fmaxtime.write(' -v ')
744-
fmaxtime.write(writeedges.split(":")[0].split("-v ")[1].strip()+":/")
745-
fmaxtime.write(writeedges.split(":")[0].split("-v ")[1].strip())
752+
# escape volume paths in Docker run
753+
vol_path = writeedges.split(":")[0].split("-v ")[1].strip()
754+
fmaxtime.write(shlex.quote(vol_path)+":/")
755+
fmaxtime.write(shlex.quote(vol_path))
746756
writeedges = writeedges[writeedges.find(":")+1:]
747757
i=i+1
748758
fmaxtime.write(' docker-concore >/dev/null &\n')
@@ -756,7 +766,9 @@ def safe_name(value, context):
756766
writeedges = volswr[i]
757767
while writeedges.find(":") != -1:
758768
fmaxtime.write('sudo docker cp concore.maxtime concore:/')
759-
fmaxtime.write(writeedges.split(":")[0].split("-v ")[1].strip()+"/concore.maxtime\n")
769+
# escape destination path in docker cp
770+
vol_path = writeedges.split(":")[0].split("-v ")[1].strip()
771+
fmaxtime.write(shlex.quote(vol_path+"/concore.maxtime")+"\n")
760772
writeedges = writeedges[writeedges.find(":")+1:]
761773
i=i+1
762774
fmaxtime.write('sudo docker stop concore \n')
@@ -780,8 +792,10 @@ def safe_name(value, context):
780792
writeedges = volswr[i]
781793
while writeedges.find(":") != -1:
782794
fparams.write(' -v ')
783-
fparams.write(writeedges.split(":")[0].split("-v ")[1].strip()+":/")
784-
fparams.write(writeedges.split(":")[0].split("-v ")[1].strip())
795+
#escape volume paths
796+
vol_path = writeedges.split(":")[0].split("-v ")[1].strip()
797+
fparams.write(shlex.quote(vol_path)+":/")
798+
fparams.write(shlex.quote(vol_path))
785799
writeedges = writeedges[writeedges.find(":")+1:]
786800
i=i+1
787801
fparams.write(' docker-concore >/dev/null &\n')
@@ -795,7 +809,9 @@ def safe_name(value, context):
795809
writeedges = volswr[i]
796810
while writeedges.find(":") != -1:
797811
fparams.write('sudo docker cp concore.params concore:/')
798-
fparams.write(writeedges.split(":")[0].split("-v ")[1].strip()+"/concore.params\n")
812+
# escape destination path
813+
vol_path = writeedges.split(":")[0].split("-v ")[1].strip()
814+
fparams.write(shlex.quote(vol_path+"/concore.params")+"\n")
799815
writeedges = writeedges[writeedges.find(":")+1:]
800816
i=i+1
801817
fparams.write('sudo docker stop concore \n')
@@ -818,8 +834,10 @@ def safe_name(value, context):
818834
writeedges = volswr[i]
819835
while writeedges.find(":") != -1:
820836
funlock.write(' -v ')
821-
funlock.write(writeedges.split(":")[0].split("-v ")[1].strip()+":/")
822-
funlock.write(writeedges.split(":")[0].split("-v ")[1].strip())
837+
# escape volume paths
838+
vol_path = writeedges.split(":")[0].split("-v ")[1].strip()
839+
funlock.write(shlex.quote(vol_path)+":/")
840+
funlock.write(shlex.quote(vol_path))
823841
writeedges = writeedges[writeedges.find(":")+1:]
824842
i=i+1
825843
funlock.write(' docker-concore >/dev/null &\n')
@@ -833,7 +851,9 @@ def safe_name(value, context):
833851
writeedges = volswr[i]
834852
while writeedges.find(":") != -1:
835853
funlock.write('sudo docker cp ~/concore.apikey concore:/')
836-
funlock.write(writeedges.split(":")[0].split("-v ")[1].strip()+"/concore.apikey\n")
854+
# escape destination path
855+
vol_path = writeedges.split(":")[0].split("-v ")[1].strip()
856+
funlock.write(shlex.quote(vol_path+"/concore.apikey")+"\n")
837857
writeedges = writeedges[writeedges.find(":")+1:]
838858
i=i+1
839859
funlock.write('sudo docker stop concore \n')
@@ -850,7 +870,8 @@ def safe_name(value, context):
850870
dockername,langext = sourcecode.split(".")
851871
# safe_container added to debug line (POSIX)
852872
safe_container = shlex.quote(containername)
853-
fdebug.write(DOCKEREXE+' run -it --name='+safe_container+volswr[i]+volsro[i]+" docker-"+dockername+"&\n")
873+
safe_image = shlex.quote("docker-" + dockername) # escape docker image name
874+
fdebug.write(DOCKEREXE+' run -it --name='+safe_container+volswr[i]+volsro[i]+" "+safe_image+"&\n")
854875
i=i+1
855876
fdebug.close()
856877
os.chmod(outdir+"/build",stat.S_IRWXU)
@@ -994,12 +1015,13 @@ def safe_name(value, context):
9941015
fdebug.write('start /D '+q_container+' cmd /K vvp a.out\n')
9951016
#fdebug.write('start /D '+containername+' cmd /K "'+CPPWIN+' '+sourcecode+'|a"\n')
9961017
elif langext=="m": #3/23/21
1018+
# Use q_source in Windows commands to ensure quoting consistency
9971019
if M_IS_OCTAVE:
998-
frun.write('start /B /D '+q_container+" "+OCTAVEWIN+' -qf --eval "run('+"'"+sourcecode+"'"+')"'+" >"+q_container+"\\concoreout.txt\n")
999-
fdebug.write('start /D '+q_container+" cmd /K " +OCTAVEWIN+' -qf --eval "run('+"'"+sourcecode+"'"+')"'+"\n")
1020+
frun.write('start /B /D '+q_container+" "+OCTAVEWIN+' -qf --eval "run('+q_source+')"'+" >"+q_container+"\\concoreout.txt\n")
1021+
fdebug.write('start /D '+q_container+" cmd /K " +OCTAVEWIN+' -qf --eval "run('+q_source+')"'+"\n")
10001022
else: # 4/2/21
1001-
frun.write('start /B /D '+q_container+" "+MATLABWIN+' -batch "run('+"'"+sourcecode+"'"+')"'+" >"+q_container+"\\concoreout.txt\n")
1002-
fdebug.write('start /D '+q_container+" cmd /K " +MATLABWIN+' -batch "run('+"'"+sourcecode+"'"+')"'+"\n")
1023+
frun.write('start /B /D '+q_container+" "+MATLABWIN+' -batch "run('+q_source+')"'+" >"+q_container+"\\concoreout.txt\n")
1024+
fdebug.write('start /D '+q_container+" cmd /K " +MATLABWIN+' -batch "run('+q_source+')"'+"\n")
10031025
else:
10041026
#use shlex.quote for POSIX systems
10051027
safe_container = shlex.quote(containername)
@@ -1034,15 +1056,22 @@ def safe_name(value, context):
10341056
fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + safe_container + '\\\\\\"; ' + VEXE + ' ' + safe_source + '; vvp a.out\\"" \n')
10351057

10361058
elif langext == "sh": # 5/19/21
1037-
frun.write('(cd ' + safe_container + '; ./' + safe_source + ' ' + MCRPATH + ' >concoreout.txt & echo $! >concorepid) &\n')
1059+
# FIX: Escape MCRPATH to prevent shell injection
1060+
safe_mcr = shlex.quote(MCRPATH)
1061+
frun.write('(cd ' + safe_container + '; ./' + safe_source + ' ' + safe_mcr + ' >concoreout.txt & echo $! >concorepid) &\n')
10381062
if ubuntu:
10391063
fdebug.write('concorewd="$(pwd)"\n')
1040-
fdebug.write('xterm -e bash -c "cd \\"$concorewd/' + safe_container + '\\"; ./' + safe_source + ' ' + MCRPATH + '; bash" &\n')
1064+
fdebug.write('xterm -e bash -c "cd \\"$concorewd/' + safe_container + '\\"; ./' + safe_source + ' ' + safe_mcr + '; bash" &\n')
10411065
else:
10421066
fdebug.write('concorewd="$(pwd)"\n')
1043-
fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + safe_container + '\\\\\\"; ./' + safe_source + ' ' + MCRPATH + '\\"" \n')
1067+
fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + safe_container + '\\\\\\"; ./' + safe_source + ' ' + safe_mcr + '\\"" \n')
10441068

10451069
elif langext == "m": #3/23/21
1070+
# FIX: Verify filename safety for MATLAB to prevent injection in run()
1071+
# MATLAB/Octave run('filename') is vulnerable if filename contains quotes or metachars.
1072+
if not re.match(r'^[A-Za-z0-9_./\-]+$', sourcecode):
1073+
raise ValueError(f"Invalid MATLAB/Octave source file name: {sourcecode!r}")
1074+
10461075
# construct safe eval command
10471076
safe_eval_cmd = shlex.quote(f"run('{sourcecode}')")
10481077
if M_IS_OCTAVE:
@@ -1093,7 +1122,9 @@ def safe_name(value, context):
10931122
if concoretype=="windows":
10941123
fclear.write('del /Q "' + path_part + '\\*"\n')
10951124
else:
1096-
fclear.write('rm ' + shlex.quote(path_part) + '/*\n')
1125+
# FIX: Safer wildcard removal.
1126+
# Avoid quoting the wildcard itself ('path/*'). Instead cd into directory and remove contents.
1127+
fclear.write(f'cd {shlex.quote(path_part)} && rm -f *\n')
10971128
writeedges = writeedges[writeedges.find(":")+1:]
10981129
i=i+1
10991130
fclear.close()

0 commit comments

Comments
 (0)