Cookie name per Application and accross its Instances

[jtkech]

Fixes #11489
Maybe fixes #11501

Here the copy of my last comment in #11526

First I was asking to me if the cookie name itself needs to be encrypted if aspnetcore uses the right secure options / policies when building the cookie. So as i saw before they were using Uri.UnescapeDataString(cookie.Name.Value) to just encode (not encrypt) the cookie name which is no more the case.

About the new vulnerability, I saw that cookies can be prefixed with __, i.e. __Host and __Secure, meaning that the browser will use additional policies, but because they were decoding the whole including the cookie name, it was possible to use __%48ost and __%53ecure without involving the same policies.

dotnet/aspnetcore#23578
https://fletchto99.dev/2020/december/cookie-vulnerability/

the flaw allowed for a __%48ost- or __%53ecure- cookie to be set without meeting the required attributes (I.e. set without HTTPS, from root domain, or from a secure page). This means a malicious cookie set by an attacker could potentially craft a malicious __%48ost- and set it on their victim.

That's why now they don't encode / decode anymore. So my feeling is that we don't need to encrypt the cookie name, just escape it and avoid the not allowed chars as you are working on.

---

Just saw this data protection utility GetApplicationUniqueIdentifier https://github.com/dotnet/aspnetcore/blob/181db595bb11ef99e3a9167e7b940124363c1e36/src/DataProtection/DataProtection/src/DataProtectionUtilityExtensions.cs#L41 using IApplicationDiscriminator https://github.com/dotnet/aspnetcore/blob/a450cb69b5e4549f5515cdb057a68771f56cefd7/src/DataProtection/DataProtection/src/Internal/HostingApplicationDiscriminator.cs#L18-L23 which uses ContentRootPath ;)

---

That said I remembered that when I added ContentRootPath to the cookie name this was not to have the same name accross instances of the same application, but instead to have different cookie names to prevent logged errors when we run, i.e. on a local machine for development, different applications but with the same url.

when we run an application from a given location and login to the admin, and then do the same from another application location but same url.

But somewhere it "broke" the use case where we want the same cookie name accross instances but for the same application, as discussed in #11501 and as you said where ContentRootPath may be different.

So yes, would be great to have an application identifier to be used as a suffix for each tenant name, maybe a secret suffix from configuration whose default would be ContentRootPath.

Oh yes, just remembered that we have a shell setting Identifier, renamed at some point to VersionId, and in a distributed env the ShellSettings are persisted in a shared store. So maybe we could use it in place of ContentRootPath, we may still need to escape the settings name, or just only use the VersionId. as it varies per tenant for a given application. So one solution would be to just use.

var cookieName = "orchantiforgery_" + settings.VersionId;

[Skrypt]

To validate if it is valuable to use instead :

var cookieName = "__orchantiforgery_" + settings.VersionId;

To add additional policies.

[Skrypt]

Now that it works for most use cases. The only remaining issue is that the VersionId will change on each tenant reload which can cause a form post that uses an old antiforgery cookie/token to fail posting. This should instead use an "Id" that is created on the tenant creation which will never change in it's entire lifetime. We can't use the tenant name and use an MD5 on it to hash it because it could be the same resulting GUID for different tenants used in development like the "default" tenant. So, we found that it would be probably best to create a GUID that is stored in the tenants.json file just like we do with the VersionId.

So, while it fixes the issue for now maybe we should consider adding also this ShellSettings.Id parameter.

[jtkech]

Yes,

Only one precision, the VersionId is created when creating the tenant, it changes when editing and saving the tenant, when enabling or disabling the tenant. The VersionId doesn't change on other loading / reloading cases as on startup, on the reload action of the Tenants page, when enabling / disabling a feature and so on.

[Skrypt]

This line can be removed

[jtkech]

Good catch, thanks I'm removing it #11544

[ns8482e]

when the version id change will it make existing cookie invalidate then?

[jtkech]

#11535 (comment)

the VersionId is created when creating the tenant, it changes when editing and saving the tenant, when enabling or disabling the tenant. The VersionId doesn't change on other loading / reloading cases as on startup, on the reload action of the Tenants page, when enabling / disabling a feature and so on.

[Skrypt]

Yes, it will invalidate the cookie since it will regenerate a new cookie with a new anti-forgery token. Though, the name of the cookie will remain the same. And the lifetime of the cookie is the session lifetime.