Skip to content

Fix harmful shell injection vulnerability in mkconcore.py#212

Open
GREENRAT-K405 wants to merge 3 commits intoControlCore-Project:devfrom
GREENRAT-K405:fix/shell-injection
Open

Fix harmful shell injection vulnerability in mkconcore.py#212
GREENRAT-K405 wants to merge 3 commits intoControlCore-Project:devfrom
GREENRAT-K405:fix/shell-injection

Conversation

@GREENRAT-K405
Copy link

@GREENRAT-K405 GREENRAT-K405 commented Feb 5, 2026

Changes made:

  • POSIX/docker: Implemented shlex.quote() for all user-controlled inputs in generated shell scripts.
  • for windows: Applied strict double-quoting ("{var}") to prevents argument splitting and basic injection attacks in generated .bat files. (since shlex doesn't work for windows, this had to be done manually)
  • Introduced a safe_name() helper function that strictly validates all node and edge labels from the GraphML file at ingestion time.

Impacts:

  • This should prevent path traversal characters (/, ) and shell metacharacters (&, |, ;, $, etc.).
  • This prevents malicious GraphML files from writing outside the output directory or executing arbitrary commands during build/run time

This fixes #194

Copilot AI review requested due to automatic review settings February 5, 2026 19:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses shell injection vulnerabilities in mkconcore.py by adding input validation and shell escaping mechanisms. The PR introduces a safe_name() validation function to block dangerous characters at GraphML ingestion time, applies shlex.quote() for POSIX shell escaping, and implements double-quoting for Windows batch files.

Changes:

  • Added input validation via safe_name() function with regex-based character blocking
  • Implemented shlex.quote() for user-controlled inputs in POSIX/Docker shell scripts
  • Applied double-quoting for Windows batch file generation to prevent argument splitting
Comments suppressed due to low confidence (14)

mkconcore.py:180

    funlock = open("unlock", "w") # 12/4/21

mkconcore.py:178

    fclear = open("clear", "w") 

mkconcore.py:168

    fclear = open("clear.bat", "w") 

mkconcore.py:167

    fstop = open("stop.bat", "w")  #3/27/21

mkconcore.py:176

    fdebug = open("debug", "w")

mkconcore.py:169

    fmaxtime = open("maxtime.bat", "w") # 9/12/21

mkconcore.py:179

    fmaxtime = open("maxtime", "w") # 9/12/21

mkconcore.py:170

    funlock = open("unlock.bat", "w") # 12/4/21

mkconcore.py:181

    fparams = open("params", "w") # 9/18/22

mkconcore.py:165

    frun = open("run.bat", "w")

mkconcore.py:177

    fstop = open("stop", "w")  #3/27/21

mkconcore.py:166

    fdebug = open("debug.bat", "w")

mkconcore.py:171

    fparams = open("params.bat", "w") # 9/18/22

mkconcore.py:175

    frun = open("run", "w")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + containername + '\\\\\\"; ./' + sourcecode + ' ' + MCRPATH + '\\"" \n')
fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + safe_container + '\\\\\\"; ./' + safe_source + ' ' + MCRPATH + '\\"" \n')

elif langext == "m": #3/23/21
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential command injection in eval command construction: The safe_eval_cmd is constructed as shlex.quote(f"run('{sourcecode}')"), which means if sourcecode contains a single quote, the resulting MATLAB/Octave command could be malformed or exploitable. For example, if sourcecode is "test'); system('evil", the eval command becomes "run('test'); system('evil')". While shlex.quote() will escape the outer shell layer, the inner MATLAB/Octave eval still executes the command. Consider validating that sourcecode doesn't contain quotes, or use a safer parameter passing mechanism.

Suggested change
elif langext == "m": #3/23/21
elif langext == "m": #3/23/21
# validate MATLAB/Octave source filename to prevent injection in run('...'):
# allow only common filename characters (no quotes, semicolons, or whitespace)
if not re.match(r'^[A-Za-z0-9_./\\-]+$', sourcecode):
raise ValueError("Invalid MATLAB source file name: {!r}".format(sourcecode))

Copilot uses AI. Check for mistakes.
mkconcore.py Outdated
Comment on lines 84 to 85
#blocks path traversal (/, \\) and shell metacharacters (*, ?, <, >, |, ;, &, $, `)
if re.search(r'[\\/:*?"<>|;&`$]', value):
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete validation regex: While newlines are replaced before validation (line 217), the regex pattern doesn't block all potentially dangerous characters. Consider adding:

  1. Null byte (\x00) which could truncate paths in C-based programs
  2. Other control characters (tabs are replaced, but other control chars are not)
  3. Parentheses which could be problematic in some shell contexts
    Note that single quotes are missing from the regex (see separate comment), which is the most critical omission.
Suggested change
#blocks path traversal (/, \\) and shell metacharacters (*, ?, <, >, |, ;, &, $, `)
if re.search(r'[\\/:*?"<>|;&`$]', value):
# blocks path traversal (/, \\), control chars, and shell metacharacters
if re.search(r'[\x00-\x1F\x7F\\/:*?"<>|;&`$\'()]', value):

Copilot uses AI. Check for mistakes.
mkconcore.py Outdated
if ubuntu:
fdebug.write('concorewd="$(pwd)"\n')
fdebug.write('xterm -e bash -c "cd \\"$concorewd/' + containername + '\\"; ./' + sourcecode + ' ' + MCRPATH + '; bash" &\n')
fdebug.write('xterm -e bash -c "cd \\"$concorewd/' + safe_container + '\\"; ./' + safe_source + ' ' + MCRPATH + '; bash" &\n')
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell injection vulnerability: MCRPATH is used without escaping in the xterm command. This creates the same injection risk as in the frun.write() call above.

