| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-11-21 00:24 |
| Thank you, I've committed the patch to 3.5 now. |
|
|