GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-34155: Dont parse domains containing @ by jpic · Pull Request #13079 · python/cpython

Conversation

@jpic

@jpic jpic commented May 3, 2019 •
edited by miss-islington
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor 
Before:

    >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
    (Address(display_name='', username='a', domain='malicious.org'),)

    >>> parseaddr('[email protected]@important.com')
    ('', '[email protected]')

After:

    >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
    (Address(display_name='', username='', domain=''),)

    >>> parseaddr('[email protected]@important.com')
    ('', 'a@')

https://bugs.python.org/issue34155

Automerge-Triggered-By: @warsaw

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
neofutur and bsolomon1124 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@jpic
jpic requested a review from a team as a code owner May 3, 2019 21:27
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label May 3, 2019
@the-knights-who-say-ni

the-knights-who-say-ni commented May 3, 2019

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting review label May 3, 2019
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 4, 2019
@jpic
jpic force-pushed the bpo-34155 branch 3 times, most recently from 41c6fe8 to 1b6269b Compare May 4, 2019 01:16
@jpic jpic changed the title bpo-34155: Check for conflicting atomends in parseaddr bpo-34155: Dont parse domains containing @ May 4, 2019
@jpic
jpic force-pushed the bpo-34155 branch from 1b6269b to 41287e8 Compare May 4, 2019 01:32
maxking
maxking reviewed May 31, 2019
View reviewed changes

@maxking maxking left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for the patch!

I have some different ideas on how to do error recovery in this case, please see my comment on bpo-34155.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/email/_header_value_parser.py Outdated

@maxking maxking May 31, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd rename the error as Invalid Domain.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Security/2019-05-04-13-33-37.bpo-34155.MJll68.rst Outdated

@maxking maxking May 31, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would suggest a slightly different wording for this:

Fix parsing of invalid email addresses with more than one ``@`` (e.g. a@[email protected].) to not return the part before 2nd ``@`` as valid email address. Patch by jpic.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jpic

jpic commented Jun 11, 2019

Copy link
Copy Markdown
Contributor Author

Pushed a couple of fixes in new commits in my branch, left a question for you.

Thank you very much for your review @maxking ! You're max kind ;)
BoboTiG reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic
jpic force-pushed the bpo-34155 branch from 6d00315 to 07e5094 Compare June 25, 2019 19:26
@warsaw

warsaw commented Jul 2, 2019

Copy link
Copy Markdown
Member

As I mentioned in this comment on bpo I think that parseaddr('a@b@c') has to return ('', ''). To me, it's the only sane return value for illegal addresses.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@maxking

maxking commented Jul 2, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I totally agree on the fact that we should return tuple of two empty strings on "failed to parse", whether on not we decide to be opinionated about what we fail to parse. In this case security issue should return the same value.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic

jpic commented Jul 3, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Understood, it was decided to deal with that later if it had to be dealt with at all anyway.

I have fixed my own tests, but other tests fail since i changed the implementation and i'm currently digging into it... I lost track and started over from master which means that some commit history is lost.

I could add maxking credit too in the news item and bpo commit message.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic
jpic force-pushed the bpo-34155 branch 3 times, most recently from 84ca1e1 to 85ec1a8 Compare July 3, 2019 05:42
@jpic

jpic commented Jul 3, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Just one last question please, currently with this patch we get:

>>> parseaddr('a@b@c')                
('', '')

But when there is an invalid domain such as b., it doesn't return an empty string:

>>> parseaddr('a@b.')
('', 'a@b.')

Do you also want this patch to make parseaddr('a@b.') to return a tuple of empty strings ?

Or is it out of the scope of this patch ?

Or is a@b. not an illegal address at all ? I thought it was but I'm having a last minute doubt now (sorry about that)

Thanks a heap for your support ! Deeply appreciated ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@maxking

maxking commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

Do you also want this patch to make parseaddr('a@b.') to return a tuple of empty strings ?
Or is it out of the scope of this patch ?

