No nonce verification for code flow?

[andkorsh]

As far as I understand, nonce stored in the cookie to make sure the authentication response is coming from the same same client which originally initiated the request.

I'm using code flow and it seems like even though nonce cookie does get set, it's not really used and I successfully get authenticated no matter if I had the nonce cookie or not. Looks like one of the reasons is this line:

https://github.com/aspnet/AspNetKatana/blob/main/src/Microsoft.Owin.Security.OpenIdConnect/OpenIdConnectAuthenticationOptions.cs#L62

By this, if the response has been intercepted somewhere in the middle, an attacker can go and use the auth code without the additional layer of security the nonce provides.

Not sure if this is by design or is a bug?

Below is my config - please pay attention to the DiscardCookieManager that basically discards any cookies the middleware is trying to set:

public void Configuration(IAppBuilder app)
{
    app.UseOpenIdConnectAuthentication(new OpenIdConnectAuthenticationOptions
    {
        Authority = oidcAuthority,
        ClientId = oidcClientId,
        ClientSecret = oidcClientSecret,
        RedirectUri = applicationUrl + "signin-oidc",
        PostLogoutRedirectUri = applicationUrl,
        Scope = OpenIdConnectScope.OpenIdProfile,
        ResponseType = OpenIdConnectResponseType.Code,
        RedeemCode = true,
        SaveTokens = false,
        AuthenticationMode = AuthenticationMode.Passive,
        CookieManager = new DiscardCookieManager(),
        TokenValidationParameters = new TokenValidationParameters
        {
            AuthenticationType = OpenIdConnectAuthenticationDefaults.AuthenticationType,
            NameClaimType = oidcNameClaimType
        },
        Notifications = new OpenIdConnectAuthenticationNotifications
        {
            AuthenticationFailed = OnAuthenticationFailed,
            SecurityTokenValidated = OnSecurityTokenValidated
        }
    });
}

class DiscardCookieManager : ICookieManager
{
    public void AppendResponseCookie(IOwinContext context, string key, string value, CookieOptions options) { }
    public void DeleteCookie(IOwinContext context, string key, CookieOptions options) { }
    public string GetRequestCookie(IOwinContext context, string key) => "";
}

private Task OnAuthenticationFailed(AuthenticationFailedNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions> n)
{
    // I get here second.
    return Task.FromResult(0);
}

private Task OnSecurityTokenValidated(SecurityTokenValidatedNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions> n)
{
    // I get here first.
    return Task.FromResult(0);
}

