app crashes when returning a ForbidResult with OpenIdConnect as your DefaultChallengeScheme

[mkhalife]

I'm trying to get one of my .net core apps to work with Azure AD using OpenIdConnect. I'm experiencing a weird problem whenever a user tries to access a resource they don't have permission to (i.e. they violate an Authorization Policy). After hours of messing with it I finally have some steps below to easily reproduce the issue.

1. use the project from here: https://github.com/Azure-Samples/active-directory-dotnet-webapp-openidconnect-aspnetcore/tree/aspnet_core_2_0

2. open the HomeController and just add a simple method that will return a ForbidResult()

public IActionResult Test()
{
    return new ForbidResult();
}

3. start the app and navigate to https://localhost:44353/Home/Test

the app crashes and doesn't provide much exception information. this is the last thing shown in the "ASP.NET Core Web Server" output:

WebApp-OpenIDConnect-DotNet>       Executing action method WebApp_OpenIDConnect_DotNet.Controllers.HomeController.Test (WebApp-OpenIDConnect-DotNet) with arguments ((null)) - ModelState is Valid
WebApp-OpenIDConnect-DotNet> info: Microsoft.AspNetCore.Mvc.ForbidResult[1]
WebApp-OpenIDConnect-DotNet>       Executing ForbidResult with authentication schemes ().
WebApp-OpenIDConnect-DotNet> 
WebApp-OpenIDConnect-DotNet> Process is terminating due to StackOverflowException.

this issue won't happen if you simply change the DefaultChallengeScheme in the Startup.cs to CookieAuthenticationDefaults.AuthenticationScheme

i'm using the 2.0 sdk and here is my OS information in case it's needed. I've uninstalled all other versions of the sdk but the problem persists.

[jkotalik]

Hello @mkhalife . I am trying to reproduce your scenario. Just to confirm, was there a logged on user before navigating to https://localhost:44353/Home/Test or were you able to simply clone the repo and try navigating to Home/Test?

My guess is there is probably an infinite redirect going on somewhere.

[davidfowl]

This is a bug. It seems as though the RemoteAuthenticationHandler.HandleForbidAsync calls ForbidAsync(SignInScheme) which should call Forbid on the CookieHandler but we have another bug aspnet/HttpAbstractions#917. So instead it ends up stack overflowing because it's calling itself.

Security/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs

Lines 174 to 176 in 23da476

	protected override Task HandleForbiddenAsync(AuthenticationProperties properties)
	=> Context.ForbidAsync(SignInScheme);

/cc @HaoK

/cc @Eilon this looks like a 2.0.1 candidate.

[mkhalife]

thanks for the quick responses!

@jkotalik the user does not need to be logged in to reproduce the issue.

@davidfowl thanks for confirming the bug and opening that other issue. Any recommendations/workarounds in the mean time?

[davidfowl]

@davidfowl thanks for confirming the bug and opening that other issue. Any recommendations/workarounds in the mean time?

How did you run into it in the first place? The workaround may be easy or hard depending on that.

[kevinchalet]

You guys can't say you had not been warned about this eventuality...

If the handler overrides HandleSignInAsync() and calls SignInAsync(SignInScheme), it will result in an endless recursive call and eventually, a stack overflow exception.

#1264 (comment)

[mkhalife]

@davidfowl I was adding Policy-based Authorize attributes to my controllers e.g. [Authorize(Policy = "SuperUser")]. I want unauthenticated users to get a 401 (which works) and authenticated users who don't have certain claims to get a 403. As soon as I logged in as someone who didn't have the required claim, I encountered this.

[davidfowl]

@mkhalife as a workaround, specify the SignInScheme explicitly on AddOpenIdConnect call via the options:

// Add Authentication services.
services.AddAuthentication(sharedOptions =>
{
    sharedOptions.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
    sharedOptions.DefaultChallengeScheme = OpenIdConnectDefaults.AuthenticationScheme;
})
// Configure the pipeline to use cookie auth.
.AddCookie()
// Configure the pipeline to use OpenID Connect auth.
.AddOpenIdConnect(option =>
{
    option.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;
    option.ClientId = Configuration["AzureAD:ClientId"];
    option.Authority = String.Format(Configuration["AzureAd:AadInstance"], Configuration["AzureAd:Tenant"]);
    option.SignedOutRedirectUri = Configuration["AzureAd:PostLogoutRedirectUri"];
    option.Events = new OpenIdConnectEvents
    {
        OnRemoteFailure = OnAuthenticationFailed,
    };
});

[kevinchalet]

The real bug is here: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/AuthenticationServiceCollectionExtensions.cs#L100

SignInScheme defaults to DefaultSignInScheme but if you leave it to null, ForbidAsync() is called with a null parameter, which asks the authentication service to use the default forbid scheme.

The initializer should throw an exception if the sign-in scheme can't be inferred (i.e if SignInScheme is still null).

[davidfowl]

SignInScheme defaults to DefaultSignInScheme but if you leave it to null, ForbidAsync() is called with a null parameter, which asks the authentication service to use the default forbid scheme.

Which in theory should be "Cookies" not "OpendIdConnect".

[kevinchalet]

Which in theory should be "Cookies" not "OpendIdConnect".

Not here. When it's null, DefaultForbidScheme defaults to DefaultChallengeScheme, which explicitly points to OIDC in the sample mentioned by @mkhalife (most likely because the sample writer wanted to immediately redirect the users to the IdP instead of redirecting them to an intermediate login page).

[davidfowl]

Not here. When it's null, DefaultForbidScheme defaults to DefaultChallengeScheme, which explicitly points to OIDC in the sample mentioned by @mkhalife (most likely because the sample writer wanted to immediately redirect the users to the IdP instead of redirecting them to an intermediate login page).

That's what I called out here #1376 (comment).

I'm not sure DefaultForbidScheme should default to DefaultChallengeScheme over DefaultScheme. I think I understand why we did that though. I'm just questioning it.

[kevinchalet]

Making DefaultForbidScheme default to DefaultChallengeScheme is fine (changing that would be a breaking change anyway).

What's totally wrong is allowing RemoteAuthenticationOptions.SignInScheme to be eventually null. It should default to DefaultSignInScheme and nothing else.

[kevinchalet]

What's totally wrong is allowing RemoteAuthenticationOptions.SignInScheme being eventually null. It should default to DefaultSignInScheme and nothing else.

1.x had the right checks but for some reasons, they've been removed in 2.0: https://github.com/aspnet/Security/blob/rel/1.1.2/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectMiddleware.cs#L95-L102

[mkhalife]

@davidfowl thanks so much for the workaround...all is working fine now.

[HaoK]

@davidfowl The reason we decided to default forbid to challenge, is because they were a single concept in 1.x, we only split them apart in 2.0, so we thought this would be more natural...

I would have expected there to be validation in RemoteAuthenticationOptions.Validate for SignInScheme to be non null, but I think we might have removed it to enable using the default schemes (which is what null does)... I filed #1378 to follow up on that though

[davidfowl]

Got it

[zetanet]

Same problem here!.
davidfowl workaround "solve" the problem.

Thanks.

[HaoK]

The fix for this is being tracked via #1378 and aspnet/HttpAbstractions#917