That seems to be a change of behaviour, even though it is going to align with what we say in docs. Perhaps for the the short-term, we should make the docs align a bit more with the reality.

In the long term, we can decide if we want to return empty string where the email address is technically invalid. As it stands today, "failed to parse" means there is no way to read the value at all, so that includes weird encodings, random chars etc. I am not sure if we do RFC level validation for what is and isn't a valid address (maybe we do, haven't read those parts of the code too closely, but that is my current impression) so it may or may not be tricky to weed out all invalid addresses.

That discussion can perhaps start on a new issue and follow the usual change-of-behavior process and should be fixed in a different patch. This being a security fix will go back to 3.6 as well, so I guess we shouldn't do that here.

Or is a@b. not an illegal address at all ? I thought it was but I'm having a last minute doubt now (sorry about that)

It is an invalid address AFAIK, I can't quote the right RFC page to corroborate that right now, will need some more time for that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic

jpic commented Jul 3, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Thanks for your explanation. Please let me know if there's anything else you want me to do. If you want me to open the other issue I will start studying the corresponding contribution docs and try my best to match Python's standard expectations. Thanks for baring with me.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
maxking
maxking reviewed Jul 3, 2019
View reviewed changes

@maxking maxking left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The patch looks good to me, just a very teeny tiny nitpick that I have added inline comment for.

@warsaw I think after the fix for the comment, this patch should be good to go!
Needs backport to 3.6, 3.7, 3.8 since this has security implications.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/email/_parseaddr.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@maxking

maxking commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

@jpic I think it would be good to create a new issue to discuss if we want to change behaviour of parseaddr to validate valid addresses.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic

jpic commented Jul 3, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There goes the new issue, turned out that domains ending with a dot are valid ! Good to know
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic
jpic force-pushed the bpo-34155 branch from 85ec1a8 to 672f228 Compare July 4, 2019 11:11
@jpic

jpic commented Jul 4, 2019

Copy link
Copy Markdown
Contributor Author

Tests passed except MacOS job which failed on Azure, but it doesn't seem related to this patch.

2019-07-04-183254_1478x905_scrot
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jpic

jpic commented Jul 5, 2019

Copy link
Copy Markdown
Contributor Author

Do you also want PR against 3.6, 3.7, 3.8 since this has security implications ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@maxking

maxking commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

I am not sure about the failing tests, maybe we can rebase this against master and hope that it is fixed there? :)

I am currently travelling, so won't be able to help out with debugging much for this week.

And no, you don't need to manually create backports, I added the comment for whoever merges this to add right "needs backport to xxx" labels, which would then trigger the bot to create backport PRs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Jul 17, 2019

Copy link
Copy Markdown

GH-14824 is a backport of this pull request to the 3.8 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 17, 2019
@jpic @miss-islington
bpo-34155: Dont parse domains containing @ (pythonGH-13079)
77bb21f 
Before:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)

        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')

    After:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)

        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')

https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
@bedevere-bot

bedevere-bot commented Jul 17, 2019

Copy link
Copy Markdown

GH-14825 is a backport of this pull request to the 3.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Jul 17, 2019
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 17, 2019
@jpic @miss-islington
bpo-34155: Dont parse domains containing @ (pythonGH-13079)
aa329ac 
Before:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)

        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')

    After:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)

        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')

https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
@bedevere-bot

bedevere-bot commented Jul 17, 2019

Copy link
Copy Markdown

GH-14826 is a backport of this pull request to the 3.6 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.6 label Jul 17, 2019
@ned-deily

ned-deily commented Aug 1, 2019

Copy link
Copy Markdown
Member

@warsaw, do you still want the backports of this PR to be merged? They are still awaiting review and merging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@maxking

maxking commented Aug 9, 2019

Copy link
Copy Markdown
Contributor