While I still do get the error callback (from validation that's happening here: https://github.com/aspnet/AspNetKatana/blob/main/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L529), this is not enough because a user has already been validated by ID token and the ticket has already been created.

[Tratcher]

What error do you get in the error callback?

[andkorsh]

Hi @Tratcher

I get this, but this is from the auth token validation (not ID token validation):

Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolInvalidNonceException: IDX21323: RequireNonce is '[PII is hidden]'. OpenIdConnectProtocolValidationContext.Nonce was null, OpenIdConnectProtocol.ValidatedIdToken.Payload.Nonce was not null. The nonce cannot be validated. If you don't need to check the nonce, set OpenIdConnectProtocolValidator.RequireNonce to 'false'. Note if a 'nonce' is found it will be evaluated.
   at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator.ValidateNonce(OpenIdConnectProtocolValidationContext validationContext) in C:\agent2\_work\56\s\src\Microsoft.IdentityModel.Protocols.OpenIdConnect\OpenIdConnectProtocolValidator.cs:line 639
   at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolValidator.ValidateTokenResponse(OpenIdConnectProtocolValidationContext validationContext) in C:\agent2\_work\56\s\src\Microsoft.IdentityModel.Protocols.OpenIdConnect\OpenIdConnectProtocolValidator.cs:line 292
   at Microsoft.Owin.Security.OpenIdConnect.OpenIdConnectAuthenticationHandler.<AuthenticateCoreAsync>d__11.MoveNext()

ValidateTokenResponse is called here: https://github.com/aspnet/AspNetKatana/blob/main/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L529

While originally validation is done using Options.SecurityTokenValidator which doesn't care about nonce: https://github.com/aspnet/AspNetKatana/blob/main/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L811

The user stays logged in even after the error.

[andkorsh]

I can reproduce the same even on office.com actually, not sure what tech they are using though.

STR:

1. In Chrome Incognito go office.com
2. Click Log In
3. After being redirected to https://login.microsoftonline.com/common/oauth2/v2.0/authorize?xxxxx copy the URL and paste into another browser
4. Keep on logging in in another browser AR: Successfully logged in in another browser

Thus there's nothing that would verify the response is coming from the same client that initiated the original request.

In fact when using form_post response mode this might not be too much of an issue, as if an attacker had stolen the form data, they would most likely have acquired the nonce cookie as well. This would be a much more sensitive when response mode was query.

[andkorsh]

The same thing applies to the PKCE implementation I think. The code verifier should be stored in cookie, so that even if an attacker did steal the resulting query, they wouldn't be able to use it without having the code verifier stored in a cookie. In OWIN implementation the verifier is stored in parameters which are.... part of the query. Effectively making PKCE useless?

[Tratcher]

Auth validation is a multi-step process, OnSecurityTokenValidated is only part way through, so long as the nonce is validated by the end of the process that should be sufficient. The cookie isn't issued until the end, and OpenIdConnectProtocolInvalidNonceException should prevent the flow from reaching that code.

The user stays logged in even after the error.

What do you mean by that? They're still signed into the remote provider, but they shouldn't be signed into the local app.

[andkorsh]

I tend to disagree, the docs say: https://docs.microsoft.com/en-us/previous-versions/aspnet/mt180993(v=vs.113)

OpenIdConnectAuthenticationNotifications.SecurityTokenValidated Property
Invoked after the security token has passed validation and a ClaimsIdentity has been generated.

But the token cannot pass the validation without validating the nonce.

What do you mean by that? They're still signed into the remote provider, but they shouldn't be signed into the local app.

Check office.com example - they do set nonce and correlation cookies, but if you just delete them half way through or even go use another browser, nothing is going to change, you'll still be logged in to office.com

You can use Postman to GET (but disable the "follow redirect" feature): https://www.office.com/login?es=Click You'll get 302 to login.microsoftonline.com and a bunch of set-cookies (including nonce and correlation) Receive 302 and grab the Location header, open that url in any browser (preferably incognito) and keep on logging in You will get logged in but obviously you didn't have nonce cookies in that incognito session

[LeaFrock]

I'm using code flow and it seems like even though nonce cookie does get set, it's not really used and I successfully get authenticated no matter if I had the nonce cookie or not.

nonce is used for your client(in this case, it's your web backend) validation but not for identity provider. And you've already seen that ValidateTokenResponse at your client side will throw a RequireNonce exception. Without nonce, a user is only online for identity server, but is forbidden by your own server. So being authenticated without nonce is OK, and it seems to be by design.

[andkorsh]

@LeaFrock Yes and this is exactly how I implemented it, but my point is that raising SecurityTokenValidated and then AuthenticationFailed is confusing and the first one should've never been raised when there's no nonce present, the token cannot be verified without it and there should be no confusing event raised. Apparently it seems like even people in Microsoft confuse this (maybe creating auth ticket cookies in SecurityTokenValidated event) because you can clearly see in my example that office.com obviously doesn't validate nonce although it is being saved in cookie but is never used.

And also my other important point in this issue is that current PKCE implementation might not be the most secure - it would be better to store the token verifier in a cookie as well as nonce. Correct me if I'm wrong, but for client-server apps PKCE mostly makes sense for combination of ResponseMode=Query and ResponseType=Code. In this case Code is travelling unencrypted and therefore can be stolen and used by an attacker. Storing PKCE key verifier in this same query request does not make it any more secure, while putting it into the cookie would make sure it gets encrypted and is not part of the query string but rather transferred in the request headers which makes it way more harder to steal.

[Tratcher]

Yes there's confusion here and we've made that clarification to the docs for the aspnetcore version of this component: https://github.com/dotnet/aspnetcore/blob/c9af60637e42aa6bed8f9fd49b29001f220a5f99/src/Security/Authentication/OpenIdConnect/src/Events/OpenIdConnectEvents.cs#L54-L55

/// Invoked when an IdToken has been validated and produced an AuthenticationTicket. Note there are additional checks after this
/// event that validate other aspects of the authentication flow like the nonce.

I can't speak to Office's usage, you'd need to contact secure@microsoft.com to discuss that with them.

[will-bartlett]

The same thing applies to the PKCE implementation I think. The code verifier should be stored in cookie, so that even if an attacker did steal the resulting query, they wouldn't be able to use it without having the code verifier stored in a cookie. In OWIN implementation the verifier is stored in parameters which are.... part of the query. Effectively making PKCE useless?

This comment seems valid to me. As the BCP states in 4.2, one of the benefits of PKCE is that it protects from accidental leakage of state+code in the URI. If the PKCE verifier is embedded in state (rather than being embedded in cookie), this benefit is discarded.

[will-bartlett]

My general feedback regarding OAuth state and cookies is that the simplest and most secure implementation is as follows:

1. Make "state" a uuid
2. Have an encrypted cookie that contains a map of UUIDs => data.
3. On incoming request, lookup the state uuid in the cookie. If missing, fail. If present, use data.
4. Put nonce/PKCE in data

This sort of implementation protects against any sort of state leakage (it's just leaking random UUIDs), keeps the URIs short, etc. A secure implementation necessarily requires some linkage between cookie and state - sites can implement this as long-cookie+short-state or short-cookie+long-state, but generally long-cookie+short-state (e.g. with state as a UUID) is a little simpler.

[kevinchalet]

I read the entire discussion and I must admit I'm a bit confused by some of the statements.

Storing PKCE key verifier in this same query request does not make it any more secure, while putting it into the cookie would make sure it gets encrypted and is not part of the query string but rather transferred in the request headers which makes it way more harder to steal.

The state parameter is always encrypted and HMACed using Data Protection (on OWIN+IIS, it uses Machine Key, on OWIN+HttpListener, it uses DPAPI and on ASP.NET Core, it uses ASP.NET Core Data Protection) so it can only be read by the client application that created it (i.e the OWIN or ASP.NET Core app).

Since the whole state is encrypted, the code_verifier cannot be extracted (unless the attacker managed to exfiltrate the crypto keys from the host, but then you have much bigger problems 🤣) so the only attack vector is through the original client application: in this case, being able to intercept both the code and the matching state from the query string (containing the correct code_verifier) is not enough for the attack to succeed: you also need the matching correlation identifier (a 256-bit crypto-secure random ID) that is stored in the correlation cookie (it can be seen as a form of binding).

You could certainly swap the state parameter and the correlation cookie but it's not clear to me how this would improve things from a security perspective.

This sort of implementation protects against any sort of state leakage (it's just leaking random UUIDs), keeps the URIs short, etc.

I'm not sure how replacing the actual state payload by a UUID pointer would provide - by itself - a protection against leakage: it would work essentially like reference access tokens: the attacker doesn't have access to the actual token payload but if the reference ID leaks, it's game over.

Storing things in cookies - specially large values such as state payloads - comes with a major downside: it increases the chances of hitting cookie size limits (that can be as low as 4KB per domain on Safari). On ASP.NET Core, it's mostly just annoying as OIDC/OAuth 2.0 authorization flows can't be completed (or the user is simply not authenticated if one of the chunks of the authentication cookie is missing) but on Katana, it's terrible as the default cookie manager throws an exception if the cookie chunks can't be re-assembled: the user will keep getting an error until the cookies are manually flushed (unless you replace the default cookie manager and set ThrowForPartialCookies to false).

[andkorsh]

@kevinchalet the problem is not in code verifier being extracted and decrypted, but rather the whole response query URL containing State and Code (in case of Response Mode = Query) being obviously available to your ISP, firewall/access logs, etc, which can easily be intercepted and used by a hacker. Yes you can argue that one should never use Response Mode Query but it does exist unfortunately and PKCE is meant to make this mode secure and protect against such attacks. But this specific implementation of PKCE doesn't add any value.

[kevinchalet]

@kevinchalet the problem is not in code verifier being extracted and decrypted, but rather the whole response query URL containing State and Code (in case of Response Mode = Query) being obviously available to your ISP, firewall/access logs, etc, which can easily be intercepted and used by a hacker.

You're expected to use TLS so your ISP should never be able to access any of your query string parameters. Firewalls and applications can certainly log them, but since authorization codes are generally one-time tokens, it's not considered a viable attack vector in most cases.

which can easily be intercepted and used by a hacker

You need to be more specific: used how? It's one thing to pretend something is vulnerable to a class of attack, but you need to demonstrate it.

[andkorsh]

since authorization codes are generally one-time tokens, it's not considered a viable attack vector in most cases.

So you saying PKCE should've never existed then?

[kevinchalet]

So you saying PKCE should've never existed then?

I never said that 😕

PKCE has been originally invented to protect against URI scheme hijacking on mobile devices and has later been extended to all types of OAuth 2.0 clients to prevent an application from redeeming an authorization code if it's not the client that initially started the authorization process. It doesn't protect against replay attacks (which is the main concern with firewalls and applications logging the authorization code in the query string).

[Tratcher]

A secure implementation necessarily requires some linkage between cookie and state - sites can implement this as long-cookie+short-state or short-cookie+long-state, but generally long-cookie+short-state (e.g. with state as a UUID) is a little simpler.

We already have an implementation of short-cookie+long-state. The unique correlation cookie is verified against state field before any of the data in the state is consumed (including the PKCE value). A stolen state field cannot be re-used. Swapping the state and the cookie would not make this any more secure, it would only be a trade-off between cookie limits and url limits. Since url limits are more easily controlled/mitigated by the server we don't plan on changing how this works.

Thanks @kevinchalet!