-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Safe eval #8936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Safe eval #8936
Changes from 3 commits
a855124
58e721e
dd97330
f329f15
420d610
2f083a9
23f9b25
1ca759d
274c880
25d302b
3982165
456d069
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ | |
| min_version, | ||
| optional_import, | ||
| pprint_edges, | ||
| safe_eval | ||
| ) | ||
|
|
||
| validate, _ = optional_import("jsonschema", name="validate") | ||
|
|
@@ -161,7 +162,7 @@ def _get_fake_spatial_shape(shape: Sequence[str | int], p: int = 1, n: int = 1, | |
| for c in _get_var_names(i): | ||
| if c not in ["p", "n"]: | ||
| raise ValueError(f"only support variables 'p' and 'n' so far, but got: {c}.") | ||
| ret.append(eval(i, {"p": p, "n": n})) | ||
| ret.append(safe_eval(i, {"p": p, "n": n})) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is exactly the sink the GHSA advisory called out, and the safe_eval swap correctly blocks the disclosed Attribute/Subscript/Call bypass -- nice. But i here comes straight from bundle metadata (attacker-controlled per the advisory's own reachability note), and safe_eval still allows unbounded ** chains, so a shape entry like "9999999" will hang this call indefinitely. Worth bounding it here too, not just relying on safe_eval's docstring claim of being safe and secure.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bounding expressions is still going to be an ad-hoc approach, we can restrict power chains like your example but then what about |
||
| else: | ||
| raise ValueError(f"spatial shape items must be int or string, but got: {type(i)} {i}.") | ||
| return tuple(ret) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # Copyright (c) MONAI Consortium | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import ast | ||
| from typing import Any | ||
| from collections.abc import Mapping, Sequence | ||
|
|
||
| __all__ = ["SAFE_TYPES", "safe_eval"] | ||
|
|
||
| # default set of safe AST node types | ||
| SAFE_TYPES = ( | ||
| ast.Expression, | ||
| ast.Name, | ||
| ast.Load, | ||
| ast.Constant, | ||
| ast.BinOp, | ||
| ast.UnaryOp, | ||
| ast.Add, | ||
| ast.Sub, | ||
| ast.Mult, | ||
| ast.Div, | ||
| ast.FloorDiv, | ||
| ast.Pow, | ||
| ast.Mod, | ||
| ast.USub, | ||
| ast.UAdd, | ||
| ) | ||
|
|
||
|
|
||
| def safe_eval( | ||
| expr: str, | ||
| globals: Mapping[str, Any] | None = None, | ||
| locals: Mapping[str, object] | None = None, | ||
|
ericspod marked this conversation as resolved.
Outdated
|
||
| allowed_types: Sequence[type] = SAFE_TYPES, | ||
| ): | ||
| """ | ||
| Evaluate the Python expression `expr` using `eval`, but only if it is a safe expression in that its parsed AST | ||
| contains nodes whose types are given in `allowed_types`. This ensures unsafe node types are excluded, if these | ||
| are present in the AST a ValueError is raised. The default set of such types in `SAFE_TYPES` ensures only | ||
| expressions with constants and names can be evaluated, so excludes attribute access, indexing, and calls. Code | ||
| injection is infeasible through such expressions, so this is a safe and secure way of evaluating simple expressions. | ||
|
|
||
| Args: | ||
| expr: expression to evaluate, this will be stripped before parsing to avoid indentation complaints | ||
| globals: global variable mapping | ||
| locals: local variable mapping | ||
| allows_types: sequence of allowed AST types which can be found in `expr` when parsed | ||
|
|
||
| Raises: | ||
| ValueError: raised when any node in the AST parsed from `expr` has a type not in `allowed_types` | ||
|
|
||
| Returns: | ||
| The evaluated expression value, using `eval` with `globals` and `locals` | ||
| """ | ||
| parsed = ast.parse(expr.strip(), mode="eval") | ||
|
|
||
| def _disallowed_node(n): | ||
| return not any(isinstance(n, at) for at in allowed_types) | ||
|
|
||
| disallowed = list(filter(_disallowed_node, ast.walk(parsed))) | ||
|
|
||
| if disallowed: | ||
| disallowed_strs = list(map(ast.unparse, disallowed)) | ||
| raise ValueError( | ||
| f"Unsafe expression `{expr}` cannot be evaluated, contains disallowed components: {disallowed_strs}" | ||
| ) | ||
|
|
||
| return eval(expr, globals, locals) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| # Copyright (c) MONAI Consortium | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import ast | ||
| import unittest | ||
| from parameterized import parameterized | ||
|
|
||
| from monai.utils import safe_eval | ||
|
|
||
| GOOD_EXPRS = [ | ||
| ("1+2", None, None, 3), | ||
| (" 1 + 2 ", None, None, 3), | ||
| ("1+2+x", {"x": 4}, None, 7), | ||
| ("1+2+x", None, {"x": 4}, 7), | ||
| ("1*2+x", {"x": 4}, None, 6), | ||
| ("(1+2)*3", None, None, 9), | ||
| ("foo+bar", {"foo": 1030}, {"bar": 204}, 1234), | ||
| ] | ||
|
|
||
| BAD_EXPRS = [("foo()",), ("foo.bar",), ("foo[123]",), ("(1,2)",), ("[3,4]",), ("int.__class__.__init__.__globals__",)] | ||
|
|
||
|
|
||
| class TestSafeEval(unittest.TestCase): | ||
| @parameterized.expand(GOOD_EXPRS) | ||
| def test_good_exprs(self, expr, globals, locals, expected): | ||
| """Test valid expressions with globals/locals evaluate to correct values.""" | ||
| result = safe_eval(expr, globals, locals) | ||
| self.assertEqual(result, expected) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| @parameterized.expand(BAD_EXPRS) | ||
| def test_bad_exprs(self, expr): | ||
| """Test bad expressions correctly raise ValueError.""" | ||
| with self.assertRaises(ValueError): | ||
| safe_eval(expr) | ||
|
|
||
| def test_allowed_types(self): | ||
| """Test restricting the allowed list of types.""" | ||
| allowed = [ast.Expression, ast.Constant, ast.BinOp, ast.Add] | ||
| result = safe_eval("1+2", allowed_types=allowed) | ||
| self.assertEqual(result, 3) | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| safe_eval("1*2", allowed_types=allowed) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
Uh oh!
There was an error while loading. Please reload this page.