[issue25228] Regression in cookie parsing with brackets and quotes

[Collin Anderson]

Collin Anderson added the comment:

It should be safe to hard split on semicolon. `name="some;value"` is not valid, 
even though it's quoted. I think raw double quotes, commas, semicolons and 
backslashes are _always_ invalid characters in cookie values.

>From https://tools.ietf.org/html/rfc6265:

{{{
 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
}}}

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Collin Anderson]

Collin Anderson added the comment:

The issue I'm currently running into, is that although browsers correctly 
ignore invalid Set-Cookie values, they allow 'any CHAR except CTLs or ";"' in 
cookie values set via document.cookie.

So, if you say document.cookie = 'key=va"lue; path=/', the browser will happily 
pass 'key=va"lue;' to the server on future requests.

So, I like the behavior of this patch, which skips over these invalid cookies 
and continues parsing. I've cleaned the patch up a little, but it should be the 
same logically.

--
nosy: +collinanderson
Added file: http://bugs.python.org/file41889/cookie-bracket-quotes.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Martin Panter]

Martin Panter added the comment:

To move forward on this, I would like someone else (hopefully Antoine? :) to 
confirm my theory about the cookie injection attack, or otherwise explain why 
the patch won’t (re)open any security holes. Also, I would like to add some 
more test cases based on Sergey Bobrov’s post (especially the from the heading 
Особенности обработки Cookie #3).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Martin Panter]

Martin Panter added the comment:

Looking at this a second time, I think I have figured out what the security 
report was about. Before the fix (before revision 270f61ec1157), an attacker 
could trick the parser into accepting a separate key=value cookie “morsel”, 
when it was supposed to be part of some other cookie value. Suppose the “c=d” 
text was meant to be associated with the “message” key. Before the security 
fix, “c=d” is separated:

>>> SimpleCookie('a=b; messages=[""]c=d;')


With the fix applied, we now silently abort the parsing, and there is no 
spurious “c” key:

>>> SimpleCookie('a=b; messages=[""]c=d;')


This also seems to be described by Sergey Bobrov in Russian at 
.

Looking at the proposed patch again, I think the fix might be okay. Some 
specifications for cookies allow semicolons to be quoted or escaped, and I was 
a bit worried that this might be a problem. But all the scenarios I can imagine 
would be no worse with the patch compared to without it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Guido van Rossum]

Changes by Guido van Rossum :


--
nosy:  -gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Will Harris]

Changes by Will Harris :


--
nosy: +harris

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Martin Panter]

Martin Panter added the comment:

Adding Guido and Antoine, who committed the security fix as 9e765e65e5cb in 2.7 
and 5cfe74a9bfa4 in 3.2. Perhaps you are able to help decide if the proposal 
here would affect the original security report. Basically this issue (as well 
as #22758 and #22983) are complaining that 3.2’s cookie parsing became too 
strict. People would like to parse subsequent cookie “morsels” if an earlier 
one is considered invalid, rather than aborting the parse completely.

All I can find out about the security report is from 
 and 
, but that doesn’t explain the test cases 
with square brackets in the cookie names.

RFC 6265 says double quotes (") are not meant to be sent by the server, but the 
client should tolerate them without any special handling (different to Python’s 
handling and earlier specs, which parse a special double-quoted string syntax). 
One potential problem that comes to mind is that the current patch blindly 
searches for the next semicolon “;”, which would not be valid inside a 
double-quoted string, e.g. name="some;value".

Python behaviour:

* Before the 3.2 security fix, square brackets and double quotes caused 
truncation of the cookie value, but subsequent cookies were still parsed in 
most cases

* The security fix prevents parsing of subsequent cookies (either on purpose or 
as a side effect)

* The HttpOnly and Secure support in 3.3+ (Issue 16611) prevents parsing of the 
cookie morsel with the offending square bracket or double quote. This is 
proposed for 3.2 backport in Issue 22758.

* Square brackets are now allowed in 3.2+ thanks to Issue 22931. So 3.2 should 
truncate the original test case at the double quote, while 3.3+ drops the 
offending cookie.

The current patch proposed here appears to solve Issue 22983 (permissive 
parsing) in general. If the current cookie does not match the syntax, it is 
skipped, by falling back to a search for a semicolon “;”. So I am inclined to 
close Issue 22983 as a duplicate of this issue.

And Tim, I understand your main interest in Issue 22758 is that parsing aborts 
for things like "a=value1; HttpOnly; b=value2". If this patch were ported to 
3.2 it should also fix that for free.

Pathangi: did you see my review comment about unnecessary backslashes? I also 
left another comment today.

--
nosy: +gvanrossum, pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Just saw the code review comments now, didn't know that there was a separate 
section for code review comments until now. Will take a look and implement them.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

New patch with code review comments incorporated.

--
Added file: http://bugs.python.org/file40919/patch_review.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Guido van Rossum]

Guido van Rossum added the comment:

I'm coming at this without much context (I don't recall the original issue)
but IIUC from a security POV, lenient parsing is unsafe -- it could allow
an attacker to modify a cookie (or part of a cookie -- I'm unclear on the
correct terminology here) and that's what we're trying to avoid.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Has there been any movement on this issue?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[R. David Murray]

R. David Murray added the comment:

Yes, we should get signoff from someone who was involved in the original 
security fix, since it was a security fix.

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Hi, I've made the change to use str.find() and removed the while loop, can you 
take a look at it?

--
Added file: http://bugs.python.org/file40717/patch.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Martin Panter]

Martin Panter added the comment:

The str.find() call was kind of what I had in mind. But I don’t feel qualified 
to say whether the fix is good in general. I would have to find out about at 
the Cookie header format, and understand what the security implications are to 
do with lax parsing.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Tim Graham]

