A feeble attempt to plug a theoretical security hole #6274
Conversation
| # 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" |
There was a problem hiding this comment.
This should be put outside the try or it will be caught and re-raised with a different message below.
There was a problem hiding this comment.
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.
|
Travis failure doesn't have anything to do with what I am doing here, but doesn't seem transient. I restarted it just in case anyway. |
|
With the exception of the "cycler(" part at the beginning, I think most of these expressions are actually just JSON, aren't they? Maybe we could detect/strip the prefix and then use JSON to parse the rest, which would be much safer. |
|
We allow binary operations, too, in order to gain the full features of On Thu, Apr 7, 2016 at 2:16 PM, Michael Droettboom <notifications@github.com
|
|
But are those binary operations used in matplotlibrc files? |
|
Not in the ones we package, but anyone else can do that, and we do On Thu, Apr 7, 2016 at 2:46 PM, Michael Droettboom <notifications@github.com
|
|
I have a super rough pass at this on a computer someplace, but it is at least a few days of work (well, for me, might only take @mdboom an hour 😉 ). In the pyparsing docs there is a 4 function calculator I was using as a template. |
|
Nothing takes an hour with PyParsing ;) |
I introduced a security hole when I implemented the property cycle support last year. I should have read this blog posting: http://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html that would have scared me off of the current implementation.
What this PR does is tries to plug two possible holes. First, it neuters
__builtins__, which is a good step, but as Ned points out in his post, it is insufficient. Second, I reject any cycler strings that have the substring of'.__'in it, as it could be a dunder method access, which is key to the other holes that Ned demonstrates. I don't like that because I can see possible valid uses of that combo of three characters in a property cycler.@tacaswell and I have discussed some future directions to fix this permanently that should go into v2.0. Essentially, use PyParsing and
ast.literal_eval()instead of the unsafeeval(). That would take some time, and given that v2.0 is still in the works, I would suggest releasing v1.5.2 as a security fix release.