X Tutup
Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/next_api_changes/deprecations/31248-SS.rst
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.
14 changes: 14 additions & 0 deletions doc/release/next_whats_new/cycler_rcparam_security.rst
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]
83 changes: 60 additions & 23 deletions lib/matplotlib/rcsetup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to utilize ast.walk() to walk the tree nodes? It should help prevent any recursion errors or other oddities that can happen with trees.

if isinstance(node, ast.BinOp):
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, do you see any issues with that?

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func = cycler if node.func.id == 'cycler' else cconcat
func = ccycler if node.func.id == 'cycler' else cconcat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation on each node with cycler is needed, tests fail with ccycler.

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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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.
Expand Down
22 changes: 15 additions & 7 deletions lib/matplotlib/tests/test_rcparams.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,16 +275,21 @@ def generate_validator_testcases(valid):
cycler('linestyle', ['-', '--'])),
(cycler(mew=[2, 5]),
cycler('markeredgewidth', [2, 5])),
("2 * cycler('color', 'rgb')", 2 * cycler('color', 'rgb')),
("2 * cycler('color', 'r' + 'gb')", 2 * cycler('color', 'rgb')),
("cycler('color', 'rgb') * 2", cycler('color', 'rgb') * 2),
("concat(cycler('color', 'rgb'), cycler('color', 'cmk'))",
cycler('color', list('rgbcmk'))),
("cycler('color', 'rgbcmk')[:3]", cycler('color', list('rgb'))),
("cycler('color', 'rgb')[::-1]", cycler('color', list('bgr'))),
),
# This is *so* incredibly important: validate_cycler() eval's
# an arbitrary string! I think I have it locked down enough,
# and that is what this is testing.
# TODO: Note that these tests are actually insufficient, as it may
# be that they raised errors, but still did an action prior to
# raising the exception. We should devise some additional tests
# for that...
# validate_cycler() parses an arbitrary string using a safe
# AST-based parser (no eval). These tests verify that only valid
# cycler expressions are accepted.
'fail': ((4, ValueError), # Gotta be a string or Cycler object
('cycler("bleh, [])', ValueError), # syntax error
("cycler('color', 'rgb') * * cycler('color', 'rgb')", # syntax error
ValueError),
('Cycler("linewidth", [1, 2, 3])',
ValueError), # only 'cycler()' function is allowed
# do not allow dunder in string literals
Expand All @@ -298,6 +303,9 @@ def generate_validator_testcases(valid):
ValueError),
("cycler('c', [j.__class__(j).lower() for j in ['r', 'b']])",
ValueError),
# list comprehensions are arbitrary code, even if "safe"
("cycler('color', [x for x in ['r', 'g', 'b']])",
ValueError),
('1 + 2', ValueError), # doesn't produce a Cycler object
('os.system("echo Gotcha")', ValueError), # os not available
('import os', ValueError), # should not be able to import
Expand Down
Loading
X Tutup