Review generation of UID in DefaultClaimUidExtractor

[rynowak]

According to @blowdart and @lodejard the way we generate the UID for a ClaimsPrincipal is wrong in antiforgery.

We should use the ClaimsIdentity.NameClaimType of all authenticated identities to generate the UID rather than looking for a claim of type ClaimTypes.NameIdentifier.

We need to also figure out the right thing to do (or if it's possible) when the user does not have a name.

[Eilon]

@ajaybhargavb please discuss with @Tratcher to understand the multi-name and no-name scenarios.

[leastprivilege]

The Name claims is a display name - you want to use a claim that has the unique id of the user - that would be the sub claim (https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims) - but since you are stuck in medieval aka WS* times - NameIdentifier comes closest.

Historically Name or IIdentity.Name had no clear semantics and was often used for one or the other - the MS templates used it to display the "unique" name - e.g. "Hello Domain\User" (which is perfectly natural language ;))

IOW - the Name claim is not the right one - the templates should set both nameid and Name - and use NameId for anti-forgery to educate people. If NameId is not set - thrown an exception.

Now give me your "hobbyist user" arguments ;p

[blowdart]

OK so.

Look for a "sub" claim first. If it's present use that, the value and the issuer.
If it's not present look for a ClaimTypes.NameIdentifer property. If it's present use that, the value and the issuer
If that's not present look for ClaimTypes.Upn. If it's present, use that, the value and the issuer

And then if nothing of the above is present, sha256 the entire identity.

Throwing seems rather drastic. Keep in mind we're not using the identity alone, it is combined with a random value.

That work @leastprivilege ?

[brockallen]

The one last comment about this is that using the authenticated user's identifier tends to break antiforgery once the user toggles between anon and authenticated (as it did in prior versions of MVC).

[blowdart]

Added bonus - make sure you check all identities on the principal, and have configuration so a developer can specify a claim to use if needed.

[blowdart]

Oh wait, hmmm, yea, when a user logins the CSRF token would no longer be valid. Argh. That would affect login pages only though/

[brockallen]

Or if user has 2 tabs open. But sure, it's a narrow window of problems, but we saw them every once in a while with MVC 4 & 5 (with IdentityServer2). If there was some facility that could tell if the user's csrf token was authenticated vs not and reg, that might be useful. Then again, it might just be too complicated, and/or open to some abuse by an attacker in some oddball way.

[blowdart]

I'm not so worried about the two tab problem - if they already have a token, which they would get on the first request, we'll reuse that. It doesn't change between pages.

[Tratcher]

No, but your auth cookies can be changed by actions in one tab while you're filling out the form on the other one.

[blowdart]

True. I'm kinda ok breaking at that point, if you've logged in on one tab and not another

[brockallen]

Imagine you submit a login (current request is anon, so you have an anon antixsrf). The result of the login page (for whatever reason) does not redirect, but renders a new form. That form's anti-xsrf is under the impression that you're anon, but now you've got an auth cookie. When you submit this newly rendered page they don't line up.

That was the scenario I saw people hitting (more than one) in the prior versions of MVC.

[blowdart]

Are we agreed though that the token should regenerate / change when someone logs in?

[brockallen]

I guess I never understood why the user's identity was incorporated into the antixsrf token. RoR does not do this. What's the additional protection from doing this (if I may ask)?

[blowdart]

Well to be fair rails doesn't do auth, that's left to 3rd parties.

It's to stop CSRF dropping an auth cookie for an authenticated user by an attacker, which can then be used to produce a known value.

[lodejard]

/cc @GrabYourPitchforks as well in case he has addtl specifics on the history

I guess I never understood why the user's identity was incorporated into the antixsrf token. RoR does not do this. What's the additional protection from doing this (if I may ask)?

[blowdart]

I just emailed him :) So the cookie part of the token is basically a session identifier. It's only the form based half that contains the UID part, so login works just fine.

[brockallen]

Well to be fair rails doesn't do auth, that's left to 3rd parties.

Well, this repo for anti-xsrf is also arguably independent from the rest of ASP.NET, no? Meaning, can you assume the use of cookie authentication? Maybe you can... just thinking out loud (not arguing for anything).

It's to stop CSRF dropping an auth cookie for an authenticated user by an attacker, which can then be used to produce a known value.

But if the login form has xsrf, then wouldn't that protect against this? Also, how would the xsrf attacker get a known value if it's CRNG? I guess I'm missing the attack vector.

[blowdart]

No, it's not assume cookie, it'll use any valid identity on the principal.

Anon users get a simple match, drop csrf cookie with value. Match it against the value in the form. So, you can set it if you got XSS, and lo, the next form generated will use it.

[brockallen]

No, it's not assume cookie, it'll use any valid identity on the principal.

I meant to re-gen the antixsrf when the identity changes. Would that be done somehow related to the cookie MW, or do you detect if the identity is different?

So, you can set it if you got XSS, and lo, the next form generated will use it.

Well, if there's XSS then you have bigger problems.

[blowdart]

It also protects against supercookies, i.e. someone on badsite.azurewebsites.net dropping a CSRF cookie for azurewebsites.net

[brockallen]

someone on badsite.azurewebsites.net dropping a CSRF cookie for azurewebsites.net

LOL that's what they get for running in a shared host name/trust boundary. same for localStorage, sessionStorage, CSP, etc... :)

[dougbu]

In the default Antiforgery configuration, the cookie token contains no identify information. That's left to the request token (aka form or header token). And the request token is regenerated in every request.

Put another way, IClaimUidExtractor.ExtractClaimUid() is used at the end of a request as part of request token generation. If the user's going to get logged in, they're already logged in. If the same method is used at the start of the next request (i.e. prior to login changes) to validate the incoming token, life is good.

We aren't there yet. But the "logging in breaks tokens" problem will go away once validation occurs early enough. @rynowak is working on this.