Fix invalid cookie name caused by chars in path name

[Skrypt]

Closes #11489

[Skrypt]

@jtkech If I understand correctly you used the ContentRootPath because you wanted to have the same Cookie name when trying to use the setup from different served paths. Using a GUID here might not fix that issue but it is more secure as it cannot be forged.

Though, I suggested using an MD5 or any other hash to create that UID based on that ContentRootPath to at least make it a little more complex to guess the name of the cookie. Though, I think that we should leave it as a random GUID.

[jtkech]

@Skrypt

Yes, if multiple instances where we can use e.g. the Redis distributed data protection provider, to not have to login again when the request is balanced to a new instance, also when submitting a form if no affinity is used, this let me think about this issue #11501 where @CrestApps tried to not use ARR affinity.

I'm not a security expert so not sure, but this is only a cookie name but whose value is encrypted, otherwise I don't see how we could share a data protection mechanism accross instances.

---

Hmm but look at this, just saw in #5803, first I just added ContentRootPath

 var cookieName = "orchantiforgery_" + settings.Name + environment.ContentRootPath;

Then @sebastienros asked to me Doesn't it need to be url encoded? and I answered Yes, but as i saw it is done automatically by aspnetcore, but finally added the UrlEncode() saying maybe better to do it explicitly.

Hmm, maybe aspnetcore would have done the right and adapted encoding, so maybe we just need to remove the UrlEncode() that we finally added in a second step.

Maybe worth to give it a try ;)

[Skrypt]

Ok, a little bit of explanation here first. We can't use a method that is meant for "slugifying" or escaping a URL here because eventually, these methods could allow other chars that we don't want. We need a specific method here that is not related to a URI transformation. The best thing is to always use a Unique ID for these cookie names so I suggested something like a MD5 against the ContentRootPath that could return always the same unique chars and that would not need to be "escaped". Else, we will need to simply not use the ContentRootPath as is and remove unrestricted chars that could come from a path but these could change too from one system to another so the best way is to transform the ContentRootPath to some hash.

Else, here the issue with Guid.NewGuid().ToString("N") is that it will be a different GUID per Azure instance so if the Request is sent from Instance A to Instance B; the Antiforgery cookie name will be different and fail to process the request even though the Antiforgery tokens are equal.

Sebastien Ros suggested use Base64 at some point but looking at it and it is not escaping all chars we don't want.
We want only letters and numbers, so a GUID is what we want. So to make it unique I think we can also use new Guid(data)

Something like this :

    MD5 md5Hasher = MD5.Create();
    byte[] data = md5Hasher.ComputeHash(Encoding.Default.GetBytes(environment.ContentRootPath));
    var guid = new Guid(data).ToString("N");

[jtkech]

This is not exactly what I said, I just mentioned an old comment where I said that I saw that aspnetcore was encoding the whole cookie name, and if so maybe would be more adapted and we would just have to remove the UrlEncode() we added at some point. But yes good catch for the ContentRoot path that may be different.

But just saw this breaking change in their doc saying that this is no more the case https://docs.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/5.0/security-cookie-name-encoding-removed

They link to a related issue, I will take a look when I will have time.

For now I saw this

Recommended action
Applications moving to .NET 5.0 should ensure that their cookie names conform to the token spec requirements: ASCII characters excluding controls and separators "(" | ")" | "<" | ">" | "@" | "," | ";" | ":" | "" | <"> | "/" | "[" | "]" | "?" | "=" | "{" | "}" | SP | HT. The use of non-ASCII characters in cookie names or other HTTP headers may cause an exception from the server, or be improperly round tripped by the client.

[Skrypt]

Here is the result of this PR with the latest commit which should fix the entire issue.

[Skrypt]

@jtkech What I meant is that the ContentRootPath allowed chars could be different from Windows to Linux for example.

https://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names

So, basically ASCII seems safe here but hashing with a MD5 also does the trick.

We could also do simply : byte[] bytes = Encoding.ASCII.GetBytes(environment.ContentRootPath);

[Skrypt]

Ok somehow the new Guid(byte[]) method requires a 16 byte array to generate a Guid from and the MD5 hasher returns exactly that. I'm going to keep it as it is because it produces a consistent byte array for a given string.

[jtkech]

Here some findings, just for info, nothing incompatible on what you're working on ;)

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;