Support for httponly/secure cookies reintroduced lax parsing behavior

[timgraham]

BPO	22796
Nosy	@birkenfeld, @rhettinger, @pitrou, @bitdancer, @berkerpeksag, @PaulMcMillan, @timgraham
Files	cookie_strict_parsing.patch

^{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 2014-11-21.00:24:26.306>
created_at = <Date 2014-11-04.16:47:43.696>
labels = ['type-security', 'library']
title = 'Support for httponly/secure cookies reintroduced lax parsing behavior'
updated_at = <Date 2014-11-21.00:24:26.305>
user = 'https://github.com/timgraham'

bugs.python.org fields:

activity = <Date 2014-11-21.00:24:26.305>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2014-11-21.00:24:26.306>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2014-11-04.16:47:43.696>
creator = 'Tim.Graham'
dependencies = []
files = ['37130']
hgrepos = []
issue_num = 22796
keywords = ['patch']
message_count = 16.0
messages = ['230637', '230644', '230645', '230650', '230651', '230653', '230654', '230655', '230658', '230668', '230719', '230721', '230728', '230758', '231451', '231452']
nosy_count = 9.0
nosy_names = ['georg.brandl', 'rhettinger', 'pitrou', 'Arfrever', 'r.david.murray', 'python-dev', 'berker.peksag', 'PaulMcMillan', 'Tim.Graham']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue22796'
versions = ['Python 3.5']

[timgraham]

As noted in the comments of bpo-22758 by Georg Brandle:

• Django uses __init__(str()) roundtripping, which is not explicitly supported by the library, and worked by accident with previous versions. That it works again with 3.3+ is another accident, and a bug.

(The change for bpo-16611 reintroduces "lax" parsing behavior that the security fix [1] was supposed to prevent.)

[1] https://hg.python.org/cpython/rev/d3663a0f97ed

[bitdancer]

This test still exists, so the change didn't cause it to trigger. What is the security bug? The commit doesn't say, and doesn't reference an issue number. So if that test still passes, what's the bug?

[birkenfeld]

Well, with this change you can again (e.g.) pass

"Set-cookie: foo=bar"

which isn't a valid cookie. It doesn't reintroduce the same vulnerability, but it will still silently consume invalid cookies (i.e. such with attribute-like tokens upfront) and return a seemingly valid one.

IMO this is questionable behavior of the kind that can enable exploits, which is also why it was disallowed by the fix of the first vulnerability.

[pitrou]

The security issue isn't easy to explain, it involves an elaborated set of services (browser, Web site...) each having a slightly different notion of cookie parsing to mount an attack allowing to bypass CSRF protection on certain Python-powered frameworks. It's from a report made to security@p.o.

[pitrou]

This qualification isn't really accurate:

The change for bpo-16611 reintroduces "lax" parsing behavior that the security fix [1] was supposed to prevent

since the bpo-16611 changes were committed *before* the security fix.

[pitrou]

Note that f81846c2b746 adds an explicit test for acceptance of invalid cookie strings ("test_bad_attrs").

[birkenfeld]

These are unknown attributes after a key=value pair. What this issue is about is accepting attributes *before* any key=value pair.

[pitrou]

Well, if we want to become stricter, I don't it makes sense to stop at the middle of the road.
In any case, here is a patch enabling strict parsing.

[timgraham]

Django's test suite passes with the proposed patch after some updates: django/django#3455

[rhettinger]

The patch looks good. One nit, please change "items" to "typed_items" or somesuch. That will make it clear why there are 3-tuples instead of the traditional 2-tuple used for normal mappings.

[pitrou]

Paul, Tim, do you think there's a real risk of regression with the proposed patch?

[timgraham]

Security-wise? I don't know, I haven't really been in the loop on the original issue.

[pitrou]

No, I meant functionality-wise.