SEC: Remove eval() from validate_cycler#31248
SEC: Remove eval() from validate_cycler#31248scottshambaugh wants to merge 4 commits intomatplotlib:mainfrom
Conversation
…through a malicious matplotlibrc
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.
| cycler('c', 'rgb') * cycler('linestyle', ['-', '--']) | ||
| """ | ||
| try: | ||
| tree = ast.parse(s, mode='eval') |
There was a problem hiding this comment.
I'm not finding much information about the mode parameter. https://docs.python.org/3/library/ast.html#ast.parse. It would be useful to understand what setting it to "eval" means.
There was a problem hiding this comment.
It's not super clearly laid out, but my understanding it that this limits inputs to single expressions rather than statements (e.g. imports, variable assignments, defs) or compiling modules
There was a problem hiding this comment.
It's specified in the compile() docs
The mode argument specifies what kind of code must be compiled; it can be 'exec' if source consists of a sequence of statements, 'eval' if it consists of a single expression, or 'single' if it consists of a single interactive statement (in the latter case, expression statements that evaluate to something other than None will be printed).
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
fa7b0aa to
e7694f6
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