FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Merge pull request #31544 from ksunden/backport-of-pr-31248-on-v3.10.x · matplotlib/matplotlib@063288d · GitHub

/ matplotlib Public

Commit 063288d

Browse files
timhoffm
authored
Merge pull request #31544 from ksunden/backport-of-pr-31248-on-v3.10.x
Backport PR #31248: SEC: Remove eval() from validate_cycler
2 parents e3fb541 + b2ed196 commit 063288d

4 files changed

Lines changed: 100 additions & 30 deletions

File tree

‎doc/api/next_api_changes/deprecations/31248-SS.rst‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
Arbitrary code in ``axes.prop_cycle`` rcParam strings
2+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3+
4+
The ``axes.prop_cycle`` rcParam accepts Python expressions that are evaluated
5+
in a limited context. The evaluation context has been further limited and some
6+
expressions that previously worked (list comprehensions, for example) no longer
7+
will. This change is made without a deprecation period to improve security.
8+
The previously documented cycler operations at
9+
https://matplotlib.org/cycler/ are still supported.

‎doc/users/next_whats_new/cycler_rcparam_security.rst‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
``axes.prop_cycle`` rcParam security improvements
2+
-------------------------------------------------
3+
4+
The ``axes.prop_cycle`` rcParam is now parsed in a safer and more restricted
5+
manner. Only literals, ``cycler()`` and ``concat()`` calls, the operators
6+
``+`` and ``*``, and slicing are allowed. All previously valid cycler strings
7+
documented at https://matplotlib.org/cycler/ are still supported, for example:
8+
9+
.. code-block:: none
10+
11+
axes.prop_cycle : cycler('color', ['r', 'g', 'b']) + cycler('linewidth', [1, 2, 3])
12+
axes.prop_cycle : 2 * cycler('color', 'rgb')
13+
axes.prop_cycle : concat(cycler('color', 'rgb'), cycler('color', 'cmk'))
14+
axes.prop_cycle : cycler('color', 'rgbcmk')[:3]

‎lib/matplotlib/rcsetup.py‎

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
from matplotlib._enums import JoinStyle, CapStyle
3131

3232
# Don't let the original cycler collide with our validating cycler
33-
from cycler import Cycler, cycler as ccycler
33+
from cycler import Cycler, concat as cconcat, cycler as ccycler
3434

3535

3636
@_api.caching_module_getattr
@@ -759,11 +759,62 @@ def cycler(*args, **kwargs):
759759
return reduce(operator.add, (ccycler(k, v) for k, v in validated))
760760

761761

762-
class _DunderChecker(ast.NodeVisitor):
763-
def visit_Attribute(self, node):
764-
if node.attr.startswith("__") and node.attr.endswith("__"):
765-
raise ValueError("cycler strings with dunders are forbidden")
766-
self.generic_visit(node)
762+
def _parse_cycler_string(s):
763+
"""
764+
Parse a string representation of a cycler into a Cycler object safely,
765+
without using eval().
766+
767+
Accepts expressions like::
768+
769+
cycler('color', ['r', 'g', 'b'])
770+
cycler('color', 'rgb') + cycler('linewidth', [1, 2, 3])
771+
cycler(c='rgb', lw=[1, 2, 3])
772+
cycler('c', 'rgb') * cycler('linestyle', ['-', '--'])
773+
"""
774+
try:
775+
tree = ast.parse(s, mode='eval')
776+
except SyntaxError as e:
777+
raise ValueError(f"Could not parse {s!r}: {e}") from e
778+
return _eval_cycler_expr(tree.body)
779+
780+
781+
def _eval_cycler_expr(node):
782+
"""Recursively evaluate an AST node to build a Cycler object."""
783+
if isinstance(node, ast.BinOp):
784+
left = _eval_cycler_expr(node.left)
785+
right = _eval_cycler_expr(node.right)
786+
if isinstance(node.op, ast.Add):
787+
return left + right
788+
if isinstance(node.op, ast.Mult):
789+
return left * right
790+
raise ValueError(f"Unsupported operator: {type(node.op).__name__}")
791+
if isinstance(node, ast.Call):
792+
if not (isinstance(node.func, ast.Name)
793+
and node.func.id in ('cycler', 'concat')):
794+
raise ValueError(
795+
"only the 'cycler()' and 'concat()' functions are allowed")
796+
func = cycler if node.func.id == 'cycler' else cconcat
797+
args = [_eval_cycler_expr(a) for a in node.args]
798+
kwargs = {kw.arg: _eval_cycler_expr(kw.value) for kw in node.keywords}
799+
return func(*args, **kwargs)
800+
if isinstance(node, ast.Subscript):
801+
sl = node.slice
802+
if not isinstance(sl, ast.Slice):
803+
raise ValueError("only slicing is supported, not indexing")
804+
s = slice(
805+
ast.literal_eval(sl.lower) if sl.lower else None,
806+
ast.literal_eval(sl.upper) if sl.upper else None,
807+
ast.literal_eval(sl.step) if sl.step else None,
808+
)
809+
value = _eval_cycler_expr(node.value)
810+
return value[s]
811+
# Allow literal values (int, strings, lists, tuples) as arguments
812+
# to cycler() and concat().
813+
try:
814+
return ast.literal_eval(node)
815+
except (ValueError, TypeError):
816+
raise ValueError(
817+
f"Unsupported expression in cycler string: {ast.dump(node)}")
767818

