GeezerVison is showing github.com — Rephrase ast.literal_eval() to remove any security warranty · Issue #95588 · python/cpython
Rephrase ast.literal_eval() to remove any security warranty #95588DescriptionCurrently, ast.literal_eval() documentation gives multiple security warranties:
- Safely evaluate
- This can be used for safely evaluating strings containing Python values from untrusted sources
IMO that's plain wrong if you read the following RED WARNING:
It is possible to crash the Python interpreter (...)
The documentation should be rephrased to only described the purpose of the function and make it very clear that it must NOT be used on untrusted sources.
We can follow the phrasing of the pickle documentation: https://docs.python.org/dev/library/pickle.html
The pickle module is not secure. Only unpickle data you trust.
Linked PRs
Activityvstinner commented on Aug 3, 2022 njsmith commented on Aug 3, 2022 Comparing literal_eval to unpickle seems really misleading... unpickling is straight up arbitrary code execution, which is a completely different threat than "might cause a crash".
Also, I've seen lots of folks use literal_eval on untrusted data, because that's always been its whole reason for existence. (If you have trusted data, you can just eval :-).) So I don't think just yanking that guarantee out from under people's feet is likely to fly with the community... it's effectively a major backwards-incompatible change.
If you're really worried about this, maybe it should get a max_input_length=10000 argument or something, or whatever would make it safe enough for you? Do we even know whether it's actually unsafe right now? Reacted by Arno Gobbin and Nikolaus Hansen tiran commented on Aug 3, 2022 We could remove the safety warranties from our documentation, but we cannot drop the security promise for Python 3.11 and earlier. The API was documented as safe when Python 3.10 and earlier came out. We cannot just void this promise mid-air. tiran commented on Aug 3, 2022
If you're really worried about this, maybe it should get a max_input_length=10000 argument or something, or whatever would make it safe enough for you? Do we even know whether it's actually unsafe right now?
I made a similar suggestion in our internal chat. We may have to restrict both input string length and amount of AST nodes. Py_CompileString* would need to track nodes with an internal counter. pablogsal commented on Aug 3, 2022
If you're really worried about this, maybe it should get a max_input_length=10000 argument or something, or whatever would make it safe enough for you? Do we even know whether it's actually unsafe right now?
I made a similar suggestion in our internal chat. We may have to restrict both input string length and amount of AST nodes. Py_CompileString* would need to track nodes with an internal counter.
I am generally opposed to that kind of thing because is very difficult to predict correctly what the effect will be. For example, the parser creates AST nodes on the go as it parses incorrect constructs and those will not be ultimately used unless the end in a cache. What is worse, changing the grammar will have very visible effects so something that didn't crash before now will reach the limit.
Given how tricky is to maintain the parser in general I advice strongly against more complexity than we already have, which is a lot.
Every special mode of the parser can be a pain to maintain when extending error messages or doing more grammar rules Reacted by Gregory P. Smith and Zac Hatfield-Dodds vstinner commented on Aug 3, 2022
So I don't think just yanking that guarantee out from under people's feet is likely to fly with the community... it's effectively a major backwards-incompatible change.
The documentation is wrong, misleading and should be fixed. A function which is known to crash must not be used with an untrusted string.
IMO making literal_eval() safer is a new and separated feature request. If you have a concrete idea how to make literal_eval() safer, please open a separated issue.
Also, fixing a bunch of parser issues is always welcomed, but it's also a separated issue. vstinner commented on Aug 3, 2022
IMO making literal_eval() safer is a new and separated feature request. If you have a concrete idea how to make literal_eval() safer, please open a separated issue.
See issue #83340 for example. tiran commented on Aug 3, 2022
The documentation is wrong, misleading and should be fixed. A function which is known to crash must not be used with an untrusted string.
The documentation is a promise to our users. We must do our best to solve the problem -- at least for common and simple cases. Just dropping the security promise from the documentation is blame shifting. It does not help our user base who is already using the feature. nascheme commented on Aug 4, 2022 Writing a more minimal expression parser as Raymond suggests is likely the best way to make literal_eval() safe. As for a promise to our users, there are some real challenges fulfilling that and it's best to be honest if we can't fulfill it. E.g. we probably can't fix literal_eval() to be 100% bulletproof in bug fix release. OTOH, I think it's a fairly heavily used feature so probably we should try to fix it. Even though the minimal expression parser likely adds a fair bit of maintenance work. seberg commented on Aug 12, 2022 Is there no way to figure out an acceptable solution complexity wise? Or just to build more confidence in what we have? Maybe by adding a fuzzer test for it (there seems to be a test for json here, which is probably quite similar in many ways).
A reduced complexity/more minimal expression parser version seems best in principle, but I don't know how feasible it is. gpshead commented on Aug 12, 2022 We do fuzz test ast.literal_eval via Google's oss-fuzz. The entry point is https://github.com/python/cpython/blob/main/Modules/_xxtestfuzz/fuzzer.c#L396. It has revealed numerous compiler and parser issues that have been fixed. But is often stuck re-triggering known non-trivial issues such as stack overflows and computational timeouts at this point.
"The" problem with writing a minimal expression parser to replace literal_eval's implementation is the complexity. literal_eval does a lot more than I suspect many users actually need or understand as the range of things Python literals encompass is large and infinitely nested.
Ex: {(15, (9.25e-16+9j)): '''str''', "8": [{(),}, ..., None, False, ()]} is a Python literal. Did your program really need to be able to accept that as input? I doubt it.
We could remove the safety warranties from our documentation, but we cannot drop the security promise for Python 3.11 and earlier. The API was documented as safe when Python 3.10 and earlier came out. We cannot just void this promise mid-air.
We can and must. Because we'll never fix literal_eval in anything acceptable to backport to a release branch. Doing that requires an entire new non-recursive not Python compiler based implementation. The bug we can fix is in our documentation which claims it is safe to crash the process by parsing untrusted data. It isn't. seberg commented on Aug 12, 2022 I doubt many will need the full power here, but things like dicts containing a list maybe, in fact a dict with a list containing tuples is the exact, untrusted, use-case, I have.
If you update the docs, maybe mention json which has support for a large range of use-cases? For the use-case I have in mind, unfortunaely supporting tuples literals rather than only lists is the issue that json does not cover. njsmith commented on Aug 12, 2022 So the problem with literal_eval is entirely with the parser, right? the "eval" part looks pretty trivial/safe AFAICT? I think literal_eval is kind of a red herring... the much more important question is whether we document ast.parse as performing arbitrary code execution. Because tons of things use ast.parse besides literal_eval.
Concretely: suppose you have an IDE where whenever you open a .py file, it does some static introspection, part of which involves ast.parse. Is this a supported use case? Or does it need to be handled with the same radioactive tongs as unpickling and eval? gpshead commented on Aug 12, 2022 Likely? ast.parse and ast.literal_eval both crash on the same thing with a stack overflow:
$ ulimit -s 256
$ python
Python 3.10.5 (main, Jun 8 2022, 09:26:22) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> n = 200
>>> def s():
... return '(' * n + '0' + ',)' * n
...
>>> import ast
>>> ast.parse(s())
Segmentation fault
Under a more common default stack size or >= 2048KiB our internal checks for too much nesting being > 200 fire and lead to a SyntaxError: too many nested parentheses instead. So many of these crashes are likely prevented in common environments but we don't do enough to make a guarantee.
We cannot control the C stack size - and introspecting it is not always plausible - so we cannot make guarantees so long as we're C stack recursive. #91079 is related to helping out here.
Fundamentally this is a problem with using recursion in a language that doesn't have a dynamic stack (read: C and C++). Input based recursion is not wise for parsing untrusted inputs in such an environment. gpshead commented on Oct 2, 2022 More explicit wording added. vstinner commented on Oct 3, 2022 Fixed by PR #95919. Thank you! seberg commented on Oct 4, 2022 Are there any hints on a workaround(s)? The only thing I can think of right now is just rejecting "large" inputs (not sure how large). Which is still a bit inconvenient in my use-case (which at some time explicitly wanted to allow them). Rejecting deep nesting, or very many nodes would seem a bit clearer.
I can of course do that, and maybe there is just no avoiding it anyway, but "advertising" it, e.g. by having a kwarg to opt-in might be good? vstinner commented on Oct 4, 2022
Are there any hints on a workaround(s)?
In my experience, the most secure way to evaluate/run untrusted code is to spawn a separated process and runs this process in a sandbox where you can limit time, memory, syscalls, etc. seberg commented on Oct 4, 2022 Yea, but whether or not it helps me, it seem like there should be some practical solution for very simple stuff here.
- Nobody wants to sand-box evaluating a single floating point number. So there is some "threshold" of complexity that should be safe. Maybe using
json actually helps increase it (not for me, but generally)? (json also warns about sizes, but maybe it is more graceful in the sense that there is no sudden crash, just DoS through linear resource usage increase?).
- If I use literal-eval as a library, it seems strange if I sandbox for the user (can I even do that)? But, I also cannot reasonably force everyone to sandbox. So I must add some arbitrary threshold beyond which I tell users: "Oh, maybe be careful, if you are worried you probably have to sandbox."
I would be surprised if I am the only one in a position where historically literal_eval was assumed to be safe (maybe sloppily so) and removing that guarantee will be bumpy. vstinner commented on Oct 4, 2022
I would be surprised if I am the only one in a position where historically literal_eval was assumed to be safe (maybe sloppily so) and removing that guarantee will be bumpy.
It was never safe. The only change is the documentation that has been fixed. One way to reduce the risk of crash is to reject strings longer than a limit (ex: 100 characters).
This issue is closed. I suggest you opening a discussion elsewhere. There is no plan to implement a sandbox in CPython: https://lwn.net/Articles/574215/
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|