Copilot uses AI. Check for mistakes.
mkconcore.py Outdated
else:
fdebug.write('concorewd="$(pwd)"\n')
fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + containername + '\\\\\\"; ./' + sourcecode + ' ' + MCRPATH + '\\"" \n')
fdebug.write('osascript -e "tell application \\"Terminal\\" to do script \\"cd \\\\\\"$concorewd/' + safe_container + '\\\\\\"; ./' + safe_source + ' ' + MCRPATH + '\\"" \n')
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell injection vulnerability: MCRPATH is used without escaping in the osascript command for macOS.

Copilot uses AI. Check for mistakes.
mkconcore.py Outdated
writeedges = volswr[i]
while writeedges.find(":") != -1:
fclear.write(DOCKEREXE+' volume rm ' +writeedges.split(":")[0].split("-v")[1]+"\n")
fclear.write(DOCKEREXE+' volume rm ' +writeedges.split(":")[0].split("-v")[1].strip()+"\n") # Added strip()
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete shell escaping: While .strip() was added to clean whitespace, the volume path extracted from volswr (which contains validated edge labels) is not escaped with shlex.quote(). Although edge labels are validated by safe_name(), defense-in-depth suggests these should still be escaped when used in shell commands.

Copilot uses AI. Check for mistakes.
safe_name(container_part, f"Node container name '{container_part}'")
safe_name(source_part, f"Node source file '{source_part}'")
else:
safe_name(node_label, f"Node label '{node_label}'")
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input validation inconsistency: The validation logic allows node labels without colons (line 225), but all subsequent code expects node labels to follow the "container:source" format and calls split(':') without error handling. This will cause a ValueError crash if a GraphML file contains a node label without a colon. Either require colons in the validation (reject labels without colons), or handle the ValueError in all the places that use split(':').

Suggested change
safe_name(node_label, f"Node label '{node_label}'")
raise ValueError(
f"Invalid node label '{node_label}': expected format 'container:source' with a ':' separator."
)

Copilot uses AI. Check for mistakes.
mkconcore.py Outdated
fdebug.write(DOCKEREXE+' run -it --name='+containername+volswr[i]+volsro[i]+" docker-"+dockername+"&\n")
# safe_container added to debug line (POSIX)
safe_container = shlex.quote(containername)
fdebug.write(DOCKEREXE+' run -it --name='+safe_container+volswr[i]+volsro[i]+" docker-"+dockername+"&\n")
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete escaping for Docker debug script: While safe_container is properly defined with shlex.quote(), the dockername variable (derived from sourcecode.split(".")[0]) is used directly without escaping in the docker run command. Although dockername comes from a validated source, it should be escaped with shlex.quote() for consistency and defense-in-depth.

Suggested change
fdebug.write(DOCKEREXE+' run -it --name='+safe_container+volswr[i]+volsro[i]+" docker-"+dockername+"&\n")
# Quote the full Docker image name for shell safety
safe_image = shlex.quote("docker-" + dockername)
fdebug.write(DOCKEREXE+' run -it --name='+safe_container+volswr[i]+volsro[i]+" "+safe_image+"&\n")

Copilot uses AI. Check for mistakes.
Comment on lines 216 to +217
node_label = prefixedgenode + node_label_tag.text
nodes_dict[node['id']] = re.sub(r'(\s+|\n)', ' ', node_label)
node_label = re.sub(r'(\s+|\n)', ' ', node_label)
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command-line argument used in label prefix without validation: The outdir argument from sys.argv[3] is concatenated into node and edge labels via prefixedgenode without validation. While command-line arguments are typically trusted, this creates an additional injection vector if the script is called by another program with untrusted input. Consider validating outdir with safe_name() or a similar function to ensure it doesn't contain shell metacharacters.

Copilot uses AI. Check for mistakes.
mkconcore.py Outdated
while writeedges.find(":") != -1:
funlock.write('sudo docker cp ~/concore.apikey concore:/')
funlock.write(writeedges.split(":")[0].split("-v ")[1]+"/concore.apikey\n")
funlock.write(writeedges.split(":")[0].split("-v ")[1].strip()+"/concore.apikey\n")
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete shell escaping: Volume path is not escaped with shlex.quote() before being used in the docker cp command.

Suggested change
funlock.write(writeedges.split(":")[0].split("-v ")[1].strip()+"/concore.apikey\n")
volume_path = writeedges.split(":")[0].split("-v ")[1].strip()
safe_dest_path = shlex.quote(volume_path + "/concore.apikey")
funlock.write(safe_dest_path + "\n")

Copilot uses AI. Check for mistakes.
# print(DOCKEREXE+' run --name='+containername+volswr[i]+volsro[i]+" docker-"+dockername+' octave -qf --eval "run('+"'"+sourcecode+"'"+')"'+"&\n")
# frun.write(DOCKEREXE+' run --name='+containername+volswr[i]+volsro[i]+" docker-"+dockername+' octave -qf --eval "run('+"'"+sourcecode+"'"+')"'+"&\n")
# Use safe_container
frun.write(DOCKEREXE+' run --name='+safe_container+volswr[i]+volsro[i]+" docker-"+shlex.quote(dockername)+"&\n")
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell injection vulnerability: Same issue as line 695 - volswr[i] and volsro[i] are concatenated without proper escaping.

Copilot uses AI. Check for mistakes.
@GREENRAT-K405
Copy link
Author

GREENRAT-K405 commented Feb 5, 2026

This should fix it! (I have considered some essential github copilot suggestions and added them)

@GREENRAT-K405
Copy link
Author

@pradeeban, I have fixed the most essential shell injection vulnerabilities, I have tried to keep the core functionality unchanged.

Please do let me know if I am missing out on something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant