Fix unsafe lambda callable resolution#6112
Conversation
github-actions
Bot
added
bug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review: Fix Unsafe Lambda Callable Resolution
This PR closes a code-execution vulnerability in string_to_callable by validating lambda expression ASTs before eval() and restricting the evaluation namespace. The defense-in-depth approach (AST allowlist + sandboxed eval) is well-designed.
Summary
| Aspect | Assessment |
|---|---|
| Security effectiveness | ✅ Strong — dual-layer protection blocks known attack vectors |
| Backward compatibility | |
| Test coverage | Good, with gaps noted |
| Code quality | Clean, well-structured |
Findings
1. ⚠️ ast.Attribute ban may break existing configurations (Medium)
Blocking all ast.Attribute nodes means expressions like lambda cfg: cfg.num_envs or lambda x: x.shape are rejected. A search of this repository shows lambda expressions with attribute access exist in the codebase (e.g., in utils/array.py). While these may be Python-level lambdas rather than string-serialized ones, any downstream user storing \"lambda x: x.value\" in a config that feeds into string_to_callable will encounter a ValueError after this change.
Suggestion: Consider a more targeted approach — block only attribute access to dunder names (where node.attr.startswith(\"__\")) rather than all attribute access. Combined with the ast.Call restriction, non-dunder attribute access is benign (you can read .foo but cannot call anything on it).
2. ⚠️ ast.Call ban prevents safe builtins like int(), float(), abs() (Medium)
Legitimate configurations such as \"lambda x: int(x)\" or \"lambda x: abs(x)\" are now rejected. While this is conservative and appropriate for untrusted input, it represents a breaking change for existing configs that relied on builtin calls inside lambda strings.
Suggestion: If the strict ban is intentional, document it clearly in the string_to_callable docstring with examples of what is/isn't allowed. Alternatively, consider an allowlist of safe builtin names that could be injected into the eval namespace.
3. 🔍 Implicit protocol methods bypass the AST check (Low / Informational)
Operations like subscript (x[0]), iteration ([i for i in x]), and arithmetic (x + y) invoke dunder protocol methods (__getitem__, __iter__, __add__) at runtime. These cannot be used for code execution because:
- The lambda's
__globals__retains{\"__builtins__\": {}}, so no builtins are available at call time - Without
ast.Call, there's no way to invoke arbitrary functions
This is acceptable — noting for completeness that the security boundary relies on both layers working together.
4. 🟢 is_lambda_expression improvement is correct (Positive)
Switching to mode=\"eval\" and checking tree.body directly is cleaner and avoids triple-parsing the same string. The previous implementation called ast.parse(name) three times.
5. 📝 Missing docstring update for is_lambda_expression
The function docstring says "Checks if the input string is a lambda expression" but doesn't document that the function now uses mode=\"eval\" for stricter single-expression parsing. A brief note would help future maintainers understand why mode=\"eval\" was chosen over the default mode=\"exec\".
6. 📝 Test gap: multi-parameter and default-argument lambdas
The safe-lambda tests cover single-parameter cases. Consider adding:
assert string_utils.string_to_callable("lambda x, y: x + y")(3, 4) == 7
assert string_utils.string_to_callable("lambda x=5: x * 2")() == 10These verify that the AST validation doesn't accidentally reject valid lambda signatures.
Security Verification
I verified the following attack vectors are properly blocked:
| Attack | Mechanism | Blocked By |
|---|---|---|
__import__(\"os\").system(\"id\") |
Call + Attribute | AST validation |
eval(\"malicious\") |
Call | AST validation |
(lambda: payload)() |
Call (nested invocation) | AST validation |
x.__class__.__subclasses__() |
Attribute + Call | AST validation |
__builtins__ reference |
Dunder Name check | AST validation |
| Walrus operator side effects | NamedExpr | AST validation |
globals() / locals() |
Even if AST check bypassed | Empty __builtins__ |
The eval(name, {\"__builtins__\": {}}, {}) sandbox correctly persists into the lambda's __globals__, so even at call time, the lambda cannot access builtins. This is the correct behavior and provides a strong second layer.
Verdict
Solid security fix with good test coverage. The main concern is backward compatibility of the ast.Attribute and ast.Call bans. If this is deployed to an existing Isaac Lab installation where configs use lambdas with attribute access or builtin calls, it will be a breaking change. Consider documenting the allowed lambda syntax or relaxing the attribute restriction to dunder-only.
| String <-> Callable operations. | ||
| """ | ||
|
|
||
| _FORBIDDEN_LAMBDA_NODES = (ast.Call, ast.Attribute, ast.NamedExpr) |
There was a problem hiding this comment.
Consider documenting what each forbidden node type prevents:
ast.Call- prevents function calls like__import__("os")ast.Attribute- prevents attribute traversal likex.__class__ast.NamedExpr- prevents walrus operator side effects
This helps future maintainers understand why these restrictions exist.
| if isinstance(node, _FORBIDDEN_LAMBDA_NODES): | ||
| raise ValueError(f"Unsafe lambda expression '{name}': disallowed syntax '{type(node).__name__}'.") | ||
| if isinstance(node, ast.Name) and node.id.startswith("__"): | ||
| raise ValueError(f"Unsafe lambda expression '{name}': dunder name '{node.id}' is not allowed.") |
There was a problem hiding this comment.
Nice catch on dunder names! Consider also checking for names like globals, locals, exec, compile which could be problematic if somehow injected into the eval namespace in the future.
| if name_is_lambda: | ||
| callable_object = eval(name, {"__builtins__": {}}, {}) | ||
| else: | ||
| mod_name, attr_name = name.rsplit(separator, 1) |
There was a problem hiding this comment.
Good practice passing empty dicts for both globals and locals. One consideration: if a user needs to reference imported modules in their lambda (e.g., lambda x: math.sin(x)), this will now fail. Is there a plan for a "safe" allowlist of modules, or should users import the callable directly via the module:function syntax instead?
|
|
||
| def test_string_to_callable_allows_safe_lambdas(): | ||
| """Test that simple lambda expressions and module references resolve to callables.""" | ||
| assert string_utils.string_to_callable("lambda x: x + 1")(5) == 6 |
There was a problem hiding this comment.
Great test case with subscript and boolean operators! Consider adding a test for a no-argument lambda: lambda: 42 to ensure it works with the new validation.
Greptile SummaryThis PR hardens
Confidence Score: 3/5The two-layer defence is sound for the documented attack vectors, but f-string nodes slip through the validator and can trigger arbitrary repr/format calls on arguments at invocation time. The core logic is correct and the test suite covers the known payloads well. However, ast.JoinedStr (f-string) is not listed in _FORBIDDEN_LAMBDA_NODES, so lambda x: f"{x!r}" passes validation and eval today, then implicitly calls repr(x) when the lambda is invoked. Because the fix is specifically presented as a security boundary against malicious lambda strings, leaving this implicit-call vector unaddressed is a meaningful gap in the guard. source/isaaclab/isaaclab/utils/string.py — specifically the _FORBIDDEN_LAMBDA_NODES tuple and the surrounding validator logic.
|
| Filename | Overview |
|---|---|
| source/isaaclab/isaaclab/utils/string.py | Adds AST-based lambda validation and restricted eval; f-strings (ast.JoinedStr) are not blocked, allowing implicit format/repr calls on arguments to bypass the "no function calls" guard. |
| source/isaaclab/test/utils/test_string.py | Adds safe-lambda and unsafe-payload tests; coverage is good for the documented attack vectors but lacks f-string cases (e.g., lambda x: f"{x!r}") that currently pass validation. |
| source/isaaclab/changelog.d/fix-safe-lambda-callables.rst | New changelog entry accurately describing the lambda validation fix; no issues. |
Comments Outside Diff (1)
-
source/isaaclab/isaaclab/utils/string.py, line 183-189 (link)Validation
ValueErrorcould be silently rewrapped — theexcept (ValueError, ModuleNotFoundError)handler wraps anyValueErrorraised inside thetryblock into a generic "Could not resolve…" message. The validation call is outsidetryso its original message survives today, but a future refactor that moves_validate_lambda_expressioninside thetrywould silently replace "Unsafe lambda expression …" with the generic message and break thepytest.raises(ValueError, match="Unsafe lambda expression")assertions in the test suite. Consider catching onlyModuleNotFoundErrorhere, or explicitly re-raisingValueErrorfrom validation.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Fix unsafe lambda callable resolution" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| String <-> Callable operations. | ||
| """ | ||
|
|
||
| _FORBIDDEN_LAMBDA_NODES = (ast.Call, ast.Attribute, ast.NamedExpr) |
There was a problem hiding this comment.
ast.JoinedStr) are not in _FORBIDDEN_LAMBDA_NODES, so lambda x: f"{x}" and lambda x: f"{x!r}" both pass AST validation. When the resulting callable is invoked, f"{x}" implicitly triggers type(x).__format__(x, '') and f"{x!r}" triggers type(x).__repr__(x) — neither produces an ast.Call in the AST but both call arbitrary methods on the argument. For objects with side-effecting __repr__/__format__, a crafted lambda string can trigger code at call time. Adding ast.JoinedStr to _FORBIDDEN_LAMBDA_NODES closes this gap without affecting any of the safe lambda patterns currently in the test suite.
| _FORBIDDEN_LAMBDA_NODES = (ast.Call, ast.Attribute, ast.NamedExpr) | |
| _FORBIDDEN_LAMBDA_NODES = (ast.Call, ast.Attribute, ast.NamedExpr, ast.JoinedStr) |
| def _validate_lambda_expression(name: str) -> None: | ||
| """Validate that a lambda expression string cannot execute arbitrary code.""" | ||
| tree = ast.parse(name, mode="eval") | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, _FORBIDDEN_LAMBDA_NODES): | ||
| raise ValueError(f"Unsafe lambda expression '{name}': disallowed syntax '{type(node).__name__}'.") | ||
| if isinstance(node, ast.Name) and node.id.startswith("__"): | ||
| raise ValueError(f"Unsafe lambda expression '{name}': dunder name '{node.id}' is not allowed.") |
There was a problem hiding this comment.
is_lambda_expression calls ast.parse(name, mode="eval") and _validate_lambda_expression calls it again immediately after. For hot paths that call string_to_callable frequently (e.g., config reload loops), this doubles parse overhead. Threading the already-parsed tree into the validator — or caching it — would eliminate the redundant work.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| String <-> Callable operations. | ||
| """ | ||
|
|
||
| _FORBIDDEN_LAMBDA_NODES = (ast.Call, ast.Attribute, ast.NamedExpr) |
There was a problem hiding this comment.
Consider narrowing the ast.Attribute restriction. Currently, this blocks all attribute access including benign patterns like lambda cfg: cfg.enabled. A targeted check on node.attr.startswith("__") within _validate_lambda_expression would block dunder traversal (the actual security concern) while preserving legitimate attribute access.
Suggested alternative:
_FORBIDDEN_LAMBDA_NODES = (ast.Call, ast.NamedExpr)
# Then in _validate_lambda_expression:
if isinstance(node, ast.Attribute) and node.attr.startswith("__"):
raise ValueError(...)This maintains security (you cannot chain .__class__.__subclasses__() because ast.Call is still blocked) while allowing safe attribute reads.
| if name_is_lambda: | ||
| _validate_lambda_expression(name) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Good: the empty __builtins__ persists into the lambda's __globals__, so builtins remain unavailable even when the lambda is later invoked. This is correct defense-in-depth.
Note: the empty locals {} means the lambda cannot reference any module-level names. If you ever want to allow a curated set of safe builtins (e.g., int, float, abs, len), you could inject them here:
_SAFE_EVAL_GLOBALS = {"__builtins__": {}, "int": int, "float": float, "abs": abs}
callable_object = eval(name, _SAFE_EVAL_GLOBALS, {})This is optional — the current strict approach is safer for untrusted input.
|
|
||
|
|
||
| def test_resolve_matching_names_with_basic_strings(): | ||
| """Test resolving matching names with a basic expression.""" |
There was a problem hiding this comment.
Good coverage of attack vectors. Consider adding a few more cases:
# Multi-param lambdas (verify they pass validation)
assert string_utils.string_to_callable("lambda x, y: x + y")(3, 4) == 7
# Lambda with default args
assert string_utils.string_to_callable("lambda x=5: x * 2")() == 10
# Additional bypass attempts to add to the parametrize list:
"lambda x: [i for i in x.__dict__]", # Attribute in comprehensionThe comprehension case is already blocked by ast.Attribute, but an explicit test documents the security boundary.
## Summary - Backport #6112 to `release/3.0.0-beta2`. - Validate lambda strings before resolving them with `string_to_callable`. - Evaluate supported lambda expressions without Python builtins. - Add focused tests for safe lambdas and unsafe payloads. ## Cherry-pick - Cherry-picked `ffe85a4ccc9521cae079bac1c33f7e632e6da26e` with `-x`. ## Testing - `pre-commit run --files source/isaaclab/changelog.d/fix-safe-lambda-callables.rst source/isaaclab/isaaclab/utils/string.py source/isaaclab/test/utils/test_string.py` - `PYTHONPATH=/home/zhengyuz/Projects/IsaacLab.wt/beta2-safe-lambda-string-to-callable/source/isaaclab /home/zhengyuz/Projects/IsaacLab.wt/beta2-automate-collision-stack/.venv/bin/python -m pytest source/isaaclab/test/utils/test_string.py source/isaaclab/test/utils/test_dict.py -q` (26 passed) - `/home/zhengyuz/Projects/IsaacLab.wt/beta2-automate-collision-stack/.venv/bin/python tools/changelog/cli.py check release/3.0.0-beta2`
2 participants
Summary
Testing