Skip to content

fix: avoid shell injection in training helper#28776

Merged
tianleiwu merged 3 commits into
microsoft:mainfrom
tianleiwu:tlwu/20260603/icm_5
Jun 4, 2026
Merged

fix: avoid shell injection in training helper#28776
tianleiwu merged 3 commits into
microsoft:mainfrom
tianleiwu:tlwu/20260603/icm_5

Conversation

@tianleiwu

Copy link
Copy Markdown
Contributor

Description

Fix the command-injection risk in the training helper by removing shell-based execution and validating the safe subprocess path with regression tests.

Summary of Changes

Security fix: replace shell execution with argument-list execution

File Change
orttraining/tools/scripts/train.py Replace os.system(...) with subprocess.run([...], check=True) and add a small main(argv=None) entry point for safe argument forwarding.
orttraining/orttraining/test/python/test_train_script.py Add regression tests to confirm the helper does not execute shell commands on import and uses an argument list when invoking the training binary.

Motivation

The previous implementation built a single shell string from sys.argv[1:] and executed it with os.system(). That allowed shell metacharacters in user-supplied arguments to be interpreted by the shell. The updated path passes the command and arguments directly to subprocess.run, which avoids shell interpretation and preserves the original argument boundaries.

Testing

  • pytest orttraining/orttraining/test/python/test_train_script.py -q

Checklist

  • Security fix implemented
  • Regression tests added
  • No shell-based command execution remains in the helper path

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR mitigates command-injection risk in the ORT training helper script by removing shell-based command construction/execution and replacing it with safe argument-list subprocess invocation, plus adding regression tests.

Changes:

  • Replace os.system() shell execution in train.py with subprocess.run([...], check=True) behind a main(argv=None) entry point.
  • Add pytest coverage to ensure importing the script does not execute a command and that subprocess.run is called with an argument list.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
pr_description.md Documents the security motivation, implementation summary, and test command for the change.
orttraining/tools/scripts/train.py Switches from shell-string execution to subprocess.run with an argv list via a main() entry point.
orttraining/orttraining/test/python/test_train_script.py Adds regression tests for import-time behavior and argv-list subprocess invocation.

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

Comment thread orttraining/orttraining/test/python/test_train_script.py Outdated
Comment thread orttraining/tools/scripts/train.py
@tianleiwu
tianleiwu marked this pull request as ready for review June 3, 2026 23:03
@tianleiwu
tianleiwu merged commit 7f32673 into microsoft:main Jun 4, 2026
86 checks passed
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.

3 participants