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.

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

parse_message_id in email module is very buggy / crashy #82889

Closed
xnox mannequin opened this issue Nov 5, 2019 · 6 comments
Closed

parse_message_id in email module is very buggy / crashy #82889

xnox mannequin opened this issue Nov 5, 2019 · 6 comments
Labels
3.8 only security fixes 3.9 only security fixes topic-email type-bug An unexpected behavior, bug, or error

Comments

@xnox
Copy link
Mannequin

xnox mannequin commented Nov 5, 2019

BPO 38708
Nosy @warsaw, @bitdancer, @xnox, @maxking, @miss-islington, @tirkarthi
PRs
  • bpo-38708: email: Fix a potential IndexError when parsing Message-ID #17504
  • [3.8] bpo-38708: email: Fix a potential IndexError when parsing Message-ID (GH-17504) #17515
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-12-09.02:35:20.213>
    created_at = <Date 2019-11-05.22:48:04.516>
    labels = ['type-bug', '3.8', 'expert-email', '3.9']
    title = 'parse_message_id in email module is very buggy / crashy'
    updated_at = <Date 2019-12-09.02:35:20.210>
    user = 'https://github.com/xnox'

    bugs.python.org fields:

    activity = <Date 2019-12-09.02:35:20.210>
    actor = 'maxking'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-12-09.02:35:20.213>
    closer = 'maxking'
    components = ['email']
    creation = <Date 2019-11-05.22:48:04.516>
    creator = 'xnox'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38708
    keywords = ['patch']
    message_count = 6.0
    messages = ['356072', '356109', '356123', '358045', '358047', '358048']
    nosy_count = 6.0
    nosy_names = ['barry', 'r.david.murray', 'xnox', 'maxking', 'miss-islington', 'xtreak']
    pr_nums = ['17504', '17515']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue38708'
    versions = ['Python 3.8', 'Python 3.9']

    @xnox
    Copy link
    Mannequin Author

    xnox mannequin commented Nov 5, 2019

    email module has recently got parse_message_id which is more strict now, then before.

    However, it's not programmed as defensively as expected. Given bogus message-id, it crashes with unbound local variable, or like accessing a non-existing index.

    So hyperkitty had a Message-ID "X"*260 in the testsuite that used to pass with 3.7, but fails with 3.8.

    ======================================================================
    ERROR: test_long_message_id (hyperkitty.tests.lib.test_incoming.TestAddToList)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "./hyperkitty/tests/lib/test_incoming.py", line 295, in test_long_message_id
        msg["Message-ID"] = "X" * 260
      File "/usr/lib/python3.8/email/message.py", line 409, in __setitem__
        self._headers.append(self.policy.header_store_parse(name, val))
      File "/usr/lib/python3.8/email/policy.py", line 148, in header_store_parse
        return (name, self.header_factory(name, value))
      File "/usr/lib/python3.8/email/headerregistry.py", line 602, in __call__
        return self[name](name, value)
      File "/usr/lib/python3.8/email/headerregistry.py", line 197, in __new__
        cls.parse(value, kwds)
      File "/usr/lib/python3.8/email/headerregistry.py", line 530, in parse
        kwds['parse_tree'] = parse_tree = cls.value_parser(value)
      File "/usr/lib/python3.8/email/_header_value_parser.py", line 2116, in parse_message_id
        message_id.append(token)
    UnboundLocalError: local variable 'token' referenced before assignment

    Similarly another user, surkova reports that value[0] in get_msg_id function is buggy too (doesn't check that value has a member)

    First reported #13397 (comment)

    Ideally, I'd like the function to raise a documented Exception for invalid Message-id, but not fail with what look like regular programming bugs in the email module. Expectation is that email module is either more permissive or is coded more defence-in-depth with more checking in place.

    @xnox xnox mannequin added 3.8 only security fixes 3.9 only security fixes labels Nov 5, 2019
    @tirkarthi
    Copy link
    Member

    This seems to be the same as bpo-38698.

    @xnox
    Copy link
    Mannequin Author

    xnox mannequin commented Nov 6, 2019

    Yes, bpo-38698 covers the UnboundLocalError,

    but doesn't cover inside get_msg_id there is also this gem:

        def get_msg_id(value):
            msg_id = MsgID()
            if value[0] in CFWS_LEADER:

    It should test value before accessing value[0] like it is done in other places, ie.:

        if value and value[0] in CFWS_LEADER:

    or indent the whole block to iterate over value with:

        while value:
            ...

    which also tests that value has [0] index.

    I guess I want to repurpose this issue for the value[0] indexerror in get_msg_id().

    @tirkarthi tirkarthi added topic-email type-bug An unexpected behavior, bug, or error labels Nov 6, 2019
    @maxking
    Copy link
    Contributor

    maxking commented Dec 9, 2019

    New changeset 3ae4ea1 by Abhilash Raj in branch 'master':
    bpo-38708: email: Fix a potential IndexError when parsing Message-ID (GH-17504)
    3ae4ea1

    @miss-islington
    Copy link
    Contributor

    New changeset 2abd3a8 by Miss Islington (bot) in branch '3.8':
    bpo-38708: email: Fix a potential IndexError when parsing Message-ID (GH-17504)
    2abd3a8

    @maxking
    Copy link
    Contributor

    maxking commented Dec 9, 2019

    Closing this as fixed.

    @maxking maxking closed this as completed Dec 9, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes topic-email type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants