bpo-39073: validate Address parts to disallow CRLF by epicfaace · Pull Request #19007 · python/cpython

epicfaace · 2020-03-15T01:28:01Z

Validate email.headerregistry.Address to disallow CRLF in address parts (username, domain, display_name)

https://bugs.python.org/issue39073

bitdancer

Thanks for the PR.

Lib/email/headerregistry.py

Lib/test/test_email/test_headerregistry.py

bedevere-bot · 2020-03-16T00:59:33Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…ddress

epicfaace

I have made the requested changes; please review again

epicfaace · 2020-03-16T13:34:25Z

I have made the requested changes; please review again

bedevere-bot · 2020-03-16T13:34:28Z

Thanks for making the requested changes!

@bitdancer: please review the changes made to this pull request.

bitdancer · 2020-03-16T14:44:29Z

Lib/email/headerregistry.py

+
+        inputs = ''.join(filter(None, (display_name, username, domain, addr_spec)))
+        if '\r' in inputs or '\n' in inputs:
+            raise ValueError("invalid inputs; address parts cannot contain CR / LF")


Hmm. Reading this I think I'd say "arguments" rather than inputs, that aligns better with our typical vocabulary. And how about "CR or LF"?

Lib/test/test_email/test_headerregistry.py

bedevere-bot · 2020-03-16T14:47:24Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

epicfaace · 2020-03-16T14:52:16Z

I have made the requested changes; please review again

bedevere-bot · 2020-03-16T14:52:19Z

Thanks for making the requested changes!

@bitdancer: please review the changes made to this pull request.

bitdancer

Except for the news item text this looks good.

bitdancer · 2020-03-16T17:41:06Z

Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst

@@ -0,0 +1 @@
+Validate email.headerregistry.Address to disallow CRLF in address parts (username, domain, display_name).


I thought I'd already made this comment but I can't find it:

"DIsallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks."

bedevere-bot · 2020-03-30T00:38:44Z

@bitdancer: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2020-03-30T00:38:44Z

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

Disallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks. (cherry picked from commit 614f172) Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>

bedevere-bot · 2020-03-30T00:39:13Z

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

Disallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks. (cherry picked from commit 614f172) Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>

bedevere-bot · 2020-03-30T00:39:23Z

GH-19223 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2020-03-30T00:39:31Z

GH-19224 is a backport of this pull request to the 3.6 branch.

Disallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks. (cherry picked from commit 614f172) Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>

Disallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks. (cherry picked from commit 614f172) Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com> Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>

Disallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks. (cherry picked from commit 614f172)

validate address parts

38d0a15

epicfaace requested a review from a team as a code owner March 15, 2020 01:28

the-knights-who-say-ni added the CLA signed label Mar 15, 2020

bedevere-bot added the awaiting review label Mar 15, 2020

📜🤖 Added by blurb_it.

d8424e5

bitdancer requested changes Mar 16, 2020

View reviewed changes

Lib/email/headerregistry.py Outdated Show resolved Hide resolved

Lib/test/test_email/test_headerregistry.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting review labels Mar 16, 2020

epicfaace added 2 commits March 16, 2020 13:29

do validation at once, on \r and \n, refactor tests

de6ef8c

Merge branch 'val-address' of github.com:epicfaace/cpython into val-a…

950bd01

…ddress

epicfaace commented Mar 16, 2020

View reviewed changes

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 16, 2020

bedevere-bot requested a review from bitdancer March 16, 2020 13:34

rm extra spacing

6cac3a0

bitdancer requested changes Mar 16, 2020

View reviewed changes

bedevere-bot removed the awaiting change review label Mar 16, 2020

bedevere-bot added the awaiting changes label Mar 16, 2020

epicfaace added 2 commits March 16, 2020 14:49

Add display_name test

d10d383

update message

81a0231

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 16, 2020

bedevere-bot requested a review from bitdancer March 16, 2020 14:52

bitdancer requested changes Mar 16, 2020

View reviewed changes

bedevere-bot requested a review from bitdancer March 16, 2020 18:33

bitdancer approved these changes Mar 30, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Mar 30, 2020

bitdancer added needs backport to 3.6 type-security A security issue labels Mar 30, 2020

bitdancer merged commit 614f172 into python:master Mar 30, 2020

bedevere-bot removed the awaiting merge label Mar 30, 2020

bedevere-bot removed the needs backport to 3.8 label Mar 30, 2020

bedevere-bot removed the needs backport to 3.7 label Mar 30, 2020

bedevere-bot removed the needs backport to 3.6 label Mar 30, 2020

epicfaace deleted the val-address branch May 27, 2020 15:11

		@@ -0,0 +1 @@
		Validate email.headerregistry.Address to disallow CRLF in address parts (username, domain, display_name). No newline at end of file

Uh oh!

Conversation

epicfaace commented Mar 15, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitdancer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Mar 16, 2020

Uh oh!

epicfaace left a comment

Choose a reason for hiding this comment

Uh oh!

epicfaace commented Mar 16, 2020

Uh oh!

bedevere-bot commented Mar 16, 2020

Uh oh!

bitdancer Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Mar 16, 2020

Uh oh!

epicfaace commented Mar 16, 2020

Uh oh!

bedevere-bot commented Mar 16, 2020

Uh oh!

bitdancer left a comment

Choose a reason for hiding this comment

Uh oh!

bitdancer Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 30, 2020

Uh oh!

miss-islington commented Mar 30, 2020

Uh oh!

bedevere-bot commented Mar 30, 2020

Uh oh!

bedevere-bot commented Mar 30, 2020

Uh oh!

bedevere-bot commented Mar 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

epicfaace commented Mar 15, 2020 •

edited by bedevere-bot

Loading