🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution vulnerability in AST type validation#386
🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution vulnerability in AST type validation#386bashandbone wants to merge 1 commit into
Conversation
Added strict whitelisting of function calls within the TypeValidator to prevent Arbitrary Code Execution (ACE) vulnerabilities during dependency injection type evaluation. Added corresponding documentation in the sentinel journal. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideTightens AST-based validation in the DI container’s Flow diagram for tightened AST type validation in _safe_eval_typeflowchart TD
A[type_str input] --> B[ast.parse]
B --> C[TypeValidator.generic_visit]
C --> D{node is ast.Call?}
D -- no --> G[continue validation]
D -- yes --> E[extract func_name]
E --> F{func_name in allowed_calls?}
F -- no --> H[raise TypeError]
F -- yes --> G[continue validation]
G --> I[eval type_str with restricted globalns]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 Hi @bashandbone, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @bashandbone, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider extracting the
allowed_callsset into a module-level constant (or sharing it with any related DI/type-handling modules) so it’s easier to keep the whitelist in sync as the dependency API evolves. - In the
ast.Callhandling, you might want to treat unresolvable/complexnode.funcvalues (wherefunc_nameends upNone) explicitly as forbidden with a clearer error message so they can’t slip through and are easier to debug.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the `allowed_calls` set into a module-level constant (or sharing it with any related DI/type-handling modules) so it’s easier to keep the whitelist in sync as the dependency API evolves.
- In the `ast.Call` handling, you might want to treat unresolvable/complex `node.func` values (where `func_name` ends up `None`) explicitly as forbidden with a clearer error message so they can’t slip through and are easier to debug.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🚨 Severity: CRITICAL
💡 Vulnerability: Unrestricted dynamic type resolution
The
_safe_eval_typefunction insrc/codeweaver/core/di/container.pyevaluated type annotation strings using python'seval()function in a supposedly restricted environment with a filtered dictionary of__builtins__. While an AST validator ensured only a safe subset of nodes were permitted, it mistakenly permitted anyast.Callnodes. This left the system vulnerable to Arbitrary Code Execution, as an attacker could supply a malicious string utilizing functions within the module's global namespace to bypass restrictions.🎯 Impact: Arbitrary Code Execution (ACE)
This could allow malicious configuration or improperly validated input bound as an annotation to execute code, which could result in compromising the host server, exposing secrets, and further lateral movement.
🔧 Fix: Introduced a Strict Whitelist
The
generic_visitoverride in the ASTTypeValidatorhas been explicitly patched. Now, anyast.Callis verified against a strict whitelist of dependencies utilized within Pydantic models or dependency injection structures within the CodeWeaver codebase (Depends,depends,Field,PrivateAttr,Tag, andParameter). If an unsupported function call is encountered, aTypeErroris raised before evaluation.✅ Verification:
mise //:formatand manually linted usinguv run ruff check.pytest tests/unit/core/di).tests/unit/usingpytest, asserting 100% successful execution without regression.PR created automatically by Jules for task 6973508759404387362 started by @bashandbone
Summary by Sourcery
Harden dynamic type evaluation in the dependency injection container to prevent arbitrary code execution via unsafe AST function calls.
Bug Fixes:
Documentation: