Handle case of stale xsrf cookie

[ShannonKasperNoesis]

This fixes an issue I ran into where the protected data was no longer available. A new cookie will now be generated for this case.

[brockallen]

I have been thinking about this issue and if we should remove the xsrf cookie if there's a problem. I came to the conclusion that we should not remove the xsrf cookie if there's a crypto error. Mainly because the protected data should be consistent. If you're getting protected data changes then either you're in development, or you're in production and haven't configured the server the way you should.

If we dropped the cookie if there's a crypto error, then an attacker in another tab could spam the idsvr endpoint and cause the legit cookie to be removed.

[ShannonKasperNoesis]

Is there a different way to more gracefully handle this? I'm worried that if something did happen in production then it would require the users to know to clear their cookies when a login error occurs.

In the case where an attacker spammed the idsvr endpoint from another tab, that would just prevent the attacked user from logging in. Alternatively, a server restart or some other reason for a loss of protected data could cause a large amount of users to run into errors trying to login.

[brockallen]

All the user would have to do is close their browser.

As for a server reset, I'd argue you should have a consistent <machineKey> element properly configured. If your server restarts and your <machineKey> changes then many things break (not just xsrf protection).

[ShannonKasperNoesis]

If an attacker spammed the idsvr endpoint from a second tab, wouldn't they still be sending the same xsrf cookie as the original tab (stale or valid)? If the cookie was valid nothing should be replaced, correct? I'm not sure if I follow on the potential for an attack.

Thanks for the tips on using the <machineKey> element. I'm using Mono and am worried there may be some differences to sort out.

In either case, I would like to avoid the situation where a user may be locked out of the system until their cookie is cleared. I've run into situations with Chrome where closing the browser doesn't clear the sessions cookies because Chrome runs background processes that don't close with the browser window.

[brockallen]

The attack would be where a user has a form where the hidden input is linked to the old xsrf cookie.

We'd love to hear how your progression on mono goes.

As for a solution, I don't have a good answer. But given the potential for abuse, I don't think removing the cookie automatically is the right approach.

[ShannonKasperNoesis]

Just to clarify, my code change will only replace the cookie if the protected data is missing or can't be decoded. The result of the comparison between the hidden input and the cookie will not cause the xsrf cookie to be replaced. If the xsrf cookie was stale/invalid it should only be updated at most once. The attacker (second tab), subsequent to its first request, would then be spamming with a valid xsrf cookie and my code change would not change the behavior.

The attack would be where a user has a form where the hidden input is linked to the old xsrf cookie.

Correct me if I'm wrong, but I don't believe this situation is possible because the browser is in control of the xsrf cookie and the server will only replace the cookie if it is stale/invalid (which should happen at most once).

[brockallen]

Right -- if the attacker submits missing data, then won't the changes in the PR cause the xsrf cookie to be re-issued? If that's the case, then the attacker can do things that cause the xsrf cookie to be changed. That's the attack.

Or am I still not understanding (which is possible)?

[RyanMelenaNoesis]

Unless I'm confused, the attacker should only able to make changes to the hidden input value and not to the xsrf cookie. I believe that is the raison d'être of the xsrf cookie.

This PR should cause the server to re-issue the cookie only in the case that it finds the cookie to be 'invalid'. Perhaps the confusion may be coming from the overloading of the term 'invalid' here. In the context of this PR we are referring to the cookie as 'invalid' if the server can not decode/unprotect it properly. If the cookie can be decoded/unprotected but does not match the hidden input value then the cookie would be considered 'valid' (although mismatched) and would not be re-issued.

[...] if the attacker submits missing data, then won't the changes in the PR cause the xsrf cookie to be re-issued?

If by "submits missing data" you mean submits a mismatched hidden input value then no, the xsrf cookie would not be re-issued in that case.

Previous to the PR the server said:

If I see an xsrf cookie I'll decode/unprotect it.  Otherwise, I'll generate a new xsrf cookie.

With the PR the server would say:

If I see an xsrf cookie that I can decode and unprotect I'll decode/unprotect it.  Otherwise, I'll generate a new xsrf cookie.

Or at least that is the intent, please advise if we have missed something here.

[brockallen]

So I spent some time looking at the PR. I guess it seems that you're assuming the data protector will return null if it can't decode the value. Are you certain this is how it works?

[ShannonKasperNoesis]

After debugging and digging a little further it seems that the default Owin data protector (DpapiDataProtector) will actually throw an exception (I believe CryptographicException) when failing to unprotect data. We were using our own implementation of an IDataProtector that was compatible with Mono which handled the exception and returned null. We have since refactored our MonoDataProtector to behave more like the DpapiDataProtector. Additionally we have found the issue that was causing our protected data to be unavailable after an app restart. The code for our MonoDataProtectionProvider and MonoDataProtector can be found on my colleague's blog post about using IdentityServer v3 with Mono.

So the current PR doesn't work as intended with the default Owin data protector. I still think it might be worthwhile to handle the crypto exception, although as you said it shouldn't happen unless you have something misconfigured. I can submit a new PR if you think it might be useful.

[brockallen]

I worry that swallowing the exception will hide the fact that something is incorrectly configured. Microsoft's antiforgery token handling takes the other approach -- if there's a problem it just throws and doesn't try to recover at all. As a compromise, the way the code's written now it will handle the exception and log the failure.

Let me think some more about the approach you're proposing (which basically means re-issue the cookie if the unprotect fails).

[brockallen]

Just did this checkin: f9b85bd

Mind taking a look to see if it provides the enhancement you were looking for? Thx.