768819

769820
# A validator dedicated to the named legend loc
@@ -814,25 +865,11 @@ def _validate_legend_loc(loc):
814865
def validate_cycler(s):
815866
"""Return a Cycler object from a string repr or the object itself."""
816867
if isinstance(s, str):
817-
# TODO: We might want to rethink this...
818-
# While I think I have it quite locked down, it is execution of
819-
# arbitrary code without sanitation.
820-
# Combine this with the possibility that rcparams might come from the
821-
# internet (future plans), this could be downright dangerous.
822-
# I locked it down by only having the 'cycler()' function available.
823-
# UPDATE: Partly plugging a security hole.
824-
# I really should have read this:
825-
# https://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html
826-
# We should replace this eval with a combo of PyParsing and
827-
# ast.literal_eval()
828868
try:
829-
_DunderChecker().visit(ast.parse(s))
830-
s = eval(s, {'cycler': cycler, '__builtins__': {}})
831-
except BaseException as e:
869+
s = _parse_cycler_string(s)
870+
except Exception as e:
832871
raise ValueError(f"{s!r} is not a valid cycler construction: {e}"
833872
) from e
834-
# Should make sure what comes from the above eval()
835-
# is a Cycler object.
836873
if isinstance(s, Cycler):
837874
cycler_inst = s
838875
else:
@@ -1101,7 +1138,7 @@ def _convert_validator_spec(key, conv):
11011138
"axes.formatter.offset_threshold": validate_int,
11021139
"axes.unicode_minus": validate_bool,
11031140
# This entry can be either a cycler object or a string repr of a
1104-
# cycler-object, which gets eval()'ed to create the object.
1141+
# cycler-object, which is parsed safely via AST.
11051142
"axes.prop_cycle": validate_cycler,
11061143
# If "data", axes limits are set close to the data.
11071144
# If "round_numbers" axes limits are set to the nearest round numbers.

‎lib/matplotlib/tests/test_rcparams.py‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -272,16 +272,23 @@ def generate_validator_testcases(valid):
272272
cycler('linestyle', ['-', '--'])),
273273
(cycler(mew=[2, 5]),
274274
cycler('markeredgewidth', [2, 5])),
275+
("2 * cycler('color', 'rgb')", 2 * cycler('color', 'rgb')),
276+
("2 * cycler('color', 'r' + 'gb')", 2 * cycler('color', 'rgb')),
277+
("cycler(c='r' + 'gb', lw=[1, 2, 3])",
278+
cycler('color', 'rgb') + cycler('linewidth', [1, 2, 3])),
279+
("cycler('color', 'rgb') * 2", cycler('color', 'rgb') * 2),
280+
("concat(cycler('color', 'rgb'), cycler('color', 'cmk'))",
281+
cycler('color', list('rgbcmk'))),
282+
("cycler('color', 'rgbcmk')[:3]", cycler('color', list('rgb'))),
283+
("cycler('color', 'rgb')[::-1]", cycler('color', list('bgr'))),
275284
),
276-
# This is *so* incredibly important: validate_cycler() eval's
277-
# an arbitrary string! I think I have it locked down enough,
278-
# and that is what this is testing.
279-
# TODO: Note that these tests are actually insufficient, as it may
280-
# be that they raised errors, but still did an action prior to
281-
# raising the exception. We should devise some additional tests
282-
# for that...
285+
# validate_cycler() parses an arbitrary string using a safe
286+
# AST-based parser (no eval). These tests verify that only valid
287+
# cycler expressions are accepted.
283288
'fail': ((4, ValueError), # Gotta be a string or Cycler object
284289
('cycler("bleh, [])', ValueError), # syntax error
290+
("cycler('color', 'rgb') * * cycler('color', 'rgb')", # syntax error
291+
ValueError),
285292
('Cycler("linewidth", [1, 2, 3])',
286293
ValueError), # only 'cycler()' function is allowed
287294
# do not allow dunder in string literals
@@ -295,6 +302,9 @@ def generate_validator_testcases(valid):
295302
ValueError),
296303
("cycler('c', [j.__class__(j).lower() for j in ['r', 'b']])",
297304
ValueError),
305+
# list comprehensions are arbitrary code, even if "safe"
306+
("cycler('color', [x for x in ['r', 'g', 'b']])",
307+
ValueError),
298308
('1 + 2', ValueError), # doesn't produce a Cycler object
299309
('os.system("echo Gotcha")', ValueError), # os not available
300310
('import os', ValueError), # should not be able to import

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL