Fix Security Vulnerabilities: eval( ) Code Execution and Command Injection#505
Open
GiGiKoneti wants to merge 2 commits into
Open
Fix Security Vulnerabilities: eval( ) Code Execution and Command Injection#505GiGiKoneti wants to merge 2 commits into
GiGiKoneti wants to merge 2 commits into
Conversation
- 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #504
Summary of Changes
This Pull Request remediates two Critical security vulnerabilities discovered during a codebase audit. It replaces dangerous
eval()andshell=Trueusages 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_functionmethod was directly passing user input from the "Function Plot" GUI field into Python'seval(). This allowed arbitrary execution, including file writes, local file reads, and OS command execution (e.g.,__import__('os').system(...)).The Fix:
eval()with_safe_eval_expr, a custom Abstract Syntax Tree (AST) evaluator.+,-,*,/), a whitelistednumpymath function (e.g.,np.sin,np.sqrt), or a known trace variable. If the user attempts to callexec(),open(), or__import__(), the AST walker immediately rejects it with aValueError.string.replace(), which could accidentally corrupt expressions if a trace name was a substring of a math function (e.g., replacing a trace namedininsidenp.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
convertmethod constructed a bash string and executed it viasubprocess.run(command, shell=True). If a user uploaded a.schfile containing shell metacharacters (e.g.,test;whoami.sch), the shell would execute the injected commands.The Fix:
shell=Trueand migrated the subprocess call to a strict Python list (["python3", script_path, input_path]).exec()layer. Shell metacharacters are treated literally as filenames, entirely neutralizing the injection vector."python3"string withsys.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._safe_eval_exprand list-basedsubprocessexplicitly block every single attack vector while seamlessly allowing standard, benign mathematical plotting operations likev(out) + np.sqrt(v(in)).