Skip to content

/cpython

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

bpo-33529: Fix infinite loop in email header encoding #12020

Merged
merged 1 commit into from May 14, 2019
Merged

bpo-33529: Fix infinite loop in email header encoding #12020

merged 1 commit into from May 14, 2019

Conversation

@wojcikk2903
Copy link
Contributor

wojcikk2903 commented Feb 24, 2019 •
edited

Prevents the fold function from entering infinite loop when there are enough non-ASCII characters in the word to encode.
The issue was caused by strings being treated as in Python 2. This PR is related to the previous attempt in #7763 by @corona10 .

https://bugs.python.org/issue33529
@wojcikk2903 wojcikk2903 requested a review from python/email-team as a code owner Feb 24, 2019
@the-knights-who-say-ni

This comment has been minimized.

Sign in to view
Copy link

the-knights-who-say-ni commented Feb 24, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!
@wojcikk2903
wojcikk2903 reviewed Feb 24, 2019
View changes
Lib/test/test_email/test_headerregistry.py
' =?utf-8?q?abuse=2Ephp=3Fmid=3Dxxx-xxx-xxxx'
'xxxxxxxxxxxxxxxxxxxx=3D=3D-xxx-?=\n'
' =?utf-8?q?xx-xx=3E?=\n')
'com/report=5Fabuse?=\n'

This comment has been minimized.

Sign in to view
Copy link
@wojcikk2903

wojcikk2903 Feb 24, 2019

Author Contributor

Expected result was modified because the previous algorithms tended not to use all available space in the initial encoded words.
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented Feb 25, 2019

@bitdancer, @warsaw: Would you mind to have a look at this fix for a security issue in the email module?
@pganssle
pganssle reviewed Apr 8, 2019
View changes
Lib/test/test_email/test_policy.py Outdated
@@ -237,6 +237,12 @@ def test_adding_default_policies_preserves_default_factory(self):
email.policy.EmailPolicy.header_factory)
self.assertEqual(newpolicy.__dict__, {'raise_on_defect': True})

def test_non_ascii_chars_do_not_cause_inf_loop(self):
policy = email.policy.default + email.policy.strict
msg = email.message.EmailMessage()

This comment has been minimized.

Sign in to view
Copy link
@pganssle

pganssle Apr 8, 2019

Member

I think this doesn't need to be an EmailMessage, you can trigger this with just:

policy = email.policy.default
policy.fold('Subject', 'ą' * 100)

This comment has been minimized.

Sign in to view
Copy link
@wojcikk2903

wojcikk2903 Apr 10, 2019

Author Contributor

Thanks for the suggestion. Indeed, it's engough to just define the policy.
Lib/test/test_email/test_policy.py Outdated
policy = email.policy.default + email.policy.strict
msg = email.message.EmailMessage()
msg['Subject'] = 'ą'*100
policy.fold('Subject', msg['Subject'])

This comment has been minimized.

Sign in to view
Copy link
@pganssle

pganssle Apr 8, 2019

Member

While you are writing the test, you may want to make an assertion about the output of this.

Perhaps it will be easier to write an assertion if you set the max_line_length to be shorter, so something like:

policy = email.policy.default.clone(max_line_length=20)
actual = policy.fold("Subject", "ą" * 21)                                                                         
self.assertEqual(actual,
                 'Subject: \n ' +
                 '=?utf-8?q?ąąąąąąą?=\n ' +
                 '=?utf-8?q?ąąąąąąą?=\n ' +
                 '=?utf-8?q?ąąąąąąą?=\n')

This comment has been minimized.

Sign in to view
Copy link
@wojcikk2903

wojcikk2903 Apr 10, 2019

Author Contributor

I tested the equality following your suggestion.
Misc/NEWS.d/next/Security/2019-02-24-18-48-16.bpo-33529.wpNNBD.rst Outdated
@@ -0,0 +1,2 @@
Prevent fold function used in email header encoding from entering infinite
loop when there are enough non-ASCII characters in a header.

This comment has been minimized.

Sign in to view
Copy link
@pganssle

pganssle Apr 8, 2019

Member
Suggested change
loop when there are enough non-ASCII characters in a header.
loop when there are too many non-ASCII characters in a header.
@pganssle pganssle mentioned this pull request Apr 8, 2019
Merged
@wojcikk2903
bpo-33529: Fix infinite loop in email header encoding
Loading status checks…
0883c36
@wojcikk2903 wojcikk2903 force-pushed the wojcikk2903:bpo-33529 branch from 19f6adc to 0883c36 Apr 10, 2019
@msapiro
msapiro approved these changes May 7, 2019
View changes
Copy link

msapiro left a comment

At the request of @pganssle I have reviewed the changes in this PR and I agree that they are an appropriate fix.
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented May 10, 2019

cc @bitdancer @warsaw
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member

corona10 commented May 10, 2019

Great!
@vstinner vstinner merged commit c1f5667 into python:master May 14, 2019
5 checks passed
5 checks passed
Azure Pipelines PR #20190410.33 succeeded
Details
bedevere/issue-number Issue number 33529 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented May 14, 2019

Thanks @wojcikk2903 for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
miss-islington added a commit to miss-islington/cpython that referenced this pull request May 14, 2019
@wojcikk2903 @miss-islington
bpo-33529, email: Fix infinite loop in email header encoding (pythonG…
0f06d47 
…H-12020)

(cherry picked from commit c1f5667)

Co-authored-by: Krzysztof Wojcik <wojcikk2903@users.noreply.github.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented May 14, 2019

GH-13321 is a backport of this pull request to the 3.7 branch.
@vstinner

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented May 14, 2019

I merged your PR, thanks @wojcikk2903. Thanks @msapiro and @pganssle for your reviews.
vstinner added a commit that referenced this pull request May 14, 2019
@miss-islington @wojcikk2903 @vstinner
bpo-33529, email: Fix infinite loop in email header encoding (GH-12020)…
Loading status checks…
2fef5b0 
… (GH-13321)

(cherry picked from commit c1f5667)

Co-authored-by: Krzysztof Wojcik <wojcikk2903@users.noreply.github.com>
@vstinner vstinner mentioned this pull request May 14, 2019
Closed
@csabella csabella mentioned this pull request Jun 5, 2019
Closed
vstinner added a commit to vstinner/cpython that referenced this pull request Jun 17, 2019
@wojcikk2903 @vstinner
bpo-33529, email: Fix infinite loop in email header encoding (pythonG…
2f2806a 
…H-12020)

(cherry picked from commit c1f5667)
ned-deily added a commit that referenced this pull request Jun 18, 2019
@vstinner @ned-deily
bpo-33529, email: Fix infinite loop in email header encoding (GH-12020)…
Loading status checks…
516a6a2 
… (GH-14162)

(cherry picked from commit c1f5667)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pganssle pganssle

@msapiro msapiro

@python/email-team email-team

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
8 participants
@wojcikk2903 @the-knights-who-say-ni @vstinner @corona10 @miss-islington @bedevere-bot @pganssle @msapiro
You can’t perform that action at this time.