Skip to content
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.

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

gh-95588: Drop the safety claim from ast.literal_eval docs. #95919

Merged
merged 4 commits into from
Oct 2, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Aug 12, 2022

It was never safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.

Making it safe is not a mere bugfix but an entire rewrite, we're never going to live up to the safety claim via a bugfix and people with existing releases will always have versions around that are unsafe even if we did. So making that claim is bad for our users.

It was never safe and this claim conflicts directly with the big
warning in the docs about it being able to crash the interpreter.
@gpshead gpshead added docs Documentation in the Doc dir needs backport to 3.7 needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.11 only security fixes labels Aug 12, 2022
@gpshead gpshead requested review from vstinner and isidentical and removed request for isidentical August 12, 2022 08:35
@gpshead gpshead self-assigned this Aug 12, 2022
@njsmith
Copy link
Contributor

njsmith commented Aug 12, 2022

Can we state clearly that maliciously large/complex inputs can cause a DoS (process crash or slow execution time), but nothing worse than that? That's a useful guarantee, and not the same as what most people think of when they think "unsafe for untrusted input".

(Can it even cause unbounded execution time? with the new parser, shouldn't literal_eval be O(n) in time and space?)

@gpshead
Copy link
Member Author

gpshead commented Aug 12, 2022

It only takes 200 bytes to crash the process so it isn't even about being what I'd call large. (which is why i removed the word "large" from the warning)

@gpshead
Copy link
Member Author

gpshead commented Aug 12, 2022

i'm weary of guaranteeing it doesn't lead to arbitrary code execution. I believe that is the intent and might be true, and any example that allowed that is one we'd care about, but it is hard to claim as the code this uses for parsing wasn't designed with untrusted input in mind because of the rhetorical "who even does that to a compiler?" question.

so the wording i have here tries to explain that it isn't an arbitrary python code execution function. but doesn't specifically say it cannot lead to other things, just that we know you can get memory exhaustion and C stack overflows.

@gpshead
Copy link
Member Author

gpshead commented Aug 12, 2022

unbounded time is possible but i can't get into the details of that in public right now. at input sizes too small to cause a stack overflow I do not believe you'll run into time issues.

Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
@gpshead gpshead removed type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.7 needs backport to 3.9 only security fixes labels Oct 1, 2022
@gpshead gpshead merged commit 8baef8a into python:main Oct 2, 2022
@miss-islington
Copy link
Contributor

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@gpshead gpshead deleted the gh95588-ast-doc branch October 2, 2022 00:55
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 2, 2022
@bedevere-bot
Copy link

GH-97714 is a backport of this pull request to the 3.11 branch.

@bedevere-bot
Copy link

GH-97715 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 2, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
…ythonGH-95919)

It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
(cherry picked from commit 8baef8a)

Co-authored-by: Gregory P. Smith <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
…ythonGH-95919)

It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
(cherry picked from commit 8baef8a)

Co-authored-by: Gregory P. Smith <[email protected]>
miss-islington added a commit that referenced this pull request Oct 2, 2022
It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
(cherry picked from commit 8baef8a)

Co-authored-by: Gregory P. Smith <[email protected]>
miss-islington added a commit that referenced this pull request Oct 2, 2022
It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
(cherry picked from commit 8baef8a)

Co-authored-by: Gregory P. Smith <[email protected]>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
…ython#95919)

It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
carljm added a commit to carljm/cpython that referenced this pull request Oct 3, 2022
* main: (2069 commits)
  pythongh-96512: Move int_max_str_digits setting to PyConfig (python#96944)
  pythongh-94808: Coverage: Check picklablability of calliter (python#95923)
  pythongh-94808: Add test coverage for PyObject_HasAttrString (python#96627)
  pythongh-94732: Fix KeyboardInterrupt race in asyncio run_forever() (python#97765)
  Fix typos in `bltinmodule.c`. (pythonGH-97766)
  pythongh-94808: `_PyLineTable_StartsLine` was not used (pythonGH-96609)
  pythongh-97681: Remove Tools/demo/ directory (python#97682)
  Fix typo in unittest docs (python#97742)
  pythongh-97728: Argument Clinic: Fix uninitialized variable in the Py_UNICODE converter (pythonGH-97729)
  pythongh-95913: Fix PEP number in PEP 678 What's New ref label (python#97739)
  pythongh-95913: Copyedit/improve New Modules What's New section (python#97721)
  pythongh-97740: Fix bang in Sphinx C domain ref target syntax (python#97741)
  pythongh-96819: multiprocessing.resource_tracker: check if length of pipe write <= 512 (python#96890)
  pythongh-97706: multiprocessing tests: Delete unused variable `rand` (python#97707)
  pythonGH-85447: Clarify docs about awaiting future multiple times (python#97738)
  [docs] Update logging cookbook with recipe for using a logger like an output… (pythonGH-97730)
  pythongh-97607: Fix content parsing in the impl-detail reST directive (python#97652)
  pythongh-95975: Move except/*/finally ref labels to more precise locations (python#95976)
  pythongh-97591: In `Exception.__setstate__()` acquire strong references before calling `tp_hash` slot (python#97700)
  pythongh-95588: Drop the safety claim from `ast.literal_eval` docs. (python#95919)
  ...
@skirpichev
Copy link
Member

skirpichev commented Nov 12, 2024

Lets try to backport this on 3.9 (see #126711).

@skirpichev skirpichev added the needs backport to 3.9 only security fixes label Nov 12, 2024
@miss-islington-app
Copy link

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 12, 2024
…ythonGH-95919)

It was never really safe and this claim conflicts directly with the big warning in the docs about it being able to crash the interpreter.
(cherry picked from commit 8baef8a)

Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 12, 2024

GH-126729 is a backport of this pull request to the 3.9 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.9 only security fixes label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants