Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 2 additions & 58 deletions src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Globalization;
using System.Text;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Internal;
using Microsoft.AspNetCore.Mvc.Core;
using Microsoft.AspNetCore.Routing;

Expand Down Expand Up @@ -313,64 +314,7 @@ internal static void NormalizeRouteValuesForPage(
}

internal static bool CheckIsLocalUrl([NotNullWhen(true)] string? url)
{
if (string.IsNullOrEmpty(url))
{
return false;
}

// Allows "/" or "/foo" but not "//" or "/\".
if (url[0] == '/')
{
// url is exactly "/"
if (url.Length == 1)
{
return true;
}

// url doesn't start with "//" or "/\"
if (url[1] != '/' && url[1] != '\\')
{
return !HasControlCharacter(url.AsSpan(1));
}

return false;
}

// Allows "~/" or "~/foo" but not "~//" or "~/\".
if (url[0] == '~' && url.Length > 1 && url[1] == '/')
{
// url is exactly "~/"
if (url.Length == 2)
{
return true;
}

// url doesn't start with "~//" or "~/\"
if (url[2] != '/' && url[2] != '\\')
{
return !HasControlCharacter(url.AsSpan(2));
}

return false;
}

return false;

static bool HasControlCharacter(ReadOnlySpan<char> readOnlySpan)
{
// URLs may not contain ASCII control characters.
for (var i = 0; i < readOnlySpan.Length; i++)
{
if (char.IsControl(readOnlySpan[i]))
{
return true;
}
}

return false;
}
}
=> SharedUrlHelper.IsLocalUrl(url);
Comment thread
rokonec marked this conversation as resolved.

private static object CalculatePageName(ActionContext? context, RouteValueDictionary? ambientValues, string pageName)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Text.Encodings.Web;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Internal;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

Expand Down Expand Up @@ -446,17 +447,9 @@ await Events.RedirectToReturnUrl(
}

private static bool IsHostRelative(string path)
{
if (string.IsNullOrEmpty(path))
{
return false;
}
if (path.Length == 1)
{
return path[0] == '/';
}
return path[0] == '/' && path[1] != '/' && path[1] != '\\';
}
=> !string.IsNullOrEmpty(path)
&& path[0] == '/' // Suppresses the "~/..." branch of SharedUrlHelper.IsLocalUrl so only true host-relative paths are accepted.
&& SharedUrlHelper.IsLocalUrl(path);
Comment thread
rokonec marked this conversation as resolved.

/// <inheritdoc />
protected override async Task HandleForbiddenAsync(AuthenticationProperties properties)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

<ItemGroup>
<Compile Include="$(SharedSourceRoot)ChunkingCookieManager\**\*.cs" />
<Compile Include="$(SharedSourceRoot)ResultsHelpers\SharedUrlHelper.cs" LinkBase="Shared" />
</ItemGroup>

<ItemGroup>
Expand Down
79 changes: 79 additions & 0 deletions src/Security/Authentication/test/CookieTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,85 @@ public async Task AjaxLogoutRedirectToReturnUrlTurnsInto200WithLocationHeader()
Assert.StartsWith("/", responded.Single());
}

[Theory]
[InlineData("/\tfoo")]
[InlineData("/\rfoo")]
[InlineData("/\nfoo")]
[InlineData("/\r\nfoo")]
[InlineData("/\0foo")]
[InlineData("/\u0001foo")]
[InlineData("/\u001Ffoo")]
[InlineData("/\u007Ffoo")]
[InlineData("/foo\r\nLocation:%20evil")]
[InlineData("/foo\tbar")]
public async Task LogoutReturnUrlWithControlCharacterIsRejected(string returnUrl)
{
using var host = await CreateHost(o => o.LogoutPath = "/signout");
using var server = host.GetTestServer();
var transaction = await SendAsync(
server,
"http://example.com/signout?ReturnUrl=" + Uri.EscapeDataString(returnUrl));

Assert.False(transaction.Response.Headers.Contains("Location"));
}

[Theory]
[InlineData("/")]
[InlineData("/foo")]
[InlineData("/foo/bar")]
[InlineData("/foo?x=1#y")]
public async Task LogoutReturnUrlWithoutControlCharacterIsHonored(string returnUrl)
{
using var host = await CreateHost(o => o.LogoutPath = "/signout");
using var server = host.GetTestServer();
var transaction = await SendAsync(
server,
"http://example.com/signout?ReturnUrl=" + Uri.EscapeDataString(returnUrl));

var location = Assert.Single(transaction.Response.Headers.GetValues("Location"));
Assert.Equal(returnUrl, location);
}

[Theory]
// Protocol-relative URLs (path[1] == '/').
[InlineData("//www.example.com")]
[InlineData("//www.example.com/foobar.html")]
[InlineData("///www.example.com")]
[InlineData("//////www.example.com")]
// Forward-then-backslash (path[1] == '\\').
[InlineData("/\\")]
[InlineData("/\\foo")]
[InlineData("/\\evil.com")]
// Application-relative (~) is rejected — cookie auth never resolves PathBase like MVC's IUrlHelper.Content does.
[InlineData("~/")]
[InlineData("~/foo")]
[InlineData("~/foo.html")]
// Absolute URLs (path[0] != '/').
[InlineData("http://www.example.com")]
[InlineData("https://www.example.com")]
[InlineData("HtTpS://www.example.com")]
[InlineData("http://localhost/foobar.html")]
[InlineData("javascript:alert(1)")]
// Relative URLs (path[0] != '/').
[InlineData("foo.html")]
[InlineData("../foo.html")]
[InlineData("fold/foo.html")]
// Single-slash scheme confusion.
[InlineData("http:/foo.html")]
[InlineData("hTtP:foo.html")]
// Whitespace-only (length 1, path[0] != '/').
[InlineData(" ")]
public async Task LogoutReturnUrlNonHostRelativeIsRejected(string returnUrl)
{
using var host = await CreateHost(o => o.LogoutPath = "/signout");
using var server = host.GetTestServer();
var transaction = await SendAsync(
server,
"http://example.com/signout?ReturnUrl=" + Uri.EscapeDataString(returnUrl));

Assert.False(transaction.Response.Headers.Contains("Location"));
}

[Fact]
public async Task AjaxChallengeRedirectTurnsInto200WithLocationHeader()
{
Expand Down
4 changes: 4 additions & 0 deletions src/Shared/ResultsHelpers/SharedUrlHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ internal static class SharedUrlHelper
return contentPath;
}

// SECURITY: This is the open-redirect guard used by MVC URL helpers, Results.LocalRedirect,
// and CookieAuthenticationHandler. Changes to the accepted/rejected shapes (control characters,
// "//", "/\", "~/", etc.) must be reviewed against all call sites and the existing test corpus.
// Do not relax any check without security review.
internal static bool IsLocalUrl([NotNullWhen(true)] string? url)
{
if (string.IsNullOrEmpty(url))
Expand Down
Loading