X Tutup
Skip to content

SEC: Remove eval() from validate_cycler#31248

Open
scottshambaugh wants to merge 4 commits intomatplotlib:mainfrom
scottshambaugh:rcparam_eval
Open

SEC: Remove eval() from validate_cycler#31248
scottshambaugh wants to merge 4 commits intomatplotlib:mainfrom
scottshambaugh:rcparam_eval

Conversation

@scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Mar 6, 2026

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

@scottshambaugh scottshambaugh added this to the v3.11.0 milestone Mar 6, 2026
@scottshambaugh scottshambaugh added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Mar 6, 2026
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Please add some tests.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Mar 7, 2026

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.

ValueError),
("cycler('c', [j.\u000c__class__(j) for j in ['r', 'b']])",
ValueError),
("cycler('c', [j.__class__(j).lower() for j in ['r', 'b']])",
Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

I think that’s ok. I consider the cycler spec in matplotlibrc as purely declarative. You should write out the lists explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

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')
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@scottshambaugh scottshambaugh Mar 9, 2026

Choose a reason for hiding this comment

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

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

https://docs.python.org/3/library/ast.html#ast.Expression

Copy link
Member

Choose a reason for hiding this comment

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

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).

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)}")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@scottshambaugh scottshambaugh Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@scottshambaugh scottshambaugh Mar 9, 2026

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: rcparams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

X Tutup