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.

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

cookies: leave secure cookies alone (draft-ietf-httpbis-cookie-alone-01) #2956

Closed

Conversation

Projects
None yet
2 participants
@danielgustafsson
Copy link
Member

commented Sep 7, 2018

Only allow secure origins to be able to write cookies with the 'secure' flag set. This reduces the risk of non-secure origins to influence the state of secure origins. This implements IETF Internet-Draft draft-ietf-httpbis-cookie-alone-01 which updates RFC6265.

This is an old patch I dusted off which I wouldn't mind eyes on. There needs to be tests added of course which is on my TODO. 

Update: this used to be marked WIP as it lacked the required tests. These have now been added and the PR title is updated to reflect this.

@bagder

bagder approved these changes Sep 8, 2018

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

But yes, a test case that also verifies that it works as intended would be really good!

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2018

Do you think this should be added to an existing test, or should a new test be added?

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

I'm fine with either way. Whatever you find easiest.

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-httpbis-cookie-alone branch from dac8ee2 to a377072 Nov 18, 2018

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Rebased, test added and bugs fixed

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-httpbis-cookie-alone branch 2 times, most recently from 56f757b to a322771 Nov 19, 2018

@danielgustafsson danielgustafsson changed the title WIP: leave secure cookies alone cookies: leave secure cookies alone (draft-ietf-httpbis-cookie-alone-01) Nov 19, 2018

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

The Appveyor redness comes from 1506 failing which I believe is unrelated

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

Regarding the replacement policy for when a non-secure cookie is allowed to replace a secure cookie, the document states this for path comparisons:

Note: The "path" comparison is not symmetric, ensuring only
that a newly-created non-secure cookie does not overlay an
existing secure cookie, providing some mitigation against
cookie fixing attacks. That is, given an existing secure
cookie named "a" with a "path" of "/login", a non-secure
cookie named "a" could be set for a "path" of "/" or "/foo",
but not for a "path" of "/login" or "/login/en".

This leaves some room for implementation details IMHO. I have interpreted it in the most conservative way. The code implementing this could definitely need a set of eyes to ensure it's sane.

#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec4 myvalue5
#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec3 myvalue4
#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec2 myvalue3
#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec myvalue2

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson Nov 20, 2018

Author Member

The reason for these large sections removed is that they are setting secure cookies over HTTP, which with this patch is no longer allowed.

@bagder

bagder approved these changes Nov 22, 2018

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

As a reminder to myself for when the window opens: this patch also needs to remove item 5.8 from the TODO list. Will add that before this is pushed.

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-httpbis-cookie-alone branch from a322771 to a7a483a Dec 12, 2018

danielgustafsson added a commit to danielgustafsson/curl that referenced this pull request Dec 12, 2018

cookies: leave secure cookies alone
Only allow secure origins to be able to write cookies with the
'secure' flag set. This reduces the risk of non-secure origins
to influence the state of secure origins. This implements IETF
Internet-Draft draft-ietf-httpbis-cookie-alone-01 which updates
RFC6265.

Closes curl#2956
cookies: leave secure cookies alone
Only allow secure origins to be able to write cookies with the
'secure' flag set. This reduces the risk of non-secure origins
to influence the state of secure origins. This implements IETF
Internet-Draft draft-ietf-httpbis-cookie-alone-01 which updates
RFC6265.

Closes #2956

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-httpbis-cookie-alone branch from a7a483a to 739deca Dec 12, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Mar 13, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.