| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Please add some tests.
Sorry, something went wrong.
|
This was pretty well exercised with test_validator_invalid already. I added test_validate_cycler_no_code_execution with a (benign) proof of concept code injection that evaluates on main but not this branch. |
Sorry, something went wrong.
| 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>
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Lets not support lower until we get a bug report.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
cpython docs PR: python/cpython#145773
Sorry, something went wrong.
There was a problem hiding this comment.
Merged!
Docs reflect this now: https://docs.python.org/3/library/functions.html#eval
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
Unfortunately, my takeaway from what I've been reading about this is that it's essentially impossible to sandbox eval() by blocking lists of known exploits, and an allowlist of operations with ast.literal_eval is the only reasonably safe way of allowing users to submit arbitrary code. See for example a myriad of techniques on display here: https://gist.github.com/rebane2001/a3734ecae4a05e3b85bafc1ae17ee864 |
Sorry, something went wrong.
|
Added a deprecation notice. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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:
***@***.***>
|
Sorry, something went wrong.
|
I'm ok with this going in as-is, but both of Elliott's comments are valid. |
Sorry, something went wrong.
There was a problem hiding this comment.
Should we backport? / Will there be a 3.10.9 release?
Sorry, something went wrong.
|
You will need to rebase if you want to get docs build working due to differences in how it processes PR merges/heads. |
Sorry, something went wrong.
|
But on the other hand CI is broken without setuptools-scm being pinned, so you may want to wait on that. |
Sorry, something went wrong.
…through a malicious matplotlibrc
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
git checkout v3.10.x git pull
git cherry-pick -x -m1 2ad0aa7d24d3cca8505b89b763559ceba025d004
git commit -am 'Backport PR #31248: SEC: Remove eval() from validate_cycler'
git push YOURFORK v3.10.x:auto-backport-of-pr-31248-on-v3.10.x
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the Still Needs Manual Backport label once the PR gets merged. If these instructions are inaccurate, feel free to suggest an improvement. |
Sorry, something went wrong.
Backport PR #31248: SEC: Remove eval() from validate_cycler
| Back | FazBrowse Home | New Git URL |
PR summary
validate_cycler() uses eval() to parse axes.prop_cycle rcParam strings. Combined with automatic matplotlibrc loading from the current working directory, this allows arbitrary code execution on import matplotlib via 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