Rephrase ast.literal_eval() to remove any security warranty

[vstinner]

Currently, 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.

[vstinner]

cc @gvanrossum @pablogsal

[njsmith]

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?

[tiran]

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]

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]

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

[vstinner]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[gvanrossum]

How infeasible is it on the most common platforms (Linux, Windows, macOS, various BSD) to introspect the stack size? We're at least partially in this mess because back in 1990 I didn't know how to do that using portable C code, but hopefully things have improved since then, and there is at least a non-portable way? At least for the parser a single pointer compare shouldn't break the bank.

[pablogsal]

At least for the parser a single pointer compare shouldn't break the bank.

At least in Linux we can ask the OS for the stack limit and then we could compare addresses as they cannot move. I can try to implement a prototype of this.

[Kodiologist]

This seems like more of a semantics issue than anything else. I for one see no contradiction in the original documentation: the kind of "safety" being offered is "not executing arbitrary code", not "never crashing". This is sufficient for untrusted input in many situations. Making sure a function never crashes, even with pathological input, is a tall order. I can see the value in being able to make this additional guarantee, but in the meantime, the original issue can be resolved simply by wording the existing guarantee more explicitly.

[gpshead]

More explicit wording added.

[vstinner]

Fixed by PR #95919. Thank you!

[seberg]

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]

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]

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]

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/