fix(math): restrict sympy expression parsing#200
fix(math): restrict sympy expression parsing#200Sebastion (sebastiondev) wants to merge 2 commits into
Conversation
sympy parse_expr uses Python code execution internally and without restrictions on local_dict/global_dict allows arbitrary code execution through crafted math expression strings. Add safe_parse_expr wrapper that: - Validates input against a denylist of dangerous patterns (dunder attrs, import, exec, os, subprocess, etc.) - Restricts the namespace to only known-safe sympy objects (math functions, constants, and common symbolic variables) - Strips Python builtins from the global namespace - Enforces a maximum expression length Replace all bare parse_expr calls across the codebase: - src/qwed_new/api/main.py (/verify/math endpoint) - src/qwed_new/core/verifier.py (VerificationEngine) - src/qwed_new/core/batch.py (batch math verification) - src/qwed_new/core/validator.py (SemanticValidator) Signed-off-by: Sebastion <sebastion@sebastion.dev>
Greptile SummaryThis PR fixes a CWE-95 (eval injection) vulnerability where user-controlled math expression strings were passed directly to SymPy's
Confidence Score: 5/5This PR is safe to merge. It replaces every direct parse_expr() call site with the new sandboxed wrapper and adds no new execution paths or trust boundaries. All four previously vulnerable call sites (main.py, verifier.py, batch.py, validator.py) are consistently patched. The wrapper's three-layer defence (denylist → stripped builtins → allow-listed namespace) is correctly implemented and independently tested with 36 targeted security cases. Previous concerns about multi-letter symbols and shared global-dict mutation are both resolved in this version. No regressions in the exception-handling path were introduced, and the test for API error sanitisation still passes with the updated patch target. No files require special attention. The new safe_parser.py module is the core of the change and is well-tested. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant API as POST /verify/math
participant SP as safe_parse_expr()
participant DL as Denylist check
participant LD as _build_safe_local_dict()
participant PE as parse_expr() (SymPy)
Client->>API: expression (user input)
API->>SP: safe_parse_expr(expression)
SP->>SP: isinstance / empty / length checks
SP->>DL: _DANGEROUS_PATTERNS.search(stripped)
alt Dangerous pattern found
DL-->>SP: match
SP-->>API: ValueError("disallowed constructs")
API-->>Client: "status=ERROR, sanitised message"
else Safe input
DL-->>SP: no match
SP->>LD: build allow-listed local_dict
LD-->>SP: "{x,y,sin,cos,...,Greek letters}"
SP->>PE: "parse_expr(expr, local_dict=..., global_dict=dict(SAFE_GLOBAL))"
PE-->>SP: sympy.Basic expression
SP-->>API: sympy expression
API-->>Client: verification result
end
Reviews (2): Last reviewed commit: "fix: address review — add Greek symbols,..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thanks for the report, Sebastion (@sebastiondev). We independently audited the codebase against your claims and verified the vulnerability. Vulnerability Status: ConfirmedWe reproduced the exploit on our current SymPy 1.14.0 installation using a from sympy.parsing.sympy_parser import parse_expr
parse_expr('__import__(chr(111)+chr(115)).system(chr(105)+chr(100))')
# Executes os.system("id") on the hostAll 17 production The fix direction is correct and aligns with QWED's fail-closed philosophy. However, we need the following changes before we can merge: Required Changes1. Multi-letter symbolic variable support
2. Shared mutable The module-level dict is passed by reference to every 3. Validate
If you're able to address these, we're happy to re-review. If not, we may implement the fix internally based on your approach — the vulnerability is real and we want to close it promptly. Either way, thank you for the responsible report. |
Merging this PR will improve performance by 65%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_bench_math_algebraic_expression |
1.8 ms | 1.1 ms | +67.3% |
| ⚡ | test_bench_math_simple_arithmetic |
1.8 ms | 1.1 ms | +62.72% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sebastiondev:fix/cwe95-main-sympy-9383 (bbf9ade) with main (06801f6)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…alidate variable names
📝 WalkthroughWalkthroughThis PR hardens math expression parsing across the verification system by introducing a denylist-based safe parser that replaces SymPy's unrestricted ChangesSafe Expression Parser Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you Rahul Dass (@rahuldass19) for the thorough review and for independently confirming the vulnerability — really appreciated. All three requested changes have been addressed in bbf9ade: 1. Multi-letter symbolic variable support ✅
2. Shared mutable
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| expr = safe_parse_expr(expression) | ||
| validate_variable_name(variable) | ||
| var = Symbol(variable) |
There was a problem hiding this comment.
Bug: When verifying calculus operations, using "n" as the variable causes a symbol mismatch, leading to incorrect results like 0 for derivatives.
Severity: MEDIUM
Suggested Fix
When creating the variable symbol in the verifier (e.g., var = Symbol(variable)), check if the variable string matches a key in the _build_safe_local_dict() from safe_parser. If it does, reuse the existing symbol object from that dictionary instead of creating a new one. This ensures the symbol used for the operation is identical to the one in the parsed expression.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/qwed_new/core/verifier.py#L284-L286
Potential issue: In `verify_derivative`, `verify_integral`, and `verify_limit`, if the
variable of the operation is `"n"`, the verification will likely fail. This is because
`safe_parse_expr` parses expressions containing `"n"` into a special SymPy symbol with
assumptions (`Symbol("n", integer=True, positive=True)`). However, the verifier creates
a plain `Symbol("n")` for the operation variable. Since SymPy treats these two symbols
as distinct, calculus operations like `diff` incorrectly return 0, as the
differentiation variable is not found in the expression. This causes valid claims like
`diff(x**n, n) = x**n * log(x)` to be incorrectly rejected.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/security/test_safe_parser.py (1)
126-138: ⚡ Quick winStrengthen the global dict isolation test.
The current test only verifies that multiple calls succeed without exceptions, but it doesn't validate the actual isolation mechanism. It doesn't prove that copying
global_dictper call prevents cross-contamination of SymPy transformations or symbols.Consider testing a scenario where state leakage would actually occur if the dict were shared:
- Parse an expression with
extra_symbolscontaining a custom symbol- Parse a second expression referencing that symbol name without passing
extra_symbols- Verify the second parse fails (proving the symbol didn't leak)
Alternatively, if such leakage is difficult to trigger in practice, document why the current test provides sufficient coverage.
♻️ Example strengthened test
def test_global_dict_not_shared_between_calls(self) -> None: - """Parse two different expressions and confirm no cross-contamination.""" - safe_parse_expr("x + 1") - safe_parse_expr("alpha + beta") - # If global_dict were shared mutably, SymPy transformations could - # leak symbols from one call into another. The shallow-copy fix - # prevents this. We just verify no exception is raised and both - # parse independently. - result = safe_parse_expr("y + 2") - assert str(result) == "y + 2" + """Verify that extra_symbols from one call don't leak into another.""" + from sympy import Symbol + + # First call with a custom symbol + safe_parse_expr("custom_var + 1", extra_symbols={"custom_var": Symbol("custom_var")}) + + # Second call should NOT have access to custom_var + with pytest.raises(ValueError): + safe_parse_expr("custom_var + 2") # Should fail: custom_var not in default namespace + + # Verify normal expressions still work + result = safe_parse_expr("x + 2") + assert str(result) == "x + 2"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/test_safe_parser.py` around lines 126 - 138, Update the test for global dict isolation to actively demonstrate leakage wouldn't occur: call safe_parse_expr once with extra_symbols including a custom symbol (e.g., safe_parse_expr("leak + 1", extra_symbols={"leak": Symbol("leak")})) and assert that parsing that expression succeeds and produces the expected result, then call safe_parse_expr again for the same symbol name without providing extra_symbols and assert that this second call raises the appropriate error (e.g., NameError or SympifyError) or does not return a Symbol — this proves that the per-call copy of _SAFE_GLOBAL_DICT prevents the first call's symbol from leaking into subsequent calls; update TestSafeParseExprGlobalDictIsolation.test_global_dict_not_shared_between_calls accordingly, keeping references to safe_parse_expr and _SAFE_GLOBAL_DICT to locate the implementation if needed.src/qwed_new/core/safe_parser.py (2)
198-203: 💤 Low valueIncluding
Symbolin local_dict is necessary but warrants documentation.
Symbolis needed because SymPy's transformations may emit code referencingSymbol(). However, this allows users to create symbols with arbitrary names via expressions likeSymbol('anything'). The denylist protects against dangerous attribute accesses on the resulting symbol, but consider adding a comment documenting this security boundary.# Sympy internal types emitted by standard_transformations "Integer": Integer, "Float": Float, "Rational": Rational, + # Symbol is required by transformations; denylist prevents dangerous + # attribute access on created symbols. "Symbol": Symbol,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qwed_new/core/safe_parser.py` around lines 198 - 203, Add a short in-source comment by the local_dict entry that maps "Symbol" (the dictionary that includes Integer, Float, Rational, Symbol used with standard_transformations) explaining why Symbol is required (SymPy transformations may emit Symbol()), the security boundary it creates (users can call Symbol('name') to create arbitrary symbol names) and that the existing denylist/attribute-access protections are relied on to mitigate risks; keep the comment concise and reference the local_dict and Symbol so future readers know this is intentional and what to review if tightening is needed.
40-66: 💤 Low valueDenylist-based filtering is a reasonable mitigation but inherently weaker than allowlist.
The pattern set covers key attack vectors (dunders, code execution primitives, system modules). However, denylist approaches can be bypassed by novel attack vectors or encoding tricks. Since SymPy's
parse_exprultimately useseval(), consider adding defense-in-depth measures in future iterations:
- AST complexity/depth limits
- Symbolic expression tree validation post-parse
For this PR, the combination of denylist + restricted namespace + stripped builtins is a substantial improvement and aligns with fail-closed philosophy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qwed_new/core/safe_parser.py` around lines 40 - 66, The denylist in _DANGEROUS_PATTERNS is useful but brittle; add defense‑in‑depth by (1) enforcing an AST complexity/depth limit on parsed expressions (e.g., run ast.parse on the input and reject if node count/depth exceeds a safe threshold) before calling SymPy's parse_expr, and (2) validating the resulting SymPy expression tree after parse_expr to ensure it contains only allowed node/symbol types (reject Function, Call, or unexpected Name nodes). Update the code paths that call parse_expr/use _DANGEROUS_PATTERNS to perform the pre-parse AST checks and the post-parse symbolic validation (leave the denylist in place as an additional filter).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/qwed_new/core/safe_parser.py`:
- Around line 198-203: Add a short in-source comment by the local_dict entry
that maps "Symbol" (the dictionary that includes Integer, Float, Rational,
Symbol used with standard_transformations) explaining why Symbol is required
(SymPy transformations may emit Symbol()), the security boundary it creates
(users can call Symbol('name') to create arbitrary symbol names) and that the
existing denylist/attribute-access protections are relied on to mitigate risks;
keep the comment concise and reference the local_dict and Symbol so future
readers know this is intentional and what to review if tightening is needed.
- Around line 40-66: The denylist in _DANGEROUS_PATTERNS is useful but brittle;
add defense‑in‑depth by (1) enforcing an AST complexity/depth limit on parsed
expressions (e.g., run ast.parse on the input and reject if node count/depth
exceeds a safe threshold) before calling SymPy's parse_expr, and (2) validating
the resulting SymPy expression tree after parse_expr to ensure it contains only
allowed node/symbol types (reject Function, Call, or unexpected Name nodes).
Update the code paths that call parse_expr/use _DANGEROUS_PATTERNS to perform
the pre-parse AST checks and the post-parse symbolic validation (leave the
denylist in place as an additional filter).
In `@tests/security/test_safe_parser.py`:
- Around line 126-138: Update the test for global dict isolation to actively
demonstrate leakage wouldn't occur: call safe_parse_expr once with extra_symbols
including a custom symbol (e.g., safe_parse_expr("leak + 1",
extra_symbols={"leak": Symbol("leak")})) and assert that parsing that expression
succeeds and produces the expected result, then call safe_parse_expr again for
the same symbol name without providing extra_symbols and assert that this second
call raises the appropriate error (e.g., NameError or SympifyError) or does not
return a Symbol — this proves that the per-call copy of _SAFE_GLOBAL_DICT
prevents the first call's symbol from leaking into subsequent calls; update
TestSafeParseExprGlobalDictIsolation.test_global_dict_not_shared_between_calls
accordingly, keeping references to safe_parse_expr and _SAFE_GLOBAL_DICT to
locate the implementation if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b93953c5-3fb4-47f0-93ea-c266f7c3684a
📒 Files selected for processing (7)
src/qwed_new/api/main.pysrc/qwed_new/core/batch.pysrc/qwed_new/core/safe_parser.pysrc/qwed_new/core/validator.pysrc/qwed_new/core/verifier.pytests/security/test_safe_parser.pytests/test_api_exceptions.py
2 participants
QWED Enforcement Checklist
eval/execusage introducedSummary
This change fixes a CWE-95 code injection issue in math expression verification. User-controlled expression strings from the
POST /verify/mathendpoint,VerificationEnginemath helpers,SemanticValidator, and math batch verification were passed directly to SymPyparse_expr(). SymPy's parser evaluates generated Python code internally, so calling it without restricted namespaces allows a tenant-supplied expression to reach Python execution primitives.The affected public endpoint is authenticated with
get_current_tenant, but that is not a sufficient mitigation for a multi-tenant API: any tenant with a valid API key can submit math verification input, while a valid API key should not grant arbitrary code execution on the service host.The fix adds
qwed_new.core.safe_parser.safe_parse_expr()and replaces the bare parser calls in the affected math paths. The wrapper rejects dangerous constructs before parsing, supplies an allow-listed local namespace containing expected SymPy math symbols/functions only, removes Python builtins from the parser globals, and enforces basic input validation including empty/non-string checks and a length limit. This preserves deterministic symbolic verification while narrowing the parser boundary to math expressions.Vulnerability details
src/qwed_new/api/main.py,verify_math, requestexpression->parse_expr()src/qwed_new/core/verifier.py,VerificationEnginemath/identity/derivative/integral/limit helpers ->parse_expr()src/qwed_new/core/batch.py, math batch itemquery->parse_expr()src/qwed_new/core/validator.py, semantic math validation ->parse_expr()Proof of Concept
The underlying unsafe behavior can be reproduced against the previous implementation with the same sink used by the affected code paths:
For the API path, a tenant with a valid key could send the malicious expression to the existing math verification route:
After this patch, the same payload is rejected by
safe_parse_expr()before SymPy evaluation. Normal expressions such as2+2,x**2 + 2*x + 1,sin(x),sqrt(16), andfactorial(5)still parse successfully.Validation
I validated the changed parser boundary and the endpoint exception path with:
Results:
tests/security/test_safe_parser.py: 36 passedtests/test_api_exceptions.py::test_verify_math_exception_handling: 1 passedI also checked for remaining direct production uses of SymPy
parse_expr()outside the new wrapper. The remainingsympy.parse_exprtext is in documentation/example text, not an active parser call.A broader local run of
tests/security/test_safe_parser.py tests/test_api_exceptions.pypassed the new security tests but exposed an unrelated Python 3.14 compatibility failure intests/test_api_exceptions.py::test_verify_stats_exception_handlingcaused byast.Numremoval while importingstats_verifier.py. That failure is outside this parser change.Security analysis
The exploit works because
parse_expr()is not just a passive math grammar parser: it transforms input and evaluates Python code. Without a constrainedglobal_dictandlocal_dict, identifiers such as__import__can resolve to Python runtime capabilities and invoke OS commands. The request body fieldexpressionand the corresponding engine/batch inputs are controlled by the caller, so the attacker controls the string that reaches the evaluation sink.This patch mitigates the issue in depth. The denylist blocks common Python execution, reflection, import, filesystem, and process-spawning constructs before parsing. The
__builtins__removal prevents fallback access to builtins during evaluation. The allow-listed local dictionary limits successful names to expected mathematical symbols, constants, and SymPy functions. The length/type checks also keep malformed or oversized inputs fail-closed instead of reaching the parser.Before submitting, I attempted to disprove the issue by checking whether authentication or routing protections would remove exploitability. The route does require
get_current_tenant, but the project exposes tenant API keys for verification workloads; that precondition does not already grant code execution. I also checked for existing input validation before the parser and did not find a sanitizer that would block the payload before it reachedparse_expr().Notes
This remains compliant with QWED's deterministic verification boundary: the patch does not add fallback execution, retries, or LLM trust. It tightens the symbolic math parser so verification continues to run through SymPy, but only with a constrained math namespace.
Submitted by Sebastion — autonomous open-source security research from Foundation Machines. Free for public repos via the Sebastion AI GitHub App.
Summary by CodeRabbit
New Features
Tests
Bug Fixes