diff --git a/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs b/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs index dc2a60613d..f48c3c8e54 100644 --- a/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs +++ b/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs @@ -102,6 +102,7 @@ protected override void AttachToOwinApp(IGalleryConfigurationService config, IAp PostLogoutRedirectUri = siteRoot, Scope = OpenIdConnectScope.OpenIdProfile + " email", ResponseType = OpenIdConnectResponseType.IdToken, + // CodeQL [SM03926] We do not restrict issuers to a limited set of tenants for our multi-tenant app TokenValidationParameters = new Microsoft.IdentityModel.Tokens.TokenValidationParameters() { ValidateIssuer = false }, Notifications = new OpenIdConnectAuthenticationNotifications { diff --git a/src/NuGetGallery/Controllers/AuthenticationController.cs b/src/NuGetGallery/Controllers/AuthenticationController.cs index 9bc921256a..3e6a0a13db 100644 --- a/src/NuGetGallery/Controllers/AuthenticationController.cs +++ b/src/NuGetGallery/Controllers/AuthenticationController.cs @@ -445,6 +445,12 @@ public virtual ActionResult AuthenticateExternal(string returnUrl) .SecurityPolicies .Any(policy => policy.Name == nameof(RequireOrganizationTenantPolicy))); + // Validate that the returnUrl is a relative URL to prevent untrusted URL redirection + if (!Url.IsLocalUrl(returnUrl)) + { + returnUrl = "/"; + } + if (userOrganizationsWithTenantPolicy != null && userOrganizationsWithTenantPolicy.Any()) { var aadCredential = user?.Credentials.GetAzureActiveDirectoryCredential(); diff --git a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs index caec0e7ba0..fc0b0ee28e 100644 --- a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs +++ b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs @@ -49,6 +49,7 @@ private async Task RecaptchaIsValid(string privateKey, string response) try { + // CodeQL [SM03781] The validation Url is restricted to a specific host, which mitigates the risk of unwanted redirection var reply = await Client.Value.GetStringAsync(validationUrl); var state = JsonConvert.DeserializeObject(reply); return state.Success; diff --git a/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs index bfd58b850e..1a36497dc1 100644 --- a/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs @@ -1576,23 +1576,27 @@ public async Task GivenEnableMultiFactorAuthenticationInUserIsFalseAndLoginUsedM public class TheAuthenticateExternalAction : TestContainer { - [Fact] - public void ForMissingExternalProvider_ErrorIsReturned() + [Theory] + [InlineData("/theReturnUrl")] + [InlineData("/")] + public void ForMissingExternalProvider_ErrorIsReturned(string returnUrl) { // Arrange var controller = GetController(); controller.SetCurrentUser(TestUtility.FakeUser); // Act - var result = controller.AuthenticateExternal("theReturnUrl"); + var result = controller.AuthenticateExternal(returnUrl); // Assert - ResultAssert.IsRedirectTo(result, "theReturnUrl"); + ResultAssert.IsRedirectTo(result, returnUrl); Assert.Equal(Strings.ChangeCredential_ProviderNotFound, controller.TempData["Message"]); } - [Fact] - public void ForAADLinkedAccount_ErrorIsReturnedDueToOrgPolicy() + [Theory] + [InlineData("/theReturnUrl")] + [InlineData("/")] + public void ForAADLinkedAccount_ErrorIsReturnedDueToOrgPolicy(string returnUrl) { // Arrange var fakes = Get(); @@ -1612,15 +1616,17 @@ public void ForAADLinkedAccount_ErrorIsReturnedDueToOrgPolicy() controller.SetCurrentUser(user); // Act - var result = controller.AuthenticateExternal("theReturnUrl"); + var result = controller.AuthenticateExternal(returnUrl); // Assert - ResultAssert.IsRedirectTo(result, "theReturnUrl"); + ResultAssert.IsRedirectTo(result, returnUrl); Assert.NotNull(controller.TempData["WarningMessage"]); } - [Fact] - public void ForNonAADLinkedAccount_WithOrgPolicyCompletesSuccessfully() + [Theory] + [InlineData("/theReturnUrl")] + [InlineData("/")] + public void ForNonAADLinkedAccount_WithOrgPolicyCompletesSuccessfully(string returnUrl) { // Arrange var fakes = Get(); @@ -1640,11 +1646,11 @@ public void ForNonAADLinkedAccount_WithOrgPolicyCompletesSuccessfully() controller.SetCurrentUser(user); // Act - var result = controller.AuthenticateExternal("theReturnUrl"); + var result = controller.AuthenticateExternal(returnUrl); // Assert Assert.Null(controller.TempData["WarningMessage"]); - ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential("theReturnUrl")); + ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential(returnUrl)); } [Theory] @@ -1685,6 +1691,37 @@ public void WillCallChallengeAuthenticationForAADv2Provider() // Assert ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential(returnUrl)); } + + [Fact] + public void WillRedirectToHomePageOnFailureForAbsoluteUrls() + { + // Arrange + var controller = GetController(); + controller.SetCurrentUser(TestUtility.FakeUser); + + // Act + var result = controller.AuthenticateExternal("theReturnUrl"); // not a relative URL + + // Assert + ResultAssert.IsRedirectTo(result, "/"); + Assert.Equal(Strings.ChangeCredential_ProviderNotFound, controller.TempData["Message"]); + } + + [Fact] + public void WillRedirectToHomePageOnSuccessForAbsoluteUrls() + { + // Arrange + const string returnUrl = "theReturnUrl"; // not a relative URL + EnableAllAuthenticators(Get()); + var controller = GetController(); + controller.SetCurrentUser(TestUtility.FakeUser); + + // Act + var result = controller.AuthenticateExternal(returnUrl); + + // Assert + ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential("/")); + } } public class TheLinkExternalAccountAction : TestContainer