-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
SEC: Remove eval() from validate_cycler #31248
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: main
Are you sure you want to change the base?
Changes from all commits
09b61ac
0065bfc
45b6acd
e7694f6
f944243
610f7b9
4eba537
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 |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| Arbitrary code in ``axes.prop_cycle`` rcParam strings | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| The ``axes.prop_cycle`` rcParam accepts Python expressions that are evaluated | ||
| in a limited context. The evaluation context has been further limited and some | ||
| expressions that previously worked (list comprehensions, for example) no longer | ||
| will. This change is made without a deprecation period to improve security. | ||
| The previously documented cycler operations at | ||
| https://matplotlib.org/cycler/ are still supported. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| ``axes.prop_cycle`` rcParam security improvements | ||
| ------------------------------------------------- | ||
|
|
||
| The ``axes.prop_cycle`` rcParam is now parsed in a safer and more restricted | ||
| manner. Only literals, ``cycler()`` and ``concat()`` calls, the operators | ||
| ``+`` and ``*``, and slicing are allowed. All previously valid cycler strings | ||
| documented at https://matplotlib.org/cycler/ are still supported, for example: | ||
|
|
||
| .. code-block:: none | ||
| axes.prop_cycle : cycler('color', ['r', 'g', 'b']) + cycler('linewidth', [1, 2, 3]) | ||
| axes.prop_cycle : 2 * cycler('color', 'rgb') | ||
| axes.prop_cycle : concat(cycler('color', 'rgb'), cycler('color', 'cmk')) | ||
| axes.prop_cycle : cycler('color', 'rgbcmk')[:3] |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -36,7 +36,7 @@ | |||||
| from matplotlib._enums import JoinStyle, CapStyle | ||||||
|
|
||||||
| # Don't let the original cycler collide with our validating cycler | ||||||
| from cycler import Cycler, cycler as ccycler | ||||||
| from cycler import Cycler, concat as cconcat, cycler as ccycler | ||||||
|
|
||||||
|
|
||||||
| class ValidateInStrings: | ||||||
|
|
@@ -815,11 +815,62 @@ def cycler(*args, **kwargs): | |||||
| return reduce(operator.add, (ccycler(k, v) for k, v in validated)) | ||||||
|
|
||||||
|
|
||||||
| class _DunderChecker(ast.NodeVisitor): | ||||||
| def visit_Attribute(self, node): | ||||||
| if node.attr.startswith("__") and node.attr.endswith("__"): | ||||||
| raise ValueError("cycler strings with dunders are forbidden") | ||||||
| self.generic_visit(node) | ||||||
| def _parse_cycler_string(s): | ||||||
| """ | ||||||
| Parse a string representation of a cycler into a Cycler object safely, | ||||||
| without using eval(). | ||||||
|
|
||||||
| Accepts expressions like:: | ||||||
|
|
||||||
| cycler('color', ['r', 'g', 'b']) | ||||||
| cycler('color', 'rgb') + cycler('linewidth', [1, 2, 3]) | ||||||
| cycler(c='rgb', lw=[1, 2, 3]) | ||||||
| cycler('c', 'rgb') * cycler('linestyle', ['-', '--']) | ||||||
| """ | ||||||
| try: | ||||||
| tree = ast.parse(s, mode='eval') | ||||||
| except SyntaxError as e: | ||||||
| raise ValueError(f"Could not parse {s!r}: {e}") from e | ||||||
| return _eval_cycler_expr(tree.body) | ||||||
|
|
||||||
|
|
||||||
| def _eval_cycler_expr(node): | ||||||
| """Recursively evaluate an AST node to build a Cycler object.""" | ||||||
|
Member
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. Would it be better to utilize |
||||||
| if isinstance(node, ast.BinOp): | ||||||
|
Member
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. if I am reading this right we support the binary ops between any valid expressions?
Contributor
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. Yes, do you see any issues with that?
Member
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. I don't see an issue, but just thinking through exactly how locked down we can make it. For example, can we check that at least one side of this is a Cycler? |
||||||
| left = _eval_cycler_expr(node.left) | ||||||
| right = _eval_cycler_expr(node.right) | ||||||
| if isinstance(node.op, ast.Add): | ||||||
| return left + right | ||||||
| if isinstance(node.op, ast.Mult): | ||||||
| return left * right | ||||||
| raise ValueError(f"Unsupported operator: {type(node.op).__name__}") | ||||||
| if isinstance(node, ast.Call): | ||||||
| if not (isinstance(node.func, ast.Name) | ||||||
| and node.func.id in ('cycler', 'concat')): | ||||||
| raise ValueError( | ||||||
| "only the 'cycler()' and 'concat()' functions are allowed") | ||||||
| func = cycler if node.func.id == 'cycler' else cconcat | ||||||
|
Member
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.
Suggested change
Contributor
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. The validation on each node with |
||||||
| args = [_eval_cycler_expr(a) for a in node.args] | ||||||
| kwargs = {kw.arg: ast.literal_eval(kw.value) for kw in node.keywords} | ||||||
| return func(*args, **kwargs) | ||||||
| if isinstance(node, ast.Subscript): | ||||||
| value = _eval_cycler_expr(node.value) | ||||||
| sl = node.slice | ||||||
| if isinstance(sl, ast.Slice): | ||||||
| s = slice( | ||||||
| ast.literal_eval(sl.lower) if sl.lower else None, | ||||||
| ast.literal_eval(sl.upper) if sl.upper else None, | ||||||
| ast.literal_eval(sl.step) if sl.step else None, | ||||||
| ) | ||||||
| return value[s] | ||||||
| raise ValueError("only slicing is supported, not indexing") | ||||||
| # Allow literal values (int, strings, lists, tuples) as arguments | ||||||
| # to cycler() and concat(). | ||||||
| try: | ||||||
| return ast.literal_eval(node) | ||||||
| except (ValueError, TypeError): | ||||||
| raise ValueError( | ||||||
| f"Unsupported expression in cycler string: {ast.dump(node)}") | ||||||
|
|
||||||
|
|
||||||
| # A validator dedicated to the named legend loc | ||||||
|
|
@@ -870,25 +921,11 @@ def _validate_legend_loc(loc): | |||||
| def validate_cycler(s): | ||||||
| """Return a Cycler object from a string repr or the object itself.""" | ||||||
| if isinstance(s, str): | ||||||
| # TODO: We might want to rethink this... | ||||||
| # While I think I have it quite locked down, it is execution of | ||||||
| # arbitrary code without sanitation. | ||||||
| # Combine this with the possibility that rcparams might come from the | ||||||
| # internet (future plans), this could be downright dangerous. | ||||||
| # I locked it down by only having the 'cycler()' function available. | ||||||
| # UPDATE: Partly plugging a security hole. | ||||||
| # I really should have read this: | ||||||
| # https://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html | ||||||
| # We should replace this eval with a combo of PyParsing and | ||||||
| # ast.literal_eval() | ||||||
| try: | ||||||
| _DunderChecker().visit(ast.parse(s)) | ||||||
| s = eval(s, {'cycler': cycler, '__builtins__': {}}) | ||||||
| except BaseException as e: | ||||||
| s = _parse_cycler_string(s) | ||||||
| except Exception as e: | ||||||
| raise ValueError(f"{s!r} is not a valid cycler construction: {e}" | ||||||
| ) from e | ||||||
| # Should make sure what comes from the above eval() | ||||||
| # is a Cycler object. | ||||||
| if isinstance(s, Cycler): | ||||||
| cycler_inst = s | ||||||
| else: | ||||||
|
|
@@ -1160,7 +1197,7 @@ def _convert_validator_spec(key, conv): | |||||
| "axes.formatter.offset_threshold": validate_int, | ||||||
| "axes.unicode_minus": validate_bool, | ||||||
| # This entry can be either a cycler object or a string repr of a | ||||||
| # cycler-object, which gets eval()'ed to create the object. | ||||||
| # cycler-object, which is parsed safely via AST. | ||||||
| "axes.prop_cycle": validate_cycler, | ||||||
| # If "data", axes limits are set close to the data. | ||||||
| # If "round_numbers" axes limits are set to the nearest round numbers. | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.