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

Excess data in not handled properly in binascii.a2b_base64() #87252

Closed
idan22moral mannequin opened this issue Jan 31, 2021 · 3 comments
Closed

Excess data in not handled properly in binascii.a2b_base64() #87252

idan22moral mannequin opened this issue Jan 31, 2021 · 3 comments
Assignees
Labels
3.11 bug and security fixes extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-security A security issue

Comments

@idan22moral
Copy link
Mannequin

idan22moral mannequin commented Jan 31, 2021

BPO 43086
Nosy @gpshead, @ericvsmith, @idan22moral
PRs
  • bpo-43086: Added handling for excess data in binascii.a2b_base64 #24402
  • bpo-44678: Separate error message for discontinuous padding in binascii.a2b_base64 strict mode #27249
  • 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 = 'https://github.com/gpshead'
    closed_at = <Date 2021-07-19.00:56:06.939>
    created_at = <Date 2021-01-31.18:11:52.985>
    labels = ['type-security', 'extension-modules', 'library', '3.11']
    title = 'Excess data in not handled properly in binascii.a2b_base64()'
    updated_at = <Date 2021-07-19.18:37:26.489>
    user = 'https://github.com/idan22moral'

    bugs.python.org fields:

    activity = <Date 2021-07-19.18:37:26.489>
    actor = 'idan22moral'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2021-07-19.00:56:06.939>
    closer = 'gregory.p.smith'
    components = ['Extension Modules', 'Library (Lib)']
    creation = <Date 2021-01-31.18:11:52.985>
    creator = 'idan22moral'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43086
    keywords = ['patch']
    message_count = 3.0
    messages = ['386032', '397772', '397773']
    nosy_count = 4.0
    nosy_names = ['gregory.p.smith', 'eric.smith', 'python-dev', 'idan22moral']
    pr_nums = ['24402', '27249']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue43086'
    versions = ['Python 3.11']

    @idan22moral
    Copy link
    Mannequin Author

    idan22moral mannequin commented Jan 31, 2021

    Currently, when providing binascii.a2b_base64() base-64 input with excess data after the padding ('='/'=='), the excess data is ignored.

    Example:

    import binascii
    binascii.a2b_base64(b'aGVsbG8=')       # b'hello' (valid)
    binascii.a2b_base64(b'aGVsbG8==')      # b'hello' (ignoring data)
    binascii.a2b_base64(b'aGVsbG8=python') # b'hello' (ignoring data)

    Note: MANY libraries (such as the all-time favorite base64) use this function as their decoder.

    Why is it problematic:

    • User input can contain additional data after base64 data, which can lead to unintended behavior in products.
    • Well-crafted user input can be used to bypass conditions in code (example in the referenced tweet).
    • Can be used to target vulnerable libraries and bypass authentication mechanism such as JWT (potentially).

    The logic behind my fix PR on GitHub:

    • Before deciding to finish the function (after knowing the fact that we passed the data padding),
      we should check if there's no more data after the padding.
    • If excess data exists, we should raise an error, free the allocated writer, and return null.
    • Else, everything's fine, and we can proceed to the function's end as previously.

    Though not publicly disclosed, this behavior can lead to security issues in heavily-used projects.
    Preventing this behavior sounds more beneficial than harmful, since there's no known good usage for this behavior.

    From what I read, the python implementation in not so close (when speaking about this case of course) to the base64 RFC.
    (link: https://tools.ietf.org/html/rfc4648#section-3.3)

    Thanks to Ori Damari (twitter: https://twitter.com/0xrepnz) for bringing this behavior up,
    and thanks to Ryan Mast (twitter: https://twitter.com/rmast), and many of the other great guys for discussing the problem in the comments.

    Link to the tweet: https://twitter.com/0xrepnz/status/1355295649915404291

    --------------------------

    Idan Moral
    Twitter: https://twitter.com/idan_moral
    GitHub: https://github.com/idan22moral

    @idan22moral idan22moral mannequin added 3.10 only security fixes stdlib Python modules in the Lib dir type-security A security issue labels Jan 31, 2021
    @gpshead gpshead self-assigned this Mar 13, 2021
    @gpshead
    Copy link
    Member

    gpshead commented Jul 19, 2021

    New changeset 35b98e3 by Idan Moral in branch 'main':
    bpo-43086: Add handling for out-of-spec data in a2b_base64 (GH-24402)
    35b98e3

    @gpshead
    Copy link
    Member

    gpshead commented Jul 19, 2021

    I've merged Idan's PR adding a strict_mode parameter to a2b_base64. It defaults to False for backwards compatibility.

    From a security perspective, it'd be _ideal_ if this were True. But I expect doing that would break a bunch of existing code and tests that has been relying on some of the former leniency behaviors so I recommended the conservative approach of the old-behavior default. It'd be a good thing to change it to True, but disruptive. We need motivating reason to do that.

    As it is a new feature due to the new parameter, this is for 3.11.

    Workaround for Pythons without this: do a validity check before calling a2b_base64. I suspect a regex could be constructed for that if you're careful. If you come up with one, please share it here.

    @gpshead gpshead added extension-modules C modules in the Modules dir 3.11 bug and security fixes and removed 3.10 only security fixes labels Jul 19, 2021
    @gpshead gpshead closed this as completed Jul 19, 2021
    @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.11 bug and security fixes extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant