Closed GordonBeeming closed 6 months ago
Thanks for getting in touch and putting in the effort to create a PR! Unfortunately, I don't think we should add this option because
Javascript shouldn't interact with the identity server session cookie. The content of the session cookie is encrypted and signed by the web server using data protection, and there is no way for javascript to read or write the plaintext content of the cookie. The only things that javascript could do with the cookie if you made this change are
There is a legitimate use case for wanting to know that a session exists. If you need a cookie like that, we have the idsrv.session cookie, which we use in our implementation of OpenId Connect Session Management (check session iframes for SPAs).
But that cookie is distinct from the main session cookie, and that's important because malicious script can't use it to steal sessions.
Howzit @josephdecock
Thanks for the explanation.
Why not allow for configuration for when someone wants to not allow js to use it?
FYI, I have solved this locally by setting the global cookie policy and everything works still for me.
The need to change this came out of a PEN test where it wasn't needed by any js, but not marked as HttpOnly.
Thanks again for reviewing, the above is I guess some considerations for why you might want to consider that I should probably have added the context for earlier.
The need to change this came out of a PEN test where it wasn't needed by any js, but not marked as HttpOnly.
Hmm, that's odd. I suspect you had something else somewhere else setting it this way.
Yes, we set the session cookie to be HTTP only by default. There is more than one way to configure cookie options in ASP.NET, so as Brock says, something else is probably disabling HttpOnly. But nothing in IdentityServer itself should be doing that.
@josephdecock Line 271 of DefaultUserSession.cs sets HttpOnly to false... that's why I wanted this configurable.
var options = new CookieOptions
{
HttpOnly = false,
Secure = secure,
Ok, I think I understand. There are two different cookies in IdentityServer. One is the session cookie, which behaves as I said (it is not available to JavaScript). Http requests to identity server are authenticated with this cookie.
The other cookie in IdentityServer is the check session cookie, used at the check session endpoint. That's what the idsrv.checksession cookie is. The code that you're looking at where we set httponly to false is doing so for that cookie specifically. This cookie and endpoint are our implementation of the OpenId session management specification, which needs a JavaScript accessible cookie by design (and by the spec).
We don't regard the check session cookie as a particularly significant security threat, because it is a really well known and very commonly supported standard protocol mechanism. But if you don't want to use the check session endpoint, then the check session cookie does become unnecessary. In that case then the cookie options that you're are fine as a way to make the cookie unavailable to JavaScript. You should also disable the check session endpoint in endpoints options, since it won't work properly without a cookie available to JavaScript.
Actually, I just learned something new, which is that we actually don't issue the check session cookie at all if the check session endpoint is disabled. So, disabling the endpoint remains my recommendation since you're not using the endpoint. But then you don't need to do your workaround with the http only cookie flag.
Thanks for the replies @josephdecock, I agree that the cookies that didn't have HttpOnly set are not security threat, however having cookies not accessed by any js without this flag result in a black mark in PEN findings 😅. We've solved our need for it by setting the global cookie policy for the app, it would have just been nice having this configurable in the IDS platform.
The cookie we were looking at was idsrv.session
which had HttpOnly as false, in the PR I just updated both places I saw cookies being set so that it was fully configurable.
The other change we put in the cookie policy was changing SameSiteMode to Lax (to keep 3rd party logins working still) which seems to keep all our usage of IDS working, and is something that is configurable in the existing code
We've solved our need for it by setting the global cookie policy for the app, it would have just been nice having this configurable in the IDS platform.
Well, given the nature of that cookie it makes no sense to have it HTTP-only. It's designed in the protocol to only be useful to JS running in the browser. Sadly most of those pen tests are unaware of the underlying security protocols.
Anyway, the approach of disabling the check session endpoint, which @josephdecock suggested, is really the right way to disable this feature. You're not using the cookie, so why have it issued at all?
Currently the HttpOnly property of a cookie is not configurable, the addition to this small change will allow us to configure this when setting up Identity Server