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

[idan22moral]

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]

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

[gpshead]

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]

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.