diff --git a/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/RemoteAuthenticationCsrfTests.cs b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/RemoteAuthenticationCsrfTests.cs new file mode 100644 index 000000000000..5899c75d7b37 --- /dev/null +++ b/src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/RemoteAuthenticationCsrfTests.cs @@ -0,0 +1,167 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#nullable enable + +using System.Net; +using System.Net.Http; +using System.Text.Encodings.Web; +using Microsoft.AspNetCore.Antiforgery; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; + +namespace Microsoft.AspNetCore.Tests; + +// A remote provider's callback (e.g. OpenID Connect response_mode=form_post) is a cross-site form POST by +// protocol design. When the callback path also matches a routed endpoint that requires antiforgery +// validation, the auto-injected CSRF middleware records an invalid verdict for it, and the handler used to +// fail while reading its own callback body - before any of its events could run. +public class RemoteAuthenticationCsrfTests +{ + [Fact] + public async Task RemoteCallback_CrossSiteFormPost_CanReadCallbackForm() + { + using var app = await CreateAppWithTokenAntiforgery(); + + var response = await app.GetTestClient().SendAsync(CreateCallbackRequest()); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("handled:2", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task RemoteCallback_WhenHandlerSkipsRequest_WithTokenAntiforgery_ProtectsDownstreamEndpoint() + { + using var app = await CreateAppWithTokenAntiforgery(skipRequest: true); + + var response = await app.GetTestClient().SendAsync(CreateCallbackRequest()); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Equal("protected", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task RemoteCallback_WhenHandlerSkipsRequest_RestoresAutoCsrfVerdict() + { + using var app = await CreateAppWithAutoCsrfOnly(skipRequest: true); + + var response = await app.GetTestClient().SendAsync(CreateCallbackRequest()); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Equal("protected", await response.Content.ReadAsStringAsync()); + } + + private static HttpRequestMessage CreateCallbackRequest() + { + var request = new HttpRequestMessage(HttpMethod.Post, "/signin-oidc") + { + Content = new FormUrlEncodedContent(new Dictionary + { + ["state"] = "fakestate", + ["code"] = "fakecode", + }) + }; + request.Headers.Add("Sec-Fetch-Site", "cross-site"); + return request; + } + + private static Task CreateAppWithTokenAntiforgery(bool skipRequest = false) + => CreateApp(skipRequest, useTokenAntiforgery: true); + + private static Task CreateAppWithAutoCsrfOnly(bool skipRequest = false) + => CreateApp(skipRequest, useTokenAntiforgery: false); + + private static async Task CreateApp(bool skipRequest, bool useTokenAntiforgery) + { + var builder = WebApplication.CreateBuilder(); + builder.WebHost.UseTestServer(); + builder.Services.AddAuthentication("signin") + .AddScheme("signin", _ => { }) + .AddScheme("remote", o => + { + o.CallbackPath = "/signin-oidc"; + o.SignInScheme = "signin"; + o.SkipRequest = skipRequest; + }); + builder.Services.AddAuthorization(); + if (useTokenAntiforgery) + { + builder.Services.AddAntiforgery(); + } + + var app = builder.Build(); + app.UseAuthentication(); + app.UseAuthorization(); + if (useTokenAntiforgery) + { + app.UseAntiforgery(); + } + + // Stands in for a catch-all server-rendered page: it makes routing match the remote callback path, + // which is what causes the CSRF middleware to record a verdict for the callback request. + app.MapPost("/{**slug}", EnforceCsrf).WithMetadata(new RequiresValidationMetadata()); + + await app.StartAsync(); + return app; + } + + private static string EnforceCsrf(HttpContext context) + { + var feature = context.Features.Get(); + if (feature is null) + { + return "passthrough"; + } + + if (!feature.IsValid) + { + context.Response.StatusCode = StatusCodes.Status400BadRequest; + return "protected"; + } + + return "allowed"; + } + + private sealed class RequiresValidationMetadata : IAntiforgeryMetadata + { + public bool RequiresValidation => true; + } + + private sealed class NoOpHandler(IOptionsMonitor options, ILoggerFactory logger, UrlEncoder encoder) + : AuthenticationHandler(options, logger, encoder) + { + protected override Task HandleAuthenticateAsync() + => Task.FromResult(AuthenticateResult.NoResult()); + } + + private sealed class FakeRemoteOptions : RemoteAuthenticationOptions + { + public bool SkipRequest { get; set; } + } + + private sealed class FakeRemoteHandler(IOptionsMonitor options, ILoggerFactory logger, UrlEncoder encoder) + : RemoteAuthenticationHandler(options, logger, encoder) + { + protected override async Task HandleRemoteAuthenticateAsync() + { + // Mirrors OpenIdConnectHandler.HandleRemoteAuthenticateAsync: the form_post callback body is read + // before the handler raises any of its events. + var form = await Request.ReadFormAsync(Context.RequestAborted); + + if (Options.SkipRequest) + { + return HandleRequestResult.SkipHandler(); + } + + Response.StatusCode = StatusCodes.Status200OK; + await Response.WriteAsync($"handled:{form.Count}"); + return HandleRequestResult.Handle(); + } + } +} diff --git a/src/Security/Authentication/Core/src/Microsoft.AspNetCore.Authentication.csproj b/src/Security/Authentication/Core/src/Microsoft.AspNetCore.Authentication.csproj index 0a9f4a34f0dd..65c808aeb083 100644 --- a/src/Security/Authentication/Core/src/Microsoft.AspNetCore.Authentication.csproj +++ b/src/Security/Authentication/Core/src/Microsoft.AspNetCore.Authentication.csproj @@ -12,6 +12,7 @@ + diff --git a/src/Security/Authentication/Core/src/RemoteAuthenticationHandler.cs b/src/Security/Authentication/Core/src/RemoteAuthenticationHandler.cs index 203f17740063..0119ad1493b1 100644 --- a/src/Security/Authentication/Core/src/RemoteAuthenticationHandler.cs +++ b/src/Security/Authentication/Core/src/RemoteAuthenticationHandler.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Security.Cryptography; using System.Text.Encodings.Web; +using Microsoft.AspNetCore.Antiforgery; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -79,6 +80,11 @@ public virtual async Task HandleRequestAsync() return false; } + return await RemoteAuthenticationAntiforgery.HandleWithoutAntiforgeryVerdictAsync(Context, HandleRequestCoreAsync); + } + + private async Task HandleRequestCoreAsync() + { AuthenticationTicket? ticket = null; Exception? exception = null; AuthenticationProperties? properties = null; diff --git a/src/Security/Authentication/OpenIdConnect/src/Microsoft.AspNetCore.Authentication.OpenIdConnect.csproj b/src/Security/Authentication/OpenIdConnect/src/Microsoft.AspNetCore.Authentication.OpenIdConnect.csproj index 08d735332136..8de8159a9a01 100644 --- a/src/Security/Authentication/OpenIdConnect/src/Microsoft.AspNetCore.Authentication.OpenIdConnect.csproj +++ b/src/Security/Authentication/OpenIdConnect/src/Microsoft.AspNetCore.Authentication.OpenIdConnect.csproj @@ -19,6 +19,7 @@ + diff --git a/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs b/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs index 525f18a1050e..6e64f3d3aebb 100644 --- a/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs +++ b/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs @@ -11,6 +11,7 @@ using System.Text; using System.Text.Encodings.Web; using System.Text.Json; +using Microsoft.AspNetCore.Antiforgery; using Microsoft.AspNetCore.Authentication.OAuth; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.WebUtilities; @@ -88,13 +89,16 @@ public OpenIdConnectHandler(IOptionsMonitor options, ILogg /// public override Task HandleRequestAsync() { + // Both paths below are, like the sign-in callback, cross-site requests owned by this handler, and + // HandleRemoteSignOutAsync reads a form_post body, so they need the same antiforgery verdict handling + // that RemoteAuthenticationHandler applies to CallbackPath. if (Options.RemoteSignOutPath.HasValue && Options.RemoteSignOutPath == Request.Path) { - return HandleRemoteSignOutAsync(); + return RemoteAuthenticationAntiforgery.HandleWithoutAntiforgeryVerdictAsync(Context, HandleRemoteSignOutAsync); } else if (Options.SignedOutCallbackPath.HasValue && Options.SignedOutCallbackPath == Request.Path) { - return HandleSignOutCallbackAsync(); + return RemoteAuthenticationAntiforgery.HandleWithoutAntiforgeryVerdictAsync(Context, HandleSignOutCallbackAsync); } return base.HandleRequestAsync(); diff --git a/src/Shared/RemoteAuthenticationAntiforgery.cs b/src/Shared/RemoteAuthenticationAntiforgery.cs new file mode 100644 index 000000000000..be291fa09d65 --- /dev/null +++ b/src/Shared/RemoteAuthenticationAntiforgery.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Http; + +namespace Microsoft.AspNetCore.Antiforgery; + +// Shared between Microsoft.AspNetCore.Authentication (RemoteAuthenticationHandler) and the remote handlers +// that own additional callback paths of their own (e.g. Microsoft.AspNetCore.Authentication.OpenIdConnect). +// +// A remote provider's callback is a cross-site request by protocol design: OpenID Connect +// response_mode=form_post and WS-Federation both deliver the response as a top-level form POST from the +// identity provider's origin. Cross-origin CSRF protection therefore records an invalid +// IAntiforgeryValidationFeature verdict for it, and the handler then fails on its very first action - reading +// the callback body - before any of its events can run, so an application has no way to opt out. +// +// These callbacks carry their own forgery protection: the state parameter round-trips a protected +// AuthenticationProperties payload whose correlation id must match the correlation cookie, which the handler +// validates. The verdict is therefore suppressed while the handler owns the request, and restored when the +// handler declines it so the rest of the pipeline still sees the original verdict. +internal static class RemoteAuthenticationAntiforgery +{ + public static async Task HandleWithoutAntiforgeryVerdictAsync(HttpContext context, Func> handler) + { + var suppressedVerdict = context.Features.Get(); + if (suppressedVerdict is { IsValid: false }) + { + context.Features.Set(null); + } + + var handled = false; + try + { + handled = await handler(); + return handled; + } + finally + { + if (!handled && suppressedVerdict is { IsValid: false }) + { + context.Features.Set(suppressedVerdict); + } + } + } +}