chunkingcookiemanager leaks unused chunks

[patrick-hampton-avalara]

There seems to be a bug in Microsoft.AspNetCore.Authentication.Cookies.ChunkingCookieManager when used in conjunction with CookieAuthentication. What I've seen is that as the size of the Authentication cookie changes the number of chunks needed to the accommodate the cookie data will change as well. If the number of cookies needed decreases, then chunks that are no longer needed are not being cleaned up.

Normally this isn't a big deal, but I'm currently running in a system that has a hard limit on the header size of 10240 bytes. The specify scenario I'm running into has the Authentication cookies going from 2 chunks (3 total cookies) to 1 chunk (1 cookie). This has the added bonus of replacing the value in the cookie that previously stored the chunk count (really small), to now storing the entire Authentication Ticket (relatively large), in turn nearly doubling the amount of header space consumed by the Authentication cookie.

It seems like it could be beneficial to have ChunkingCookieManager expire these "vestigial" as it's setting the new chunks on the response.

[Tratcher]

What triggers the resize, a direct call to SignIn? The workaround would be to preceed that with a call to SignOut.

[patrick-hampton-avalara]

I haven't actually been able to diagnose that yet, because it doesn't happen consistently. What I can say is that it's not tied to a SignIn or SignOut action because the users are in the middle of an active session using the application. This leads me to believe/fear, that something else in one of the middle-ware's is manipulating and dropping some of the claims in the Authentication Ticket, but I can't see where. If that's the case this becomes a different issue for me, though it doesn't necessarily take away from this issue.

[patrick-hampton-avalara]

I've figured out why the Authentication Cookie is changing size. We're using OIDC and OAuth in our application, OIDC to manage login, and OAuth to call services outside the application. For this to work, we had to implement code to perform sliding expiration on the Access Token used in the OAuth flow so that we would always have a valid access token during a valid application session.

The service we're calling to get and refresh the tokens doesn't always return tokens of the same size. Now, it's entirely possible that such behavior is invalid, but I don't have control over that service so it is what it is at this time. This combined with the fact that our Authentication cookie is right on the boarder of being 4K in size means that as the size of the tokens changes it could push us above or below the threshold for needing to chunk the Authentication Cookie.

[Tratcher]

When the token is refreshed the changes are persisted with a call to SignIn, right? Then the SignOut mitigation should still apply.

[patrick-hampton-avalara]

As far as I know SignIn doesn't come into play. The refresh logic happens in the OnValidatePrincipal event of the CookieAuthenticationOptions. The tokens are then updated on the AuthenticationProperties of the CookieValidatePrincipalContext using the StoreTokens method.

[Tratcher]

Ah, you set CookieValidatePrincipalContext.ShouldRenew?

[patrick-hampton-avalara]

No. We're not explicitly renewing the cookie in the event because we set SlidingExpiration to true when we setup the Cookie Authentication Middleware. Keeping in mind that we're using OIDC to log in via an identity provider then using the AccessToken provided by that identity provider to call Api's that are external to the application, but secured by the same IdentityProvider. The application is not using the AccessToken in the cookie to secure itself, because it doesn't need to. We're only keeping it around to facilitate the Api calls. As such I'm not aware of a way to tell the Oidc Middlware to maintain an unexpired cookie. Thus why we implemented our own code inside OnValidatePrincipal. Here's what that looks like

//properties is of type Microsoft.AspNetCore.Authentication.AuthenticationProperties passed in from the HttpContext
var refreshToken = properties.GetTokenValue(OidcConstants.TokenTypes.RefreshToken);
var expiresAt = GetExpiresAtUtc(properties);
var utcNow = DateTime.UtcNow;

