From c9f3378e90e953ff0dabca8f32255684bee56227 Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Wed, 8 Feb 2023 16:15:42 -0800 Subject: [PATCH 1/7] added Url redirect codeql suppressions --- src/NuGetGallery/Controllers/AuthenticationController.cs | 2 ++ src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/NuGetGallery/Controllers/AuthenticationController.cs b/src/NuGetGallery/Controllers/AuthenticationController.cs index 9bc921256a..672b181ddd 100644 --- a/src/NuGetGallery/Controllers/AuthenticationController.cs +++ b/src/NuGetGallery/Controllers/AuthenticationController.cs @@ -454,6 +454,7 @@ public virtual ActionResult AuthenticateExternal(string returnUrl) userOrganizationsWithTenantPolicy.Select(member => member.Organization.Username)); TempData["WarningMessage"] = string.Format(Strings.ChangeCredential_NotAllowed, orgList); + // CodeQL [SM00405] The return Url is not a user-provided value, and is a relative Url generated by us return Redirect(returnUrl); } } @@ -462,6 +463,7 @@ public virtual ActionResult AuthenticateExternal(string returnUrl) if (externalAuthProvider == null) { TempData["Message"] = Strings.ChangeCredential_ProviderNotFound; + // CodeQL [SM00405] The return Url is not a user-provided value, and is a relative Url generated by us return Redirect(returnUrl); } diff --git a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs index caec0e7ba0..2f94a17076 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 [SM00405] The validation Url does not include any user-provided values var reply = await Client.Value.GetStringAsync(validationUrl); var state = JsonConvert.DeserializeObject(reply); return state.Success; From 301f451a09f4f28f41909899b3713ff54434d689 Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Wed, 22 Feb 2023 14:04:44 -0800 Subject: [PATCH 2/7] codeQL bug = suppression :) --- .../AzureActiveDirectoryV2Authenticator.cs | 1 + src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs b/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs index dc2a60613d..3c5ecdc44b 100644 --- a/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs +++ b/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs @@ -96,6 +96,7 @@ protected override void AttachToOwinApp(IGalleryConfigurationService config, IAp } // Configure OpenIdConnect + // CodeQL [SM03926] We do not restrict issuers to a limited set of tenants for our multi-tenant app var options = new OpenIdConnectAuthenticationOptions(BaseConfig.AuthenticationType) { RedirectUri = siteRoot + _callbackPath, diff --git a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs index 2f94a17076..8aa05c8908 100644 --- a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs +++ b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs @@ -49,7 +49,7 @@ private async Task RecaptchaIsValid(string privateKey, string response) try { - // CodeQL [SM00405] The validation Url does not include any user-provided values + // CodeQL [SM03781] The validation Url does not include any user-provided values var reply = await Client.Value.GetStringAsync(validationUrl); var state = JsonConvert.DeserializeObject(reply); return state.Success; From f822a7e7acb7532424b728ed2da4e16dd5ead3ef Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Wed, 22 Feb 2023 14:09:50 -0800 Subject: [PATCH 3/7] moved suppression comment closer to the relevant line of code --- .../AzureActiveDirectoryV2Authenticator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs b/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs index 3c5ecdc44b..f48c3c8e54 100644 --- a/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs +++ b/src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs @@ -96,13 +96,13 @@ protected override void AttachToOwinApp(IGalleryConfigurationService config, IAp } // Configure OpenIdConnect - // CodeQL [SM03926] We do not restrict issuers to a limited set of tenants for our multi-tenant app var options = new OpenIdConnectAuthenticationOptions(BaseConfig.AuthenticationType) { RedirectUri = siteRoot + _callbackPath, 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 { From e0d8a26a5bea71ea622b1bd23e8992cb747ef51e Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Thu, 23 Feb 2023 15:16:39 -0800 Subject: [PATCH 4/7] We are restricting the reCAPTCHA request to a specific host. Changed the suppression comment to reflect this as the mitigation for the vulnerability. --- src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs index 8aa05c8908..78af51788c 100644 --- a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs +++ b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs @@ -49,7 +49,7 @@ private async Task RecaptchaIsValid(string privateKey, string response) try { - // CodeQL [SM03781] The validation Url does not include any user-provided values + // CodeQL [SM03781] The validation Url is restricted to a specific host, and the 'response' parameter is generated by client-side reCAPTCHA code var reply = await Client.Value.GetStringAsync(validationUrl); var state = JsonConvert.DeserializeObject(reply); return state.Success; From 32491b6261dabaf5acf197a56c9383405db98bcf Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Mon, 27 Feb 2023 18:12:59 -0800 Subject: [PATCH 5/7] Only allow relative Urls as a return Url. Add/adapt tests. --- .../Controllers/AuthenticationController.cs | 10 ++- .../AuthenticationControllerFacts.cs | 61 +++++++++++++++---- 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/NuGetGallery/Controllers/AuthenticationController.cs b/src/NuGetGallery/Controllers/AuthenticationController.cs index 672b181ddd..53886f1961 100644 --- a/src/NuGetGallery/Controllers/AuthenticationController.cs +++ b/src/NuGetGallery/Controllers/AuthenticationController.cs @@ -9,9 +9,11 @@ using System.Net; using System.Net.Mail; using System.Security.Claims; +using System.Security.Policy; using System.Threading.Tasks; using System.Web; using System.Web.Mvc; +using System.Web.WebPages; using NuGet.Services.Entities; using NuGet.Services.Messaging.Email; using NuGetGallery.Authentication; @@ -445,6 +447,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(); @@ -454,7 +462,6 @@ public virtual ActionResult AuthenticateExternal(string returnUrl) userOrganizationsWithTenantPolicy.Select(member => member.Organization.Username)); TempData["WarningMessage"] = string.Format(Strings.ChangeCredential_NotAllowed, orgList); - // CodeQL [SM00405] The return Url is not a user-provided value, and is a relative Url generated by us return Redirect(returnUrl); } } @@ -463,7 +470,6 @@ public virtual ActionResult AuthenticateExternal(string returnUrl) if (externalAuthProvider == null) { TempData["Message"] = Strings.ChangeCredential_ProviderNotFound; - // CodeQL [SM00405] The return Url is not a user-provided value, and is a relative Url generated by us return Redirect(returnUrl); } 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 From c0ed1e13a640f8b1554d4e4987ee2d1f18f0dfa1 Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Tue, 28 Feb 2023 13:49:04 -0800 Subject: [PATCH 6/7] removed unnecessary usings --- src/NuGetGallery/Controllers/AuthenticationController.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/NuGetGallery/Controllers/AuthenticationController.cs b/src/NuGetGallery/Controllers/AuthenticationController.cs index 53886f1961..3e6a0a13db 100644 --- a/src/NuGetGallery/Controllers/AuthenticationController.cs +++ b/src/NuGetGallery/Controllers/AuthenticationController.cs @@ -9,11 +9,9 @@ using System.Net; using System.Net.Mail; using System.Security.Claims; -using System.Security.Policy; using System.Threading.Tasks; using System.Web; using System.Web.Mvc; -using System.Web.WebPages; using NuGet.Services.Entities; using NuGet.Services.Messaging.Email; using NuGetGallery.Authentication; From fc25f2ce86faa7167aa1db62d20f8c4bb906fd32 Mon Sep 17 00:00:00 2001 From: Advay Tandon Date: Thu, 2 Mar 2023 13:53:00 -0800 Subject: [PATCH 7/7] modified suppression comment for recaptcha redirection --- src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs index 78af51788c..fc0b0ee28e 100644 --- a/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs +++ b/src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs @@ -49,7 +49,7 @@ private async Task RecaptchaIsValid(string privateKey, string response) try { - // CodeQL [SM03781] The validation Url is restricted to a specific host, and the 'response' parameter is generated by client-side reCAPTCHA code + // 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;