Saml2: Multiple "Kentor." or "Saml2." cookies

[robinvanpoppel]

In our current solution we give the user an option to raise his security level (stepup). The code sends an authnrequest and sets a cookie starting with "Kentor". This cookie will only be cleaned up when the user completes the stepup, or if the user closes the browser. This leads to HTTP 400 - Bad Request (Request Header too long) when the user sends a lot of cookies. Automatically expiring these cookies would reduce the chance of this happening. Is there a reason why the cookies don't have a (configurable) expiration?

The code below sets the cookie:
https://github.com/KentorIT/authservices/blob/25a7ec8c1afeacfe681a19b5001da86f39d91cf6/Kentor.AuthServices.HttpModule/CommandResultHttpExtension.cs#L91-L96

Thanks in advance!

[explunit]

IdentityServer3 and the OpenId Connect middleware have a similar issue, i.e. balancing the ability to have multiple login tabs open in your browsers vs. overflowing the request size. The approach used there is to set a max cookies to keep, e.g. https://github.com/IdentityServer/IdentityServer3/blob/2.5.4/source/Core/Configuration/Hosting/MessageCookie.cs#L220

I don't know why expiry wasn't used, but it seems to me it wouldn't fully solve the problem, in that users could still open up quite a few login requests in a short amount of time.

A completely different approach might be to abstract away the command result cookies into IRelayStatePersistence and CookieRelayStatePersistence (used by default) so that developers can supply MyCustomDatabaseRelayStatePersistence instead and bypass any limits. #587 takes some small steps in that direction.

[AndersAbel]

Getting many Kentor.* cookies is typically a problem only for developers doing repeated sign in attempts that are aborted. So far I've not heard of this actually affecting users. In what scenario do you run into issues with this?

The problem with cookies is that you either can set an expiry time or you can let them auto expire when the browser is closed. Ideally I'd like to have both, but when implementing this I chose to have them as session cookies to get them removed on a browser restart. If a timeout is to be set, the next question is what that timeout should be.

Adding an automatic cleanup would probably require some kind of data to be added to the cookie (before encryption) that contains a time stamp to know what cookies to keep.

[robinvanpoppel]

I agree that this is kind of an exotic condition. For now I've only heard back from a few cases.

We are running our site on a subdomain. The toplevel domain already sets a few cookies. IE en Edge have a different behavior than other browsers and also send the top level cookies to our subdomain. That combined with the large WIF-cookies, the loadbalancer cookies for sticky sessions it grows quite rapid. I did a quick test with just logging in in the system. In chrome I receive about 3000 bytes of cookies. In IE after visiting the main site first I have around 3900 bytes of cookies. That leaves little room.

On top of that I've experienced that not all browsers clear the session cookies upon closing. Chrome for instance does this when you check the option "Continue where you left off".

[explunit]

I am hearing reports of this from real users (not developer scenario) since some people rarely close their browser. Over time the cookies from abandoned login redirects accumulate and cause the Request Too Long error. The concept of Session cookie seems outdated now that browser windows remain open for days or weeks.

[AndersAbel]

Ok, then we need to do something about it.

• Either we move to one single cookie name and store the relay state in the data instead. Pro: Simple solution. Con: Disables possibility to do multiple sign ons from same domain (possibly different ports).
• Or we implement a routine that removes stale AuthnRequests after a set time - might be through a normal cookie life time or still use session cookies to get auto-clear-on-exit but contain a time to live field in the data.

Opinions?

[explunit]

The second option is consistent with how IdentityServer3 works. Since AuthServices is often used with it, it's an approach that would be familiar to developer already.

But as part of this change it might be a good time to abstract the relay state persistence so that if somebody wants a different approach (e.g. database) they are free to do that. See my comments earlier in this issue.

[AndersAbel]

Doing like IdentityServer3 is often a good idea.

Enabling other mechanisms for relay state persistence is harder as CSRF (Cross Site Request Forgery) protection requires a cookie to be set, which must be correlated to an id passed in the SAML request. Without a cookie, it would be possible for me to initiate an AuthnRequest, get the server to store the state, and then do a CSRF attack by doing the GET in an iframe of a site. Login CSRF can be used to easier access other CSRF vulernabilities in a site. So I'm a bit concerned about allowing in memory storage as it would encourage CSRF vulnerable behaviour.

Then there is currently lacking CSRF protection on the SignIn and Logout commands - but that is something that will be fixed eventually (#403).

[explunit]

One more source of ideas: The Katana OIDC middleware lets you specify an ICookieManager

The built-in/default cookiemanager could do the identityserver-style automatic removal of cookies over the specified threshold.

And if that's not good enough, a developer could optimize things further by writing their own ICookieManager that writes the cookies using the names specified by AuthServices (for the CSRF protection you mention) but serializing/deserializing the contents elsewhere.

[RobSiklos]

If you allow developers access to modify the CookieOptions (see https://github.com/Sustainsys/authservices/issues/725), then we could specify our own expiry via that mechanism.

[alexandreabeh]

Any update on this issue? This is a valid concern for my team as well.

[chrisacameron]

my users are also beginning to encounter this issue

[yakovenkoroman1993]

Problem still lives

[AndersAbel]

What about limiting to one cookie per Idp? That way multiple tabs could still have active sign in flows to different idps. But only one active sign in flow to one Idp. Would that make sense?

The technical solution would be to use first X characters of BASE64-encoded SHA1 of EntityId of Idp in the cookie name.

[krompaco]

Hi @AndersAbel

I'm seeing this issue currently and I can reproduce by not completing the signin enough number of times. Of course this in combination with the app's other cookies.

Trying to fix my issue using an adaptation of what's talked about here: https://github.com/IdentityServer/IdentityServer3/issues/1124

var saml2Options = new Saml2AuthenticationOptions(true)
{
    Notifications = new Saml2Notifications
    {
        AuthenticationRequestCreated = (request, provider, dictionary) =>
        {
            const int Threshold = 1;
            var owinContext = HttpContext.Current.GetOwinContext();
            var existingCookies = owinContext.Request.Cookies.Where(kvp => kvp.Key.StartsWith("Saml2.")).ToList();

            if (existingCookies.Count >= Threshold)
            {
                for (var i = 0; i <= existingCookies.Count - Threshold; i++)
                {
                    owinContext.Response.Cookies.Delete(existingCookies[i].Key);
                }
            }
        },

Does this look OK too you?