if (!string.IsNullOrWhiteSpace(refreshToken) &&
    expiresAt.GetValueOrDefault(DateTime.MaxValue).Subtract(utcNow) < _renewTimespan)
{
    var tokenResponse = await RefreshAccessToken(refreshToken);

    //The initial token we get at login has an expiration that's precise to the second, so for consistency we'll do the same for renewals
    var utcNowTrimmed = new DateTime(utcNow.Ticks - (utcNow.Ticks % TimeSpan.TicksPerSecond), utcNow.Kind);
    var newExpiration = utcNowTrimmed.AddSeconds(tokenResponse.ExpiresIn).ToString(DateFormat);

    properties.StoreTokens(new List<AuthenticationToken> {
        new AuthenticationToken
        {
            Name =  OidcConstants.TokenTypes.AccessToken,
            Value = tokenResponse.AccessToken
        },
        new AuthenticationToken
        {
            Name = OidcConstants.TokenTypes.RefreshToken,
            Value = tokenResponse.RefreshToken
        },
        new AuthenticationToken
        {
            Name = OidcConstants.TokenTypes.IdentityToken,
            Value = tokenResponse.IdentityToken
        },
        new AuthenticationToken
        {
            Name = OidcConstants.TokenResponse.TokenType,
            Value = tokenResponse.TokenType,
        },
        new AuthenticationToken
        {
            Name = ExpiresAtTokenName,
            Value = newExpiration
        }});

If we've gone off the rails on the implementation here, then we're open to ways to simplify the code as well.

[Tratcher]

Updating properties is fine, but those changes arn't saved anywhere unless you call SignIn or ShouldRenew. You could get lucky and make this change on a request that naturally triggers a sliding refresh, but don't count on it.

[patrick-hampton-avalara]

Ah, I did find where we're calling ShouldRenew, it's just one layer up in the stack from where the code above was. Anyway, I think we've gotten a little side tracked. Thinking about the SingOut + SignIn workaround you mentioned, I assume SignOut will expire the old cookie followed by SignIn to issue a new one. Are there other side effects we should be aware of if we go that route?

[Tratcher]

Note SignOut will apply a redirect in some scenarios.

I guess you could also call DeleteCookie directly:
https://github.com/aspnet/Security/blob/42dd66647dd389c242ff796a530b880c7b9d7a90/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L363-L366

[Eilon]

This certainly sounds like a bug. It would make sense for the cookie manager to clean up unnecessary cookies as part of setting the new cookies.

[patrick-hampton-avalara]

Quick follow up. For a workaround I did try using DeleteCookie prior to calling AppendCookie, I actually ended up wrapping ChunkingCookieManager in a new implementation to do this inside the new classes AppendCookieMethod and setting the new class as the CookieManager in startup. Anyway, This worked great locally, but when we deploy to our AWS env's it caused issues. It appears the order in which the Set-Cookie headers is sent back may not be guaranteed, or was manipulated by the AWS infrastructure. So Calling Delete followed by Append could create multiple Set-Cookie headers for the same cookie leading to unexpected events.

So instead I created a helper method that would inspect the Request cookies and the Response cookies for a given key and remove any that existed in the Request but not the Response. I think called this helper method in the wrapper class after calling the base AppendCookie Method. This worked across all our envs:

public class CleanChunkingCookieManager : ICookieManager
{
	private const string ChunkSuffix = "C";

	private readonly ChunkingCookieManager _innerCookieManager = new ChunkingCookieManager();

	public void AppendResponseCookie(HttpContext context, string key, string value, CookieOptions options)
	{
		_innerCookieManager.AppendResponseCookie(context, key, value, options);
		RemoveVestigialChunkCookies(context, key, options);
	}
       ...
	private static void RemoveVestigialChunkCookies(HttpContext context, string key, CookieOptions options)
	{
		var deleteOptions = new CookieOptions
		{
			Domain = options.Domain,
			Path = options.Path,
			HttpOnly = options.HttpOnly,
			IsEssential = options.IsEssential,
			Expires = DateTime.UnixEpoch,  //Jan 1, 1970 forces deletion
			MaxAge = TimeSpan.Zero,  //Max age is zero.  Secondary value to force deletion
			SameSite = options.SameSite,
			Secure = options.Secure
		};

		string searchString = $"^{key}({ChunkSuffix}\\d+)?";
		var requestChunkNames = context.Request.Cookies.Keys.Where(k => Regex.IsMatch(k, searchString));

		var responseCookies = context.Response.Headers["Set-Cookie"];
		var responseChunkNames = responseCookies.Where(v => Regex.IsMatch(v, searchString)).Select(v => Regex.Match(v, searchString).Value);

		var vestigialChunks = requestChunkNames.Except(responseChunkNames);
		foreach (var chunkName in vestigialChunks)
		{
			context.Response.Cookies.Append(chunkName, string.Empty, deleteOptions);
		}
	}
}