fix(data): sandbox feature-expression parser to prevent code execution (CWE-94)#2300
Open
sebastiondev wants to merge 1 commit into
Open
fix(data): sandbox feature-expression parser to prevent code execution (CWE-94)#2300sebastiondev wants to merge 1 commit into
sebastiondev wants to merge 1 commit into
Conversation
User-supplied feature strings (for example those loaded from a workflow
YAML config) were transformed by parse_field() and then passed straight
to the builtin evaluator, which allowed arbitrary Python execution — a
payload such as __import__('os').system(...) would run during data
preparation.
Introduce qlib.data.expression_parser.parse_expression() that parses the
transformed string with ast.parse(..., mode='eval'), validates the AST
against a whitelist of nodes (numeric/string/None/bool literals, tuples,
arithmetic/comparison ops, calls) and a whitelist of names (Feature,
PFeature, Operators), forbids attribute access outside Operators.<op>,
forbids private (leading-underscore) attributes and **kwargs unpacking,
and executes the compiled code in a namespace with no builtins.
Callers in qlib/data/data.py (ExpressionProvider.get_expression_instance)
and qlib/data/cache.py (_expression) now use the safe parser.
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.
Description
ExpressionProvider.get_expressionandDiskExpressionCache._urievaluate feature-expression strings with the builtineval:parse_field(inqlib/utils/__init__.py) only performs regex rewrites — it turns$closeintoFeature("close")andRef(...)intoOperators.Ref(...), but it does not validate or sanitize the input. Any other Python syntax in the field string is passed straight through toeval, which happily executes it in a scope that has__builtins__available. That is a textbook CWE-94 (Improper Control of Generation of Code / code injection) sink.Field strings reach these functions from user-controlled workflow YAML configs (
qrun config.yaml) and from dataset/handler configs shared between users — a very common qlib workflow. A malicious config such asexecutes arbitrary code the moment the handler is initialized.
Motivation and Context
CWE-94 — arbitrary code execution via feature expression strings. Qlib's ecosystem revolves around sharing YAML configs (examples in
examples/, community strategies, tutorials), so "the attacker controls a field string" is a realistic threat, not a theoretical one. Opening a shared config should not be equivalent to runningpython -c "$(cat config.yaml)".Fix
This PR replaces
eval(parse_field(field))with a newparse_expression(field)helper inqlib/data/expression_parser.py. The helper:parse_fieldregex rewrite (so the DSL still works identically).ast.parse(..., mode="eval").ast.Constant, plus legacyNum/Str/NameConstant), tuples/lists.+/-, binary arithmetic, boolean ops, comparisons.Namenodes only if they resolve toFeature,PFeature, orOperators.Attributenodes only onOperatorsand only for non-dunder names.Callnodes with positional and simple keyword arguments (no*args/**kwargsunpacking).__builtins__cleared and a fixed name table{Feature, PFeature, Operators}.Attribute access, subscripting, comprehensions, lambdas, f-strings,
__import__, dunder traversal, and any unknown identifier all raiseValueErrorbefore any code runs.Call sites in
qlib/data/data.pyandqlib/data/cache.pyswitch fromeval(parse_field(...))toparse_expression(...). The publicparse_fieldhelper is left in place for backward compatibility with any downstream users that only want the string rewrite.How Has This Been Tested?
tests/misc/test_expression_parser_safety.pywith cases covering:$close,Ref($close, -1),Mean($high-$low, 5) / $close) parse to the same objects the oldevalpath produced.__import__('os').system('id'),().__class__.__bases__[0].__subclasses__(),(lambda: 1)(),[x for x in range(1)],open('/etc/passwd'),Operators.__class__,Ref($close, **{'d': -1}).examples/benchmarks/throughparse_expressionto confirm parity with the previousevalbehavior.Security analysis
We verified this is exploitable end-to-end: constructing a
QlibDataLoader(or runningqrun) with afieldsentry containing__import__('os').system('...')executes the payload on the current interpreter, with the current user's privileges, at handler-initialization time — no other preconditions.The fix removes the
evalsink entirely. The AST walker refuses any node type it hasn't explicitly whitelisted, so future additions to Python's grammar can't silently widen the attack surface — a new node type will raiseValueErroruntil it's reviewed and added. Attribute access is restricted toOperators.<name>with a dunder check, which blocks the classic().__class__.__bases__[0].__subclasses__()escape.Adversarial review
Before submitting, we tried to disprove this. We checked whether qlib treats configs as trusted-by-default (it doesn't — configs are routinely shared and downloaded), whether any upstream layer sanitizes field strings before they reach
eval(no —parse_fieldis purely a regex rewrite), and whether the attacker needs privileges that would already grant equivalent access (no — reading a YAML file is not equivalent to arbitrary Python execution). We also confirmed the whitelist doesn't over-restrict: every operator inqlib/data/ops.pyis reachable viaOperators.<name>(...), which the whitelist permits.Proof of Concept
Or via a workflow config:
After this patch both raise
ValueError: unsupported expression element ...and no code executes.Types of changes
Discovered by the Sebastion AI GitHub App.