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

danielgustafsson
Copy link
Member

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

bagder commented Sep 9, 2018

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

@danielgustafsson
Copy link
Member Author

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

@bagder
Copy link
Member

bagder commented Sep 10, 2018

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

@danielgustafsson
Copy link
Member Author

Rebased, test added and bugs fixed

@danielgustafsson danielgustafsson force-pushed the dg-httpbis-cookie-alone branch 2 times, most recently from 56f757b to a322771 Compare November 19, 2018 22:27
@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
Copy link
Member Author

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

@danielgustafsson
Copy link
Member Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@danielgustafsson danielgustafsson added the feature-window A merge of this requires an open feature window label Nov 21, 2018
@danielgustafsson
Copy link
Member Author

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 added a commit to danielgustafsson/curl that referenced this pull request Dec 12, 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.

Closes curl#2956
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
@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.
Labels
feature-window A merge of this requires an open feature window
Development

Successfully merging this pull request may close these issues.

None yet

2 participants