classification
Title: Support for httponly/secure cookies reintroduced lax parsing behavior
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, PaulMcMillan, Tim.Graham, berker.peksag, georg.brandl, pitrou, python-dev, r.david.murray, rhettinger
Priority: normal Keywords: patch

Created on 2014-11-04 16:47 by Tim.Graham, last changed 2014-11-21 00:24 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
cookie_strict_parsing.patch pitrou, 2014-11-04 20:31 review
Messages (16)
msg230637 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-04 16:47
As noted in the comments of #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 #16611 reintroduces "lax" parsing behavior that the security fix [1] was supposed to prevent.)

[1] https://hg.python.org/cpython/rev/d3663a0f97ed
msg230644 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-11-04 17:23
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?
msg230645 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-11-04 17:38
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.
msg230650 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-04 18:34
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.
msg230651 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-04 19:10
This qualification isn't really accurate:

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

since the #16611 changes were committed *before* the security fix.
msg230653 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-04 19:49
Note that f81846c2b746 adds an explicit test for acceptance of invalid cookie strings ("test_bad_attrs").
msg230654 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-11-04 19:51
These are unknown attributes after a key=value pair.  What this issue is about is accepting attributes *before* any key=value pair.
msg230655 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-04 20:31
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.
msg230658 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-05 02:41
Django's test suite passes with the proposed patch after some updates: https://github.com/django/django/pull/3455
msg230668 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-11-05 08:41
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.
msg230719 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-05 23:59
Paul, Tim, do you think there's a real risk of regression with the proposed patch?
msg230721 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-06 01:10
Security-wise? I don't know, I haven't really been in the loop on the original issue.
msg230728 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-06 07:31
No, I meant functionality-wise.
msg230758 - (view) Author: Tim Graham (Tim.Graham) * Date: 2014-11-06 19:41
Django's test suite doesn't reveal any regressions. All the changes there are expected as far as I can see.
msg231451 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-21 00:23
New changeset a065ab1c67a8 by Antoine Pitrou in branch 'default':
Issue #22796: HTTP cookie parsing is now stricter, in order to protect against potential injection attacks.
https://hg.python.org/cpython/rev/a065ab1c67a8
msg231452 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-21 00:24
Thank you, I've committed the patch to 3.5 now.
History
Date User Action Args
2014-11-21 00:24:26pitrousetstatus: open -> closed
resolution: fixed
messages: + msg231452

stage: commit review -> resolved
2014-11-21 00:23:57python-devsetnosy: + python-dev
messages: + msg231451
2014-11-06 19:41:22Tim.Grahamsetmessages: + msg230758
2014-11-06 07:31:04pitrousetmessages: + msg230728
2014-11-06 01:10:49Tim.Grahamsetmessages: + msg230721
2014-11-05 23:59:01pitrousetstage: commit review
messages: + msg230719
versions: + Python 3.5, - Python 2.7, Python 3.2, Python 3.3, Python 3.4
2014-11-05 08:41:40rhettingersetnosy: + rhettinger
messages: + msg230668
2014-11-05 02:41:55Tim.Grahamsetmessages: + msg230658
2014-11-04 20:33:01pitrousetnosy: + PaulMcMillan
2014-11-04 20:31:47pitrousetfiles: + cookie_strict_parsing.patch
keywords: + patch
messages: + msg230655
2014-11-04 19:51:30georg.brandlsetmessages: + msg230654
2014-11-04 19:49:26pitrousetmessages: + msg230653
2014-11-04 19:10:29pitrousetmessages: + msg230651
2014-11-04 18:34:36pitrousetmessages: + msg230650
2014-11-04 17:38:58georg.brandlsetmessages: + msg230645
2014-11-04 17:23:21r.david.murraysetmessages: + msg230644
2014-11-04 16:58:55Arfreversetnosy: + Arfrever
2014-11-04 16:47:43Tim.Grahamcreate