diff --git a/qlib/data/cache.py b/qlib/data/cache.py index fbf6e839db1..2e85d1b8a4b 100644 --- a/qlib/data/cache.py +++ b/qlib/data/cache.py @@ -25,7 +25,6 @@ hash_args, get_redis_connection, read_bin, - parse_field, remove_fields_space, normalize_cache_fields, normalize_cache_instruments, @@ -34,6 +33,7 @@ from ..log import get_module_logger from .base import Feature +from .expression_parser import parse_expression from .ops import Operators # pylint: disable=W0611 # noqa: F401 @@ -540,7 +540,7 @@ def _expression(self, instrument, field, start_time=None, end_time=None, freq="d field = remove_fields_space(field) # cache unavailable, generate the cache _instrument_dir.mkdir(parents=True, exist_ok=True) - if not isinstance(eval(parse_field(field)), Feature): + if not isinstance(parse_expression(field), Feature): # When the expression is not a raw feature # generate expression cache if the feature is not a Feature # instance diff --git a/qlib/data/data.py b/qlib/data/data.py index aba75c0b1ab..f9f4dc87050 100644 --- a/qlib/data/data.py +++ b/qlib/data/data.py @@ -28,7 +28,6 @@ init_instance_by_config, register_wrapper, get_module_by_module_path, - parse_field, hash_args, normalize_cache_fields, code_to_fname, @@ -38,6 +37,7 @@ ) from ..utils.paral import ParallelExt from .ops import Operators # pylint: disable=W0611 # noqa: F401 +from .expression_parser import parse_expression class ProviderBackendMixin: @@ -394,7 +394,7 @@ def get_expression_instance(self, field): if field in self.expression_instance_cache: expression = self.expression_instance_cache[field] else: - expression = eval(parse_field(field)) + expression = parse_expression(field) self.expression_instance_cache[field] = expression except NameError as e: get_module_logger("data").exception( diff --git a/qlib/data/expression_parser.py b/qlib/data/expression_parser.py new file mode 100644 index 00000000000..2b546e07599 --- /dev/null +++ b/qlib/data/expression_parser.py @@ -0,0 +1,136 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. +"""Safe parser for qlib feature expression strings. + +The qlib feature DSL lets users write expressions such as:: + + Ref($close, -2) / Ref($close, -1) - 1 + +which are transformed by :func:`qlib.utils.parse_field` into:: + + Operators.Ref(Feature("close"), -2) / Operators.Ref(Feature("close"), -1) - 1 + +Historically qlib evaluated the transformed string with the builtin +``eval`` which allowed arbitrary Python — an attacker who could influence +a field string (for example through a workflow YAML config) could gain +code execution (CWE-94). + +This module implements a whitelist-based AST evaluator that only permits +the constructs actually used by the DSL: numeric/string/boolean/None +literals, tuples/lists, unary +/-, arithmetic and comparison operators, +and calls to a small set of allowed names (``Feature``, ``PFeature`` and +``Operators.``). Anything else — attribute traversal, arbitrary +name lookups, ``__import__``, generator/lambda/subscript-escape tricks — +raises :class:`ValueError`. +""" + +from __future__ import annotations + +import ast +from typing import Any, Dict + +from ..utils import parse_field +from .base import Feature, PFeature +from .ops import Operators + + +# AST nodes that are structurally safe (contain no executable side effects +# on their own; their children are validated recursively). +_ALLOWED_NODES = ( + ast.Expression, + ast.Constant, + ast.Num, # legacy alias (<3.8) + ast.Str, # legacy alias (<3.8) + ast.NameConstant, # legacy alias (<3.8) + ast.UnaryOp, + ast.UAdd, + ast.USub, + ast.BinOp, + ast.Add, + ast.Sub, + ast.Mult, + ast.Div, + ast.FloorDiv, + ast.Mod, + ast.Pow, + ast.BoolOp, + ast.And, + ast.Or, + ast.Compare, + ast.Eq, + ast.NotEq, + ast.Lt, + ast.LtE, + ast.Gt, + ast.GtE, + ast.Tuple, + ast.List, + ast.Load, + ast.Call, + ast.keyword, + ast.Name, + ast.Attribute, +) + +# Only these bare names may appear in an expression. +_ALLOWED_NAMES = frozenset({"Feature", "PFeature", "Operators", "True", "False", "None"}) + + +def _validate(node: ast.AST) -> None: + for child in ast.walk(node): + if not isinstance(child, _ALLOWED_NODES): + raise ValueError( + f"Disallowed expression element: {type(child).__name__}" + ) + if isinstance(child, ast.Attribute): + # Only ``Operators.`` is allowed; the value must be the + # bare name ``Operators``. + if not (isinstance(child.value, ast.Name) and child.value.id == "Operators"): + raise ValueError("Only attribute access on 'Operators' is allowed") + if child.attr.startswith("_"): + raise ValueError("Private attribute access is not allowed") + if isinstance(child, ast.Name) and child.id not in _ALLOWED_NAMES: + raise ValueError(f"Unknown name in expression: {child.id!r}") + if isinstance(child, ast.Call): + # keyword arguments are OK, but reject **kwargs / *args unpacking + # that could smuggle arbitrary objects. + for kw in child.keywords: + if kw.arg is None: + raise ValueError("**kwargs is not allowed in expressions") + + +def _safe_namespace() -> Dict[str, Any]: + return { + "__builtins__": {}, + "Feature": Feature, + "PFeature": PFeature, + "Operators": Operators, + "True": True, + "False": False, + "None": None, + } + + +def parse_expression(field: str) -> Any: + """Parse a qlib feature-DSL string and return the expression object. + + This is the safe replacement for ``eval(parse_field(field))``. + + Parameters + ---------- + field: + The user-facing feature expression, e.g. ``"Ref($close, -1)"``. + + Raises + ------ + ValueError + If ``field`` contains anything outside of the feature DSL. + SyntaxError + If the transformed expression is not valid Python. + """ + transformed = parse_field(field) + tree = ast.parse(transformed, mode="eval") + _validate(tree) + code = compile(tree, filename="", mode="eval") + # pylint: disable=eval-used # sandboxed: AST whitelist + no builtins + return eval(code, _safe_namespace()) # noqa: S307 diff --git a/tests/misc/test_expression_parser_safety.py b/tests/misc/test_expression_parser_safety.py new file mode 100644 index 00000000000..a4592bb71dd --- /dev/null +++ b/tests/misc/test_expression_parser_safety.py @@ -0,0 +1,84 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. +"""Regression test for CWE-94: user-controlled feature expressions must not +be evaluated as arbitrary Python. + +The test targets the safe validator directly so it can run without +optional runtime dependencies (redis, joblib, ...). It reimplements the +whitelist portion of ``qlib.data.expression_parser`` and asserts that +malicious payloads are rejected before any evaluation happens. +""" +import ast +import os +import unittest + +from qlib.utils import parse_field + + +# Mirror qlib.data.expression_parser._ALLOWED_NODES / _ALLOWED_NAMES. +_ALLOWED_NODES = ( + ast.Expression, ast.Constant, ast.UnaryOp, ast.UAdd, ast.USub, + ast.BinOp, ast.Add, ast.Sub, ast.Mult, ast.Div, ast.FloorDiv, ast.Mod, ast.Pow, + ast.BoolOp, ast.And, ast.Or, + ast.Compare, ast.Eq, ast.NotEq, ast.Lt, ast.LtE, ast.Gt, ast.GtE, + ast.Tuple, ast.List, ast.Load, ast.Call, ast.keyword, ast.Name, ast.Attribute, +) +_ALLOWED_NAMES = frozenset({"Feature", "PFeature", "Operators", "True", "False", "None"}) + + +def _validate(tree): + for c in ast.walk(tree): + if not isinstance(c, _ALLOWED_NODES): + raise ValueError(type(c).__name__) + if isinstance(c, ast.Attribute): + if not (isinstance(c.value, ast.Name) and c.value.id == "Operators"): + raise ValueError("attr") + if c.attr.startswith("_"): + raise ValueError("private") + if isinstance(c, ast.Name) and c.id not in _ALLOWED_NAMES: + raise ValueError("name " + c.id) + + +def _check(field): + transformed = parse_field(field) + tree = ast.parse(transformed, mode="eval") + _validate(tree) + + +class ExpressionParserSafetyTest(unittest.TestCase): + def test_benign_expressions_are_accepted(self): + for expr in [ + "$close", + "$$my_feat", + "Ref($close, -2)/Ref($close, -1) - 1", + "Cut($close, 486, None)", + "(2*$close-$high-$low)/($high-$low+1e-12)", + "If(Gt($close, 1.0), $high, $low)", + ]: + _check(expr) # must not raise + + def test_import_payload_is_blocked(self): + marker = "/tmp/qlib_cwe94_marker" + if os.path.exists(marker): + os.unlink(marker) + with self.assertRaises((ValueError, SyntaxError)): + _check(f"__import__('os').system('touch {marker}')") + self.assertFalse(os.path.exists(marker)) + + def test_dunder_escape_is_blocked(self): + with self.assertRaises((ValueError, SyntaxError)): + _check("().__class__.__mro__[-1].__subclasses__()") + + def test_unknown_name_is_blocked(self): + with self.assertRaises((ValueError, SyntaxError)): + _check("open('/etc/passwd').read()") + + def test_lambda_and_comprehensions_blocked(self): + with self.assertRaises((ValueError, SyntaxError)): + _check("(lambda: 1)()") + with self.assertRaises((ValueError, SyntaxError)): + _check("[x for x in ()]") + + +if __name__ == "__main__": + unittest.main()