Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for httponly/secure cookies reintroduced lax parsing behavior #66985

Closed
timgraham mannequin opened this issue Nov 4, 2014 · 16 comments
Closed

Support for httponly/secure cookies reintroduced lax parsing behavior #66985

timgraham mannequin opened this issue Nov 4, 2014 · 16 comments
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@timgraham
Copy link
Mannequin

timgraham mannequin commented Nov 4, 2014

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
    Copy link
    Mannequin Author

    timgraham mannequin commented Nov 4, 2014

    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

    @timgraham timgraham mannequin added stdlib Python modules in the Lib dir type-security A security issue labels Nov 4, 2014
    @bitdancer
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    pitrou commented Nov 4, 2014

    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
    Copy link
    Member

    pitrou commented Nov 4, 2014

    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
    Copy link
    Member

    pitrou commented Nov 4, 2014

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

    @birkenfeld
    Copy link
    Member

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

    @pitrou
    Copy link
    Member

    pitrou commented Nov 4, 2014

    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
    Copy link
    Mannequin Author

    timgraham mannequin commented Nov 5, 2014

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

    @rhettinger
    Copy link
    Contributor

    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
    Copy link
    Member

    pitrou commented Nov 5, 2014

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

    @timgraham
    Copy link
    Mannequin Author

    timgraham mannequin commented Nov 6, 2014

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

    @pitrou
    Copy link
    Member

    pitrou commented Nov 6, 2014

    No, I meant functionality-wise.

    @timgraham
    Copy link
    Mannequin Author

    timgraham mannequin commented Nov 6, 2014

    Django's test suite doesn't reveal any regressions. All the changes there are expected as far as I can see.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 21, 2014

    New changeset a065ab1c67a8 by Antoine Pitrou in branch 'default':
    Issue bpo-22796: HTTP cookie parsing is now stricter, in order to protect against potential injection attacks.
    https://hg.python.org/cpython/rev/a065ab1c67a8

    @pitrou
    Copy link
    Member

    pitrou commented Nov 21, 2014

    Thank you, I've committed the patch to 3.5 now.

    @pitrou pitrou closed this as completed Nov 21, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @birkenfeld @bitdancer @rhettinger @pitrou