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.

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

Jump to bottom

bpo-37977: Warn more strongly and clearly about pickle security #15595

Merged
merged 1 commit into from
Aug 31, 2019
Merged

bpo-37977: Warn more strongly and clearly about pickle security #15595

merged 1 commit into from
Aug 31, 2019

Conversation

lordmauve
Copy link
Contributor

@lordmauve lordmauve commented Aug 29, 2019 •
edited by bedevere-bot
Loading

Rewrite the red warning at the top of the pickle module documentation.

  • Simpler, more direct English.
  • Explain the severity of vulnerability that doing this will cause.
  • Link to the hmac module which can be used to prevent tampering.
  • Link to the json module which is safer if less powerful.

https://bugs.python.org/issue37977

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Aug 29, 2019
@lordmauve lordmauve force-pushed the big-red-pickle-warning branch 2 times, most recently from 41f327d to 07e1e09 Compare August 29, 2019 13:48
@pitrou
Copy link
Member

pitrou commented Aug 29, 2019

There is already a section titled "Comparison with json", so perhaps move the sentence stating json is secure there?

@lordmauve
Copy link
Contributor Author

Hmm, good point. I don't think we should move it there because it wouldn't form part of the security warning, but we should link to that section - and that section should state again that JSON doesn't have the arbitrary code execution issue.

rhettinger
rhettinger reviewed Aug 29, 2019
View reviewed changes
Doc/library/pickle.rst Outdated
Consider signing data with :mod:`hmac` if you need to ensure that it has not
been tampered with.

The JSON data interchange format provided by the :mod:`json` module is more
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend leaving this out. It goes well past giving an informative warning and moves into editorializing. Also, for many uses of pickle, JSON is no substitute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this sentence to be less discursive and simply point to the json module and the "Comparison with json" section lower down.

@rhettinger rhettinger self-assigned this Aug 29, 2019
@lordmauve
Warn more strongly and clearly about pickle security
ffa614e 
Mention also that JSON is not usually subject to ACE vulnerabilities,
and link to the Comparison with JSON section.
@lordmauve lordmauve force-pushed the big-red-pickle-warning branch from 07e1e09 to ffa614e Compare August 30, 2019 10:37
@rhettinger rhettinger merged commit daa82d0 into python:master Aug 31, 2019
@miss-islington
Copy link
Contributor

Thanks @lordmauve for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @lordmauve and @rhettinger, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker daa82d019c52e95c3c57275307918078c1c0ac81 3.8

@miss-islington
Copy link
Contributor

Thanks @lordmauve for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-15629 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 31, 2019
@lordmauve @miss-islington
bpo-37977: Warn more strongly and clearly about pickle security (pyth…
7b30123 
…onGH-15595)

(cherry picked from commit daa82d0)

Co-authored-by: Daniel Pope <[email protected]>
rhettinger pushed a commit that referenced this pull request Aug 31, 2019
@miss-islington @lordmauve @rhettinger
bpo-37977: Warn more strongly and clearly about pickle security (GH-1…
6922b9e 
…5595) (GH-15629)

(cherry picked from commit daa82d0)

Co-authored-by: Daniel Pope <[email protected]>
@lordmauve
Copy link
Contributor Author

Thanks, everyone!

@lordmauve lordmauve deleted the big-red-pickle-warning branch August 31, 2019 07:40
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@lordmauve @lisroach
bpo-37977: Warn more strongly and clearly about pickle security (pyth…
a2ed2a8 
…onGH-15595)
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@lordmauve @DinoV
bpo-37977: Warn more strongly and clearly about pickle security (pyth…
9e33c10 
…onGH-15595)
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@lordmauve @websurfer5
bpo-37977: Warn more strongly and clearly about pickle security (pyth…
fdda93f 
…onGH-15595)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@disconnect3d disconnect3d disconnect3d left review comments

@moreati moreati moreati left review comments

@rhettinger rhettinger rhettinger approved these changes

@pitrou pitrou Awaiting requested review from pitrou

@tiran tiran Awaiting requested review from tiran

Assignees

@rhettinger rhettinger

Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lordmauve @pitrou @miss-islington @bedevere-bot @moreati @rhettinger @disconnect3d @the-knights-who-say-ni