Tim Graham added the comment:

Yes, when I have some time.

By the way, did you intentionally remove all the "Python 3.X" versions on the 
issue?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Martin Panter]

Martin Panter added the comment:

Instead of the while loop, can’t you use something like str.find(";", i)?

--
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Hi Tim, I have submitted a patch (patch_with_test.diff). Can you take a look at 
this?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Oh not intentional. Must have clicked something by mistake

--
versions: +Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Is this what you wanted?

--
Added file: http://bugs.python.org/file40709/patch_with_test.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Tim Graham]

Tim Graham added the comment:

I had already proposed a test, see cookie-bracket-quotes-test.diff. What I 
meant was that the fix and the test should be combined into a single patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Hi I have made a patch for this, can anyone review and let me know?

--
versions:  -Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6
Added file: http://bugs.python.org/file40700/patch.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Hi Tim, I have submitted a patch for this issue (patch_final.diff, the earlier 
one failed a UT). Now all UTs are passing. Can you take a look at this?

--
Added file: http://bugs.python.org/file40701/patch_final.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Tim Graham]

Tim Graham added the comment:

Could you please integrate my unit test into your patch?

You also need to sign the PSF Contributor Agreement:
https://www.python.org/psf/contrib/contrib-form/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Oops, sorry looks like a unit test is failing. I will fix it and submit another 
one soon.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Added a patch where unit test has been modified to include the above case. I 
have signed the agreement.

--
Added file: http://bugs.python.org/file40703/patch_unittest.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

This is the first ever bug I will be working on so there might be a bit of a 
learning curve, but I'll do my best to come out with something by this week.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Tim Graham]

Tim Graham added the comment:

Sure, feel free to propose a patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Pathangi Jatinshravan]

Pathangi Jatinshravan added the comment:

Hi, can I be assigned to this behaviour issue?

--
nosy: +Pathangi Jatinshravan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Tim Graham]

Tim Graham added the comment:

It might be a case of issue22983. I'll try to look into the details and offer a 
patch next week.

For what it's worth, there are other regressions in Python 3.2 cookie parsing 
that makes the latest patch release (3.2.6) unusable with Django (issue22758), 
so from my perspective fixing this issue there isn't as high priority as that 
one.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Martin Panter]

Martin Panter added the comment:

Thanks for the test case. It looks like the commit in question was done as a 
security fix in 3.2.6 and 3.3.6. I’m not sure on the policy, but maybe that 
justifies putting any fixes into 3.2+.

I’m not familiar with HTTP cookies. Is this a case of a 100% 
specification-compiliant cookie, or a technically invalid one that would be 
nice to handle better? If the second case, maybe it is an instance of Issue 
22983.

--
keywords: +3.2regression
nosy: +martin.panter
stage:  -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25228] Regression in cookie parsing with brackets and quotes

[Tim Graham]

New submission from Tim Graham:

Regression in https://hg.python.org/cpython/rev/9e765e65e5cb (affects 2.7 and 
3.2+), similar to issue22931 where inserting an invalid cookie value can cause 
the rest of the cookie to be ignored. A test is attached, and here's a quick 
demo:

Old:
>>> from http.cookies import SimpleCookie
>>> SimpleCookie('a=b; messages=[\"\"]; c=d;')
{'a': 'b', 'c': 'd', 'messages': ''}

New:
>>> SimpleCookie('a=b; messages=[\"\"]; c=d;')
{'a': 'b'}

Reported in Django's tracker, but Django simply delegates to SimpleCookie: 
https://code.djangoproject.com/ticket/25458

--
components: Library (Lib)
files: cookie-bracket-quotes-test.diff
keywords: patch
messages: 251538
nosy: Tim.Graham
priority: normal
severity: normal
status: open
title: Regression in cookie parsing with brackets and quotes
type: behavior
versions: Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6
Added file: http://bugs.python.org/file40566/cookie-bracket-quotes-test.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com