Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

SEC: Remove eval() from validate_cycler#31248

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

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

Conversation

@scottshambaugh
Copy link
Contributor

@scottshambaughscottshambaugh commentedMar 6, 2026
edited
Loading

PR summary

validate_cycler() useseval() to parseaxes.prop_cycle rcParam strings. Combined with automatic matplotlibrc loading from the current working directory, this allows arbitrary code execution onimport 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

@scottshambaughscottshambaugh added this to thev3.11.0 milestoneMar 6, 2026
@scottshambaughscottshambaugh added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelMar 6, 2026
Copy link
Member

@timhoffmtimhoffm 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
ContributorAuthor

scottshambaugh commentedMar 7, 2026
edited
Loading

This was pretty well exercised withtest_validator_invalid already. I addedtest_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 thesuccess list however, so I'm not sure it'sintended 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.

scottshambaugh reacted with thumbs up emoji
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.

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

@scottshambaughscottshambaughMar 9, 2026
edited
Loading

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
ContributorAuthor

@scottshambaughscottshambaughMar 9, 2026
edited
Loading

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

cpython docs PR:python/cpython#145773

@WeatherGod
Copy link
Member

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?

@scottshambaugh
Copy link
ContributorAuthor

Unfortunately, my takeaway from what I've been reading about this is that it's essentially impossible to sandboxeval() by blocking lists of known exploits, and an allowlist of operations withast.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

@scottshambaugh
Copy link
ContributorAuthor

Added a deprecation notice.

@tacaswell
Copy link
Member

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.


def _eval_cycler_expr(node):
"""Recursively evaluate an AST node to build a Cycler object."""
if isinstance(node, ast.BinOp):
Copy link
Member

Choose a reason for hiding this comment

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

if I am reading this right we support the binary ops between any valid expressions?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, do you see any issues with that?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see an issue, but just thinking through exactly how locked down we can make it. For example, can we check that at least one side of this is a Cycler?



def _eval_cycler_expr(node):
"""Recursively evaluate an AST node to build a Cycler object."""
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to utilizeast.walk() to walk the tree nodes? It should help prevent any recursion errors or other oddities that can happen with trees.

and node.func.id in ('cycler', 'concat')):
raise ValueError(
"only the 'cycler()' and 'concat()' functions are allowed")
func = cycler if node.func.id == 'cycler' else cconcat
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func=cyclerifnode.func.id=='cycler'elsecconcat
func=ccyclerifnode.func.id=='cycler'elsecconcat

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The validation on each node withcycler is needed, tests fail withccycler.

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

Reviewers

@tacaswelltacaswelltacaswell left review comments

@WeatherGodWeatherGodWeatherGod left review comments

@QuLogicQuLogicQuLogic left review comments

@anntzeranntzeranntzer left review comments

@timhoffmtimhoffmtimhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

6 participants

@scottshambaugh@WeatherGod@tacaswell@QuLogic@anntzer@timhoffm

[8]ページ先頭

©2009-2026 Movatter.jp