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

Set-Cookie not overwriting anymore / behavior change between 7.47.1 and 7.61.1 #3299

Closed
daboul opened this issue Nov 22, 2018 · 9 comments
Closed

Comments

@daboul
Copy link

daboul commented Nov 22, 2018

Hi.
I have a simple server (ASP.NET Core) with cookie Authentication that is sending to my curl client a Set-Cookie at login, and another one to overwrite the same cookie at logout. The curl client is using a cookie-jar file to store the cookie.

Note, that I'm using the code from Microsoft/ASP.NET Core and haven't done anything particular in the server code, I'm not manipulating the cookie, just a basic SignIn / SignOut which takes care of sending the Set-Cookie command back to the curl client.

With a 7.47.1, it is working fine. After the logout I'm getting the cookie cleaned out from my cookie-jar.

LOGIN 7.47.1
HTTP server response:

* Added cookie cookieModuleXXX="CfDJ8PNMkDZf1-VPjhP1ezVfsxJxlvsx40RiS-..." for domain localhost, path /XXX/YYY, expire 1542884051
< Set-Cookie: cookieModuleXXX=CfDJ8PNMkDZf1-VPjhP1ezVfsxJxlvsx40RiS-...; expires=Thu, 22 Nov 2018 10:54:11 GMT; path=/XXX/YYY; secure; samesite=lax; httponly

cookiejar.txt:
#HttpOnly_localhost FALSE /XXX/YYY TRUE 1542884051 cookieModuleXXX CfDJ8PNMkDZf1-VPjhP1ezVfsxJxlvsx40RiS-...

LOGOUT 7.47.1
HTTP server response:

* Replaced cookie cookieModuleXXX="" for domain localhost, path /XXX/YYY, expire 1
< Set-Cookie: cookieModuleXXX=; expires=Thu, 01 Jan 1970 00:00:00 GMT; path=/XXX/YYY; samesite=lax

cookiejar.txt: ==>EMPTY

With a 7.61.1, it is failing. The cookie remains in my cookie-jar, hence my session remains opened and active and I can't sign out. A simple change of curl.exe, with the exact same command line, suffices to replicate the issue.

LOGIN 7.61.1
HTTP server response:

* Added cookie cookieModuleXXX="CfDJ8PNMkDZf1-VPjhP1ezVfsxLH8UB6HEB_..." for domain localhost, path /XXX/YYY, expire 1542881087
< Set-Cookie: cookieModuleXXX=CfDJ8PNMkDZf1-VPjhP1ezVfsxLH8UB6HEB_...; expires=Thu, 22 Nov 2018 10:04:47 GMT; path=/XXX/YYY; samesite=lax; httponly

cookiejar.txt:
#HttpOnly_localhost FALSE /XXX/YYY FALSE 1542881087 cookieModuleXXX CfDJ8PNMkDZf1-VPjhP1ezVfsxLH8UB6HEB_...

LOGOUT 7.61.1
HTTP server response:

* Replaced cookie cookieModuleXXX="" for domain localhost, path /XXX/YYY, expire 1
< Set-Cookie: cookieModuleXXX=; expires=Thu, 01 Jan 1970 00:00:00 GMT; path=/XXX/YYY; samesite=lax

cookiejar.txt: ==>NOT GETTING CLEANED
#HttpOnly_localhost FALSE /XXX/YYY FALSE 1542881087 cookieModuleXXX CfDJ8PNMkDZf1-VPjhP1ezVfsxLH8UB6HEB_...

So basically, Set-Cookie cookieModuleXXX="" is not overwriting and emptying my cookie-jar with the 7.61.1 as it was done in 7.47.1. Am I missing something? Is the new behavior making more sense for a reason I'm not getting?

Thank you,
David.

@daboul
Copy link
Author

daboul commented Nov 22, 2018

Tried with 7.62 but no luck, behaves like the 7.61.1.

@bagder
Copy link
Member

bagder commented Nov 22, 2018

Hm, this seems to happen when the last cookie is removed from the internal cookie stash in memory as then it skips writing the file since there are no cookies to save... This has actually always been the case, but in recent years we've enhanced the expiring of cookies internally so it has then caused this side-effect.

I suppose the end result here is that we can't avoid writing the file even if there aren't any cookies left, for exactly this reason!

bagder added a commit that referenced this issue Nov 22, 2018
Important for when the file is going to be read again and thus must not
contain old contents!

Adds test 327 to verify.

Reported-by: daboul on github
Fixes #3299
Closes #....
@daboul
Copy link
Author

daboul commented Nov 22, 2018

Thank you so much for the swift reply and fix. Do you have a rough idea of the versions impacted? Do you see any possible workaround I could implement on the server that would prevent this from happening?

@jzakrzewski
Copy link
Contributor

I guess the server should simply invalidate the id after the logout request and not accept it again. Otherwise logout does not protect your clients in case the cookie has been compromised.

@bagder
Copy link
Member

bagder commented Nov 22, 2018

@daboul one fairly easy work-around is for you to ask curl to first read some irrelevant cookies from a local file that will remain in the cookie cache in memory (and never sent off since they won't match), as then it will have to write those to the cookie jar again later.

Or to change file names when you write the cookies as then you'll note that the file is missing which then means no cookies to load.

@daboul
Copy link
Author

daboul commented Nov 22, 2018

@jzakrzewski I agree and I'm actually surprised because I'm using the vanilla ASP.NET Core server with cookie authentication and I was expecting that calling SignOut server side would invalidate some kind of server side session, so that even if the client comes back with the cookie again, it won't get accepted and the authentication would fail. But it is not the case, I'll try to dig a little bit on the server part as well.

@bagder Thank you.

@daboul
Copy link
Author

daboul commented Nov 22, 2018

@jzakrzewski Indeed the default implementation of ASP.NET Core with the Cookie Authentication activated is stateless so I need the cookie to get destroyed by the client. I could add a session server side but making the server stateful would have important impact on the overall architecture and would be an overkill just to workaround this issue. Reference: https://stackoverflow.com/questions/48589373/is-it-asp-net-core-cookie-authentication-without-identity-session-stateless-yes/53433223#53433223

@bagder with the current delivery process which I'm not familiar with, when can we expect a new official package with that fix? Asking to decide whether I should build it myself or wait for the official package.

@bagder
Copy link
Member

bagder commented Nov 22, 2018

@daboul I expect this fix to land any minute and thus will be included and shipped with 7.63.0, due to be released on December 12, 2018.

@jzakrzewski
Copy link
Contributor

@daboul I'd argue that this is a security issue but maybe it's fine in your use case.
Anyway, you've got a workaround, fix for curl is on its way, so all is well ;)

@bagder bagder closed this as completed in a52e46f Nov 23, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2019
@bagder bagder added the cookies label Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants