classification
Title: cookies with square brackets in value
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark Hughes, Tim Pierce, Tim.Graham, Waldemar.Parzonka, benjamin.peterson, berker.peksag, demian.brecht, dlamotte, georg.brandl, gvanrossum, larry, python-dev, r.david.murray, twpierce
Priority: release blocker Keywords: patch

Created on 2014-11-24 15:43 by Waldemar.Parzonka, last changed 2015-06-03 20:48 by Tim Pierce. This issue is now closed.

Files
File name Uploaded Description Edit
issue22931.patch demian.brecht, 2014-12-02 02:23 review
issue22931_1.patch demian.brecht, 2015-03-17 14:54 review
issue22931_2.patch demian.brecht, 2015-04-01 00:00 review
Messages (17)
msg231605 - (view) Author: Waldemar Parzonka (Waldemar.Parzonka) Date: 2014-11-24 15:43
There seems to be weird behaviour in BaseCookie.load() when cookie that has '[' in one of the values is being loaded.

There is no exception being thrown as the key is still legal but the cookie is not getting loaded properly and everything that was after the '[' valued cookie is being silently ignored.

>>> dd = SimpleCookie()
>>> dd
<SimpleCookie: >
>>> s = 'a=b; c=[; d=r; f=h'
>>> dd.load(s)
>>> dd
<SimpleCookie: a='b'>
>>>
msg231847 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-11-29 01:32
There could be some history behind this that I'm unaware of that I'm not familiar with.

