.NET: Fix _CodeValidator os.* allow-list bypass for aliased imports (#7068)#7071
Open
eajajhossain wants to merge 1 commit into
Open
.NET: Fix _CodeValidator os.* allow-list bypass for aliased imports (#7068)#7071eajajhossain wants to merge 1 commit into
eajajhossain wants to merge 1 commit into
Conversation
github-actions
Bot
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the embedded Python _CodeValidator used by .NET LocalCodeAct so the documented os.environ / os.path-only policy is enforced even when the os module is accessed via aliases or simple re-bindings, closing a sandbox-escape class of bypasses.
Changes:
- Track names bound to the
osmodule (import os as ...,import os.path, andx = os/ annotated assigns) and enforce theosattribute allow-list against that alias set. - Update
visit_Attributeto enforce the allow-list based on alias membership rather than the literal identifieros. - Add integration tests to verify disallowed
os.*usage is blocked and permittedos.environ/os.pathusage still works (includingimport os.path as p).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.LocalCodeAct/Resources/validator.py | Implements os alias tracking (imports + assignments) and applies it to the os attribute allow-list enforcement. |
| dotnet/tests/Microsoft.Agents.AI.LocalCodeAct.UnitTests/LocalExecuteCodeFunctionIntegrationTests.cs | Adds regression coverage for blocked os.* access via aliases/rebindings and ensures allowed os.environ/os.path access remains functional. |
Comment on lines
+343
to
+354
| def visit_Assign(self, node: ast.Assign) -> None: | ||
| """Track re-bindings of the ``os`` module (e.g. ``_o = os``). | ||
|
|
||
| Assigning an existing ``os`` alias to new names must carry the | ||
| ``os`` attribute allow-list along, otherwise ``_o = os; _o.system(...)`` | ||
| would sidestep ``visit_Attribute``. | ||
| """ | ||
| if isinstance(node.value, ast.Name) and node.value.id in self._os_aliases: | ||
| for target in node.targets: | ||
| if isinstance(target, ast.Name): | ||
| self._os_aliases.add(target.id) | ||
| self.generic_visit(node) |
Comment on lines
+73
to
+76
| [InlineData("import os as x\nx.system('id')")] | ||
| [InlineData("import os\n_o = os\n_o.system('id')")] | ||
| [InlineData("import os as x\na = x\nb = a\nb.popen('id')")] | ||
| [InlineData("import os.path\nos.system('id')")] |
Author
|
@microsoft-github-policy-service agree |
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.
The code validator in
LocalCodeActis supposed to only let generated code touchos.environandos.path— everything else onosshould be blocked. But it only checked for the literal nameos, so renaming the module got right past it:import os; os.system("id") # blocked (correct)
import os as x; x.system("id") # NOT blocked (bug)
import os; _o = os; _o.system("id") # NOT blocked (bug)
Same dangerous call, just a different variable name. This is effectively a sandbox escape, and this PR closes the hole so the
os.environ/os.path-only rule actually holds.Major changes: Instead of checking for the literal name
os, the validator now tracks every name that points at theosmodule (viaimport ... as,import os.path, and plain assignments like_o = os) and checks against that set. This mirrors the approachvisit_ImportFromalready uses. Changes are invalidator.py, plus new regression tests.Impact: Aliased/renamed access to
osis now blocked just like direct access. Normal usage (os.environ,os.path, andimport os.path as p; p.join(...)) still works, and there are tests to make sure it isn't over-blocked. No public API changes — only the embedded Python script and its tests.Focus for reviewers: The edge cases - that
import os.path as pis correctly not treated as anosalias, and that assignment tracking doesn't accidentally block legitimate code.Related Issue
Fixes #7068
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) - a workflow keeps the label and title prefix in sync automatically.