Skip to content

A feeble attempt to plug a theoretical security hole #6274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 17, 2016
Merged

A feeble attempt to plug a theoretical security hole #6274

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions lib/matplotlib/rcsetup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,17 +746,16 @@ def validate_cycler(s):
# might come from the internet (future plans), this
# could be downright dangerous.
# I locked it down by only having the 'cycler()' function
# available. Imports and defs should not
# be possible. However, it is entirely possible that
# a security hole could open up via attributes to the
# function (this is why I decided against allowing the
# Cycler class object just to reduce the number of
# degrees of freedom (but maybe it is safer to use?).
# One possible hole I can think of (in theory) is if
# someone managed to hack the cycler module. But, if
# someone does that, this wouldn't make anything
# worse because we have to import the module anyway.
s = eval(s, {'cycler': cycler})
# available.
# UPDATE: Partly plugging a security hole.
# I really should have read this:
# http://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html
# We should replace this eval with a combo of PyParsing and
# ast.literal_eval()
if '.__' in s.replace(' ', ''):
raise ValueError("'%s' seems to have dunder methods. Raising"
Copy link
Member

Choose a reason for hiding this comment

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

This should be put outside the try or it will be caught and re-raised with a different message below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought about that, but I would rather keep all of this logic and comments together in one place. The re-raise includes this exception message, so it isn't like it is getting lost in the shuffle. All of this should be getting replaced anyway come v2.0.

" an exception for your safety")
s = eval(s, {'cycler': cycler, '__builtins__': {}})
except BaseException as e:
raise ValueError("'%s' is not a valid cycler construction: %s" %
(s, e))
Expand Down