From what I can tell, this issue is simply due to the "[" character not being in _LegalCharsPatt (http/cookies.py). _LegalCharsPatt actually seems quite a bit more restrictive than it really should be. It's set to r"[\w\d!#%&'~_`><@,:/\$\*\+\-\.\^\|\)\(\?\}\{\=]", where RFC 6265 specifies:

cookie-pair       = cookie-name "=" cookie-value
cookie-name       = token
cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
token             = <token, defined in [RFC2616], Section 2.2>

_LegalCharsPatt is used for regex matching on the cookie value, not the key (there is a distinction made between the two).

The omission of those characters is correct for the cookie keys, but not the values (RFC 2965 is a little less verbose, but nothing ruling out those characters for values).
msg231970 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-02 00:12
Err, sorry, I entirely misunderstood the problem. The invalid characters are correct ([ = 5B, which indeed is illegal, I wasn't paying close enough attention to the hex values in the ABNF). It's the fact that the valid key/value pairs after the invalid one are ignored. I'll dig into the RFC and see if there's an expected behavior here and whether or not it's currently handled as expected.
msg231972 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-02 01:12
Now I've confused myself and my first impression was correct. For some reason, my brain was thinking "%x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E" was the exclusion list for some reason (which is obviously horribly wrong).

So my first observation was correct in that they should simply be added to the valid character list and I'll get a patch together for that.
msg231973 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-02 02:23
Attached patch to fix the issue as reported.

Something interesting that came out of this though is that due to the regex expression, if there's an invalid character in one of the cookie-octets, the rest of the cookie is ignored. I would assume that it should either a) ignore the entire cookie string or b) ignore the invalid cookie pair and accept valid pairs following. I've been unable to find that defined in any of the RFCs though.
msg231982 - (view) Author: Waldemar Parzonka (Waldemar.Parzonka) Date: 2014-12-02 09:23
Thanks for taking a look into that.

And yes the behaviour when invalid value is encountered is bit weird as the rest of the cookie is being silently ignored which is probably less than ideal in most cases.

Just wonder if there is any easy way of making the matching more aware as browsers may allow various things as cookie values I guess.
msg232021 - (view) Author: Demian Brecht (demian.brecht) * Date: 2014-12-02 17:01
I do think it should be a little more permissive when parsing cookies. I've created #22983 to address that as to not conflate this issue, which the attached patch does address.
msg234913 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-01-28 20:25
Ping for review/commit.
msg238205 - (view) Author: Mark Hughes (Mark Hughes) Date: 2015-03-16 14:05
This is also an issue with Python 2.7.9 but not 2.7.8. There were various cookie related fixes in 2.7.9 which could have revealed this issue. Maybe this one?

https://hg.python.org/cpython/rev/9e765e65e5cb
msg238233 - (view) Author: Mark Hughes (Mark Hughes) Date: 2015-03-16 17:42
We experimented with a version of the patch for 2.7.9. 

One issue we immediately noticed is that even though disallowed by the spec the use of commas in cookie values is widespread so we needed to add \, to the _LEGAL_VALUES_PATT.
msg238297 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-17 14:49
Thanks for the report Mark, updating this patch to be more backwards compatible was on my to-do list. I've attached a new patch that simply adds the new characters to the legal value set.

It does look like that's the commit that introduced this issue, but the change was made for good reason.
msg239271 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-03-25 16:30
Will this regression be fixed in Python 2.7, 3.2, and 3.3? If not, Django may need to vendor Python's cookie class to workaround this bug to prevent users from losing sessions and/or being unable to login to Django powered sites as reported in https://code.djangoproject.com/ticket/24492.
msg239302 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-26 02:34
As I understand it, the change should also be applied to security releases
as the regression manifested by a security related patch being applied.
That said, there may be some debate as there apparently isn't much (if
anything) in the way of precedence here.
msg241948 - (view) Author: Tim Pierce (twpierce) Date: 2015-04-24 15:40
Adding Python 2.7 to the affected versions (from #23341 which was closed as a duplicate of this bug). We are very interested to know whether this will be fixed in a Python 2.7 patch as well.
msg243136 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-14 00:49
This needs a review from the people who created and applied the security patch.  Demian, did you add them to nosy already?

Since this is a regression I'm going to mark it as a release blocker so Benjamin can decide whether or not it is important enough to go in to 2.7.10 even though the RC is already out.
msg243604 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-05-19 16:08
> This needs a review from the people who created and applied the security patch.

+ Guido (committed https://hg.python.org/cpython/rev/9e765e65e5cb)
msg243924 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-23 15:48
New changeset 710cdba13323 by Benjamin Peterson in branch '3.2':
allow square brackets in cookie values (closes #22931)
https://hg.python.org/cpython/rev/710cdba13323

New changeset c7b3a50a2f01 by Benjamin Peterson in branch '3.3':
merge 3.2 (#22931)
https://hg.python.org/cpython/rev/c7b3a50a2f01

New changeset a43f5515e3a2 by Benjamin Peterson in branch '3.4':
merge 3.3 (#22931)
https://hg.python.org/cpython/rev/a43f5515e3a2

New changeset c58f3e76dc6c by Benjamin Peterson in branch 'default':
merge 3.4 (#22931)
https://hg.python.org/cpython/rev/c58f3e76dc6c

New changeset 2a7b0e145945 by Benjamin Peterson in branch '2.7':
allow square brackets in cookie values (#22931)
https://hg.python.org/cpython/rev/2a7b0e145945
History
Date User Action Args
2015-06-03 20:48:40Tim Piercesetnosy: + Tim Pierce
2015-05-23 15:48:16python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg243924

resolution: fixed
stage: commit review -> resolved
2015-05-19 16:08:23demian.brechtsetnosy: + gvanrossum
messages: + msg243604
2015-05-14 00:49:29r.david.murraysetpriority: normal -> release blocker
versions: + Python 3.3
nosy: + larry, benjamin.peterson, georg.brandl

messages: + msg243136
2015-04-24 15:40:31twpiercesetnosy: + twpierce

messages: + msg241948
versions: + Python 2.7
2015-04-01 00:00:52demian.brechtsetfiles: + issue22931_2.patch
2015-03-31 23:59:47demian.brechtsetstage: patch review -> commit review
2015-03-26 02:34:31demian.brechtsetmessages: + msg239302
2015-03-25 16:30:17Tim.Grahamsetmessages: + msg239271
2015-03-18 13:48:49Tim.Grahamsetnosy: + Tim.Graham
2015-03-17 14:54:42demian.brechtsetfiles: + issue22931_1.patch
2015-03-17 14:54:25demian.brechtsetfiles: - issue22931_1.patch
2015-03-17 14:49:31demian.brechtsetfiles: + issue22931_1.patch

messages: + msg238297
2015-03-16 17:42:38Mark Hughessetmessages: + msg238233
2015-03-16 14:05:43Mark Hughessetnosy: + Mark Hughes
messages: + msg238205
2015-02-20 12:28:18berker.peksagsetnosy: + berker.peksag
stage: patch review

versions: - Python 3.3, Python 3.6
2015-01-28 20:25:54demian.brechtsetmessages: + msg234913
2015-01-28 20:23:56berker.peksaglinkissue23341 superseder
2015-01-28 20:19:07dlamottesetnosy: + dlamotte
2014-12-02 17:01:17demian.brechtsetmessages: + msg232021
2014-12-02 09:23:25Waldemar.Parzonkasetmessages: + msg231982
2014-12-02 02:23:48demian.brechtsetfiles: + issue22931.patch
keywords: + patch
messages: + msg231973
2014-12-02 01:12:25demian.brechtsetmessages: + msg231972
2014-12-02 00:12:52demian.brechtsetmessages: + msg231970
2014-11-29 01:32:20demian.brechtsetmessages: + msg231847
versions: + Python 3.4, Python 3.5, Python 3.6
2014-11-28 19:09:41demian.brechtsetnosy: + demian.brecht
2014-11-24 16:12:42r.david.murraysetnosy: + r.david.murray
2014-11-24 15:43:24Waldemar.Parzonkacreate