Skip to content

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

Merged
rhettinger merged 1 commit into
python:masterfrom
lordmauve:big-red-pickle-warning
Aug 31, 2019
Merged

bpo-37977: Warn more strongly and clearly about pickle security#15595
rhettinger merged 1 commit into
python:masterfrom
lordmauve:big-red-pickle-warning

Conversation

@lordmauve

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

Copy link
Copy Markdown
Contributor

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

pitrou commented Aug 29, 2019

Copy link
Copy Markdown
Member

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

@lordmauve

lordmauve commented Aug 29, 2019

Copy link
Copy Markdown
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.

disconnect3d
disconnect3d reviewed Aug 29, 2019
View reviewed changes
Comment thread Doc/library/pickle.rst Outdated
rhettinger
rhettinger reviewed Aug 29, 2019
View reviewed changes
Comment thread Doc/library/pickle.rst Outdated

@rhettinger rhettinger Aug 29, 2019

Copy link
Copy Markdown
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.

@lordmauve lordmauve Aug 30, 2019

Copy link
Copy Markdown
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

miss-islington commented Aug 31, 2019

Copy link
Copy Markdown
Contributor

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

@miss-islington

miss-islington commented Aug 31, 2019

Copy link
Copy Markdown
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

miss-islington commented Aug 31, 2019

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

bedevere-bot commented Aug 31, 2019

Copy link
Copy Markdown

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 <lordmauve@users.noreply.github.com>
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 <lordmauve@users.noreply.github.com>
@lordmauve

lordmauve commented Aug 31, 2019

Copy link
Copy Markdown
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

@rhettinger rhettinger rhettinger approved these changes
@pitrou pitrou Awaiting requested review from pitrou
@tiran tiran Awaiting requested review from tiran
+2 more reviewers
@moreati moreati moreati left review comments
@disconnect3d disconnect3d disconnect3d left review comments
Reviewers whose approvals may not affect merge requirements

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