@ned-deily It would be good to merge the backports for 3.6, I can merge the ones for 3.7 and 3.8.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Aug 9, 2019
@miss-islington @jpic
bpo-34155: Dont parse domains containing @ (GH-13079)
c48d606 
Before:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)

        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')

    After:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)

        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')

https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
miss-islington added a commit that referenced this pull request Aug 9, 2019
@miss-islington @jpic
bpo-34155: Dont parse domains containing @ (GH-13079)
2170774 
Before:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)

        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')

    After:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)

        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')

https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
ned-deily pushed a commit that referenced this pull request Aug 9, 2019
@miss-islington @jpic @ned-deily
bpo-34155: Dont parse domains containing @ (GH-13079) (GH-14826)
13a1913 
Before:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)

        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')

    After:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)

        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')

https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
@maxking maxking added the needs backport to 2.7 label Aug 15, 2019
@miss-islington

miss-islington commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

Thanks @jpic for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

I'm having trouble backporting to 2.7. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 2.7 label.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@maxking maxking added needs backport to 2.7 and removed needs backport to 2.7 labels Aug 15, 2019
@miss-islington

miss-islington commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

Thanks @jpic for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

Sorry, @jpic, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8cb65d1381b027f0b09ee36bfed7f35bb4dec9a9 2.7
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington self-assigned this Aug 15, 2019
maxking pushed a commit to maxking/cpython-1 that referenced this pull request Aug 17, 2019
@jpic @maxking
[3.5] bpo-34155: Dont parse domains containing @ (pythonGH-13079)
2b98ade 
Before:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)

        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')

    After:

        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)

        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')

https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
larryhastings pushed a commit that referenced this pull request Sep 7, 2019
@maxking @jpic @larryhastings
[3.5] bpo-34155: Dont parse domains containing @ (GH-13079) (#15317)
063eba2 
https://bugs.python.org/issue34155
(cherry picked from commit 8cb65d1)

Co-authored-by: jpic <[email protected]>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@jpic @lisroach
bpo-34155: Dont parse domains containing @ (pythonGH-13079)
a242640 
Before:
    
        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)
    
        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')
    
    After:
    
        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)
    
        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')




https://bugs.python.org/issue34155
@bedevere-bot

bedevere-bot commented Sep 11, 2019

Copy link
Copy Markdown

GH-16006 is a backport of this pull request to the 2.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 2.7 label Sep 11, 2019
miss-islington pushed a commit that referenced this pull request Sep 14, 2019
@rcsanchez97 @miss-islington
[2.7] bpo-34155: Dont parse domains containing @ (GH-13079) (GH-16006)
4cbcd2f 
This change skips parsing of email addresses where domains include a "@" character, which can be maliciously used since the local part is returned as a complete address. 

(cherry picked from commit 8cb65d1)

Excludes changes to Lib/email/_header_value_parser.py, which did not
exist in 2.7.

Co-authored-by: jpic <[email protected]>


https://bugs.python.org/issue34155
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@jpic @DinoV
bpo-34155: Dont parse domains containing @ (pythonGH-13079)
4d58a5c 
Before:
    
        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)
    
        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')
    
    After:
    
        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)
    
        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')




https://bugs.python.org/issue34155
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@jpic @websurfer5
bpo-34155: Dont parse domains containing @ (pythonGH-13079)
34c1e51 
Before:
    
        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='a', domain='malicious.org'),)
    
        >>> parseaddr('[email protected]@important.com')
        ('', '[email protected]')
    
    After:
    
        >>> email.message_from_string('From: [email protected]@important.com', policy=email.policy.default)['from'].addresses
        (Address(display_name='', username='', domain=''),)
    
        >>> parseaddr('[email protected]@important.com')
        ('', 'a@')




https://bugs.python.org/issue34155
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@warsaw warsaw warsaw approved these changes
+1 more reviewer
@maxking maxking maxking left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@miss-islington miss-islington

Labels

type-security A security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@jpic @the-knights-who-say-ni @warsaw @maxking @miss-islington @bedevere-bot @ned-deily
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.