SEC: Remove eval() from validate_cycler#31248
Conversation
580941d to
09b61ac
Compare
|
This was pretty well exercised with |
| ValueError), | ||
| ("cycler('c', [j.\u000c__class__(j) for j in ['r', 'b']])", | ||
| ValueError), | ||
| ("cycler('c', [j.__class__(j).lower() for j in ['r', 'b']])", |
There was a problem hiding this comment.
This seems to imply that since only dunder methods are blocked, one could call .lower(). Indeed, this does work currently:
>>> mpl.rcParams['axes.prop_cycle'] = 'cycler("color", [x.lower() for x in "RGB"])'
>>> mpl.rcParams['axes.prop_cycle']
cycler('color', ['r', 'g', 'b'])This doesn't appear on the success list however, so I'm not sure it's intended to work.
In this PR it now fails, with a bit of an inscrutable error:
ValueError: Key axes.prop_cycle: 'cycler("color", [x.lower() for x in "RGB"])' is not a valid cycler construction: malformed node or string on line 1: <ast.ListComp object at 0x7fb87f6074d0>
There was a problem hiding this comment.
I think that’s ok. I consider the cycler spec in matplotlibrc as purely declarative. You should write out the lists explicitly.
There was a problem hiding this comment.
non-dunder methods of literals were intended to work. You are right, I should have added that to the list. So, some people may have discovered that and used it.
There was a problem hiding this comment.
Lets not support lower until we get a bug report.
lib/matplotlib/rcsetup.py
Outdated
| kwargs = {kw.arg: ast.literal_eval(kw.value) for kw in node.keywords} | ||
| return cycler(*args, **kwargs) | ||
| raise ValueError( | ||
| f"Unsupported expression in cycler string: {ast.dump(node)}") |
There was a problem hiding this comment.
Please keep in mind all of the valid ways cyclers can be composed: https://matplotlib.org/cycler/#composition. We didn't test all of it here because it was tested in that project.
There was a problem hiding this comment.
Thanks, I had missed that link.
It seems like the operations that were missing were integer multiplication, concat, and slicing. I believe all of those should be safe, so added them in here explicitly with some tests.
There was a problem hiding this comment.
Arbitrary non-dunder methods on the other hand I don't think we should allow here given the security risk. I'm a little skeptical that their use would be widespread, but we could always add back in specific safe methods (e.g. lower()) if people complain.
There was a problem hiding this comment.
methods from standard types that derive from literals are "safe" in that they won't provide a mechanism to modify state or chain to arbitrary code execution. At least, that was the view we had when we originally designed this.
There was a problem hiding this comment.
Had Claude tackle this for 20 minutes and found an exploit to execute arbitrary code with the original implementation using non-dunder methods. Can import modules, write to file, etc. I'll dm you on discourse instead of posting it publicly.
Edit: Don't see you on discourse @WeatherGod! Can email you if you'd like, if you reach out with contact info.
There was a problem hiding this comment.
I'm going to raise this with cpython core, since the __builtins__ approach is (incorrectly) advertised as limiting the scope:
https://docs.python.org/3/library/functions.html#eval
There was a problem hiding this comment.
Good idea to raise this with cpython devs because that would be a significant security issue. No need to inform me what the exploit is, I'd rather not have that knowledge. Just rather know if it can be fixed from cpython's end.
Given that you were able to find a possible exploit validates the effort here to move away from the old appraoch. I did something similar to this for an employer, so I need to go back over my notes and see if there is anything else to watch out for.
There was a problem hiding this comment.
Found a good amount of prior art on this method since yesterday so it's not new... my ask to the cpython security team was to update the eval docs because "you can control what builtins are available to the executed code" is incorrect security guidance.
There was a problem hiding this comment.
Merged!
Docs reflect this now: https://docs.python.org/3/library/functions.html#eval
fa7b0aa to
e7694f6
Compare
|
I don't disagree with limiting the parsing of the cycler parameter, but strictly speaking, it is an API change which normally would go through a deprecation cycle. I'm assuming that the parsing limitations goes beyond what is strictly needed to close any known security holes. What I'm worried about is that I don't know if we have any idea just how widespread the more advanced formulations of cycler is out there that could be impacted by this change. Is it at all possible to plug known security holes with the current approach and raise a deprecation warning if we detect the sort of stuff we want to eventually restrict? |
|
Unfortunately, my takeaway from what I've been reading about this is that it's essentially impossible to sandbox See for example a myriad of techniques on display here: https://gist.github.com/rebane2001/a3734ecae4a05e3b85bafc1ae17ee864 |
|
Added a deprecation notice. |
4a2c752 to
610f7b9
Compare
|
I've also found some of the ways to escape the current validation. Based on what they look like if users are "legitimately" doing those things to get a cycler then I have no compunction about breaking them. |
|
Ah, that might be why I used it in one of my projects awhile back. Doesn't
seem like there is any concern about infinite recursions or looped
references like one typically worries about when traversing trees. Maybe
one additional safety check is an upper limit to the size of the string.
Something ridiculous like 1MB. More than enough for any reasonable use, but
still providing an upper bound so that we never crash the interpreter?
…On Fri, Mar 13, 2026 at 3:40 PM Elliott Sales de Andrade < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/matplotlib/rcsetup.py
<#31248 (comment)>
:
> + 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."""
The ast.walk docs do say:
This is useful if you only want to modify nodes in place and don’t care
about the context.
So it may not be applicable here anyway?
—
Reply to this email directly, view it on GitHub
<#31248 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHF6ELC2RFNN5ZIGWFLHT4QRPZNAVCNFSM6AAAAACWKBDXV2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTSNBWGU2DQMZWG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'm ok with this going in as-is, but both of Elliott's comments are valid. |
timhoffm
left a comment
There was a problem hiding this comment.
Should we backport? / Will there be a 3.10.9 release?
|
You will need to rebase if you want to get docs build working due to differences in how it processes PR merges/heads. |
|
But on the other hand CI is broken without setuptools-scm being pinned, so you may want to wait on that. |
…through a malicious matplotlibrc
dfe0b14 to
99d07bc
Compare
PR summary
validate_cycler()useseval()to parseaxes.prop_cyclercParam strings. Combined with automatic matplotlibrc loading from the current working directory, this allows arbitrary code execution onimport matplotlibvia a malicious config file in a local directory. This poses a security risk.AI Disclosure
Claude was used to run the audit. Code manually reviewed
PR checklist