Skip to content

Fix Security Vulnerabilities: eval( ) Code Execution and Command Injection#505

Open
GiGiKoneti wants to merge 2 commits into
FOSSEE:masterfrom
GiGiKoneti:fix/security-p0-eval-and-shell-injection
Open

Fix Security Vulnerabilities: eval( ) Code Execution and Command Injection#505
GiGiKoneti wants to merge 2 commits into
FOSSEE:masterfrom
GiGiKoneti:fix/security-p0-eval-and-shell-injection

Conversation

@GiGiKoneti
Copy link
Copy Markdown
Contributor

Resolves #504

Summary of Changes

This Pull Request remediates two Critical security vulnerabilities discovered during a codebase audit. It replaces dangerous eval() and shell=True usages with strict, production-ready, and edge-case-proof alternatives.


Detailed Implementation & Rationale

1. Arbitrary Code Execution via eval() (src/ngspiceSimulation/plot_window.py)

The Problem: The plot_function method was directly passing user input from the "Function Plot" GUI field into Python's eval(). This allowed arbitrary execution, including file writes, local file reads, and OS command execution (e.g., __import__('os').system(...)).

The Fix:

  • AST-Based Safe Parser: Replaced eval() with _safe_eval_expr, a custom Abstract Syntax Tree (AST) evaluator.
  • Why this implementation? Instead of trying to "blacklist" bad strings (which is always bypassable), this uses an explicit "whitelist" approach. It parses the math expression into an AST and strictly enforces that every node is either a basic arithmetic operator (+, -, *, /), a whitelisted numpy math function (e.g., np.sin, np.sqrt), or a known trace variable. If the user attempts to call exec(), open(), or __import__(), the AST walker immediately rejects it with a ValueError.
  • Safe Token Replacement: Previously, the trace replacement logic used a naive string.replace(), which could accidentally corrupt expressions if a trace name was a substring of a math function (e.g., replacing a trace named in inside np.sin()). This PR upgrades the replacement logic to use strict Regular Expressions (re.sub) with lookaheads/lookbehinds to guarantee that trace variables are only substituted when they appear as exact, independent tokens.

2. Command Injection via shell=True (src/converter/pspiceToKicad.py)

The Problem: The convert method constructed a bash string and executed it via subprocess.run(command, shell=True). If a user uploaded a .sch file containing shell metacharacters (e.g., test;whoami.sch), the shell would execute the injected commands.

The Fix:

  • List-Based Execution: Removed shell=True and migrated the subprocess call to a strict Python list (["python3", script_path, input_path]).
  • Why this implementation? By avoiding the system shell entirely, Python passes the arguments directly to the OS exec() layer. Shell metacharacters are treated literally as filenames, entirely neutralizing the injection vector.
  • Environment Integrity: Replaced the hardcoded "python3" string with sys.executable. This ensures that the subprocess spawns using the exact same Python binary and virtual environment that the PyQt GUI is currently running in, preventing frustrating "Module Not Found" crashes for users with complex conda or venv setups.

Testing Performed

Included in this PR is a standalone, 26-test security suite (tests/test_security_p0.py) that strictly enforces these fixes.

  • Exploit Proof-of-Concepts: The first half of the suite successfully executes live exploits (shell command injections and canary file reads) against the vulnerable patterns to prove the severity of the flaw.
  • Regression & Patch Efficacy: The second half of the suite proves that the new _safe_eval_expr and list-based subprocess explicitly block every single attack vector while seamlessly allowing standard, benign mathematical plotting operations like v(out) + np.sqrt(v(in)).
  • Manual Verification: Verified in the PyQt6 GUI that the "Function Plot" tool and "Pspice to KiCad Converter" execute flawlessly.

- plot_window.py: Replace dangerous eval() with safe AST-based math expression parser to prevent arbitrary code execution.

- pspiceToKicad.py: Remove shell=True from subprocess.run() and use list format to prevent command injection.

- tests/test_security_p0.py: Add comprehensive PoC and regression test suite proving exploitation and patch efficacy.
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.

CRITICAL : Arbitrary Code Execution via eval( ) and Command Injection via shell=True

1 participant