Merge Security Patches. Closes #9 and #10.

alexhiggins732 · Feb 11, 2024 · 7736879 · 7736879
2 parents 7b03508 + 5f7c2b9
commit 7736879
Show file tree

Hide file tree

Showing 32 changed files with 842 additions and 104 deletions.
diff --git a/Directory.Packages.props b/Directory.Packages.props
@@ -11,7 +11,7 @@
     <GrpcVersion>2.59.0</GrpcVersion>
     <SerilogVersion>8.0.0</SerilogVersion>
     <NoWarn>$(NoWarn);AD0001;ASP0003;ASP0004;ASP0005;ASP0007;ASP0020;ASP0021;ASP0022;ASP0024</NoWarn>
-		<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
+    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
   </PropertyGroup>
   <ItemGroup>
     <PackageVersion Include="AspNet.Security.OAuth.GitHub" Version="$(AspnetVersion)" />
@@ -86,8 +86,8 @@
     <PackageVersion Include="Microsoft.Extensions.Configuration.FileExtensions" Version="$(AspnetVersion)" />
     <PackageVersion Include="Microsoft.Extensions.Configuration.Json" Version="$(AspnetVersion)" />
     <PackageVersion Include="Microsoft.Extensions.Configuration.UserSecrets" Version="$(AspnetVersion)" />
-    <PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="$(AspnetVersion)" />
-    <PackageVersion Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="$(AspnetVersion)" />
+    <PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />
+    <PackageVersion Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="8.0.0" />
     <PackageVersion Include="Microsoft.Extensions.Diagnostics.HealthChecks" Version="$(AspnetVersion)" />
     <PackageVersion Include="Microsoft.Extensions.Diagnostics.HealthChecks.EntityFrameworkCore" Version="$(AspnetVersion)" />
     <PackageVersion Include="Microsoft.Extensions.Http" Version="$(AspnetVersion)" />
@@ -161,6 +161,6 @@
     </PackageVersion>
 
     <PackageVersion Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="$(AspnetVersion)" PrivateAssets="All" />
-		<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="$(AspnetVersion)" PrivateAssets="All" />
+    <PackageVersion Include="Microsoft.SourceLink.GitHub" Version="$(AspnetVersion)" PrivateAssets="All" />
   </ItemGroup>
 </Project>
diff --git a/README.md b/README.md
@@ -62,7 +62,9 @@ In the meantime, NuGet packages will be published to the [IdentityServer8 NuGet
 | | |
 
 
+## What's New
 
+View the [CHANGELOG](docs/CHANGELOG.md) for the latest changes.
 
 ## About IdentityServer8
 [<img align="right" width="100px" src="https://dotnetfoundation.org/img/logo_big.svg" />](https://dotnetfoundation.org/projects?searchquery=IdentityServer&type=project)

diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
@@ -0,0 +1,51 @@
+# Change Log
+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](http://keepachangelog.com/)
+and this project adheres to [Semantic Versioning 2](http://semver.org/).
+
+## [Unreleased] - 2024-02-11
+
+- Javascript and NPM package updates to address several security vulnerablities.
+- Account Login and View security patch.
+
+### Added
+- [PROJECTNAME-XXXX](http://tickets.projectname.com/browse/PROJECTNAME-XXXX)
+  MINOR Ticket title goes here.
+- [PROJECTNAME-YYYY](http://tickets.projectname.com/browse/PROJECTNAME-YYYY)
+  PATCH Ticket title goes here.
+
+### Changed
+- [Account Login Controller] (https://github.com/alexhiggins732/IdentityServer8/issues/9) 
+- [Account Login View] (https://github.com/alexhiggins732/IdentityServer8/issues/9)  
+
+### Fixed
+- [Security: User-controlled bypass of sensitive method]
+  Login Controller and view have have explicit methods to handle login and cancel to address User-controlled bypass of sensitive method
+
+## [8.0.1] - 2024-02-10
+
+Updated build scripts to use Git Flow branching for SemVer2 compatible nuget packages.
+
+### Added
+
+- CodeQl Security scanning
+- Dependabot Package scanning. 
+### Changed
+
+- [IdentityServer8 8.0.1 changes]https://github.com/alexhiggins732/IdentityServer8/pull/7) 
+
+### Fixed
+
+- Nuget Package version conflicts.
+
+## [8.0.0] - 2024-02-09
+
+### Added
+Build scripts and readme documentation for initial port from Identity Server 4 and Identity Server 4 Admin   
+### Changed
+Upgraded Main Identity Server projects and Nuget packages to DotNet 8 
+### Fixed
+
+- Changed mixed dependencies on `System.Text.Json` and `Newtonsoft.Json` to use `System.Text.Json` which resolved several bugs.
+- Change package dependencies and version requirements to run on the latest DotNet 8 packages, resolving many security vulnerablities.
diff --git a/src/AspNetIdentity/host/Quickstart/Account/AccountController.cs b/src/AspNetIdentity/host/Quickstart/Account/AccountController.cs
@@ -81,38 +81,10 @@ public async Task<IActionResult> Login(string returnUrl)
         /// </summary>
         [HttpPost]
         [ValidateAntiForgeryToken]
-        public async Task<IActionResult> Login(LoginInputModel model, string button)
+        public async Task<IActionResult> Login(LoginInputModel model)
         {
-            // check if we are in the context of an authorization request
             var context = await _interaction.GetAuthorizationContextAsync(model.ReturnUrl);
-
-            // the user clicked the "cancel" button
-            if (button != "login")
-            {
-                if (context != null)
-                {
-                    // if the user cancels, send a result back into IdentityServer as if they 
-                    // denied the consent (even if this client does not require consent).
-                    // this will send back an access denied OIDC error response to the client.
-                    await _interaction.DenyAuthorizationAsync(context, AuthorizationError.AccessDenied);
-
-                    // we can trust model.ReturnUrl since GetAuthorizationContextAsync returned non-null
-                    if (context.IsNativeClient())
-                    {
-                        // The client is native, so this change in how to
-                        // return the response is for better UX for the end user.
-                        return this.LoadingPage("Redirect", model.ReturnUrl);
-                    }
-
-                    return Redirect(model.ReturnUrl);
-                }
-                else
-                {
-                    // since we don't have a valid context, then we just go back to the home page
-                    return Redirect("~/");
-                }
-            }
-
+            // check if we are in the context of an authorization request
             if (ModelState.IsValid)
             {
                 var result = await _signInManager.PasswordSignInAsync(model.Username, model.Password, model.RememberLogin, lockoutOnFailure: true);
@@ -150,7 +122,7 @@ public async Task<IActionResult> Login(LoginInputModel model, string button)
                     }
                 }
 
-                await _events.RaiseAsync(new UserLoginFailureEvent(model.Username, "invalid credentials", clientId:context?.Client.ClientId));
+                await _events.RaiseAsync(new UserLoginFailureEvent(model.Username, "invalid credentials", clientId: context?.Client.ClientId));
                 ModelState.AddModelError(string.Empty, AccountOptions.InvalidCredentialsErrorMessage);
             }
 
@@ -159,7 +131,42 @@ public async Task<IActionResult> Login(LoginInputModel model, string button)
             return View(vm);
         }
 
-
+        /// <summary>
+        /// Handle postback from username/password login
+        /// </summary>
+        [HttpPost]
+        [ValidateAntiForgeryToken]
+        public async Task<IActionResult> LoginCancel(LoginInputModel model)
+        {
+            // check if we are in the context of an authorization request
+            var context = await _interaction.GetAuthorizationContextAsync(model.ReturnUrl);
+
+            if (context != null)
+            {
+                // if the user cancels, send a result back into IdentityServer as if they 
+                // denied the consent (even if this client does not require consent).
+                // this will send back an access denied OIDC error response to the client.
+                await _interaction.DenyAuthorizationAsync(context, AuthorizationError.AccessDenied);
+
+                // we can trust model.ReturnUrl since GetAuthorizationContextAsync returned non-null
+                if (context.IsNativeClient())
+                {
+                    // The client is native, so this change in how to
+                    // return the response is for better UX for the end user.
+                    return this.LoadingPage("Redirect", model.ReturnUrl);
+                }
+
+                return Redirect(model.ReturnUrl);
+            }
+            else
+            {
+                // since we don't have a valid context, then we just go back to the home page
+                return Redirect("~/");
+            }
+
+        }
+
+
         /// <summary>
         /// Show logout page
         /// </summary>

diff --git a/src/AspNetIdentity/host/Quickstart/Consent/ConsentController.cs b/src/AspNetIdentity/host/Quickstart/Consent/ConsentController.cs
@@ -25,7 +25,7 @@ copies or substantial portions of the Software.
 using IdentityServer8.Validation;
 using System.Collections.Generic;
 using System;
-
+using Microsoft.DependencyInjection.Extensions;
 namespace IdentityServerHost.Quickstart.UI
 {
     /// <summary>
@@ -181,7 +181,7 @@ private async Task<ConsentViewModel> BuildViewModelAsync(string returnUrl, Conse
             }
             else
             {
-                _logger.LogError("No consent request matching request: {0}", returnUrl);
+                _logger.LogError("No consent request matching request: {0}", Ioc.Sanitizer.Log.Sanitize(returnUrl));
             }
 
             return null;

diff --git a/src/AspNetIdentity/host/Views/Account/Login.cshtml b/src/AspNetIdentity/host/Views/Account/Login.cshtml
@@ -42,7 +42,7 @@
                                 </div>
                             }
                             <button class="btn btn-primary" name="button" value="login">Login</button>
-                            <button class="btn btn-secondary" name="button" value="cancel">Cancel</button>
+                            <button class="btn btn-secondary" name="button" value="cancel" formaction="/Account/LoginCancel">Cancel</button>
                         </form>
                     </div>
                 </div>

diff --git a/src/EntityFramework.Storage/src/IdentityServer8.EntityFramework.Storage.csproj b/src/EntityFramework.Storage/src/IdentityServer8.EntityFramework.Storage.csproj
@@ -17,6 +17,7 @@
     </ItemGroup>
 
     <ItemGroup>
+        <ProjectReference Include="..\..\Security\IdentityServer8.Security\IdentityServer8.Security.csproj" />
         <ProjectReference Include="..\..\Storage\src\IdentityServer8.Storage.csproj" />
     </ItemGroup>
 

diff --git a/src/EntityFramework.Storage/src/Stores/ClientStore.cs b/src/EntityFramework.Storage/src/Stores/ClientStore.cs
@@ -22,7 +22,8 @@ copies or substantial portions of the Software.
 using IdentityServer8.Stores;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
-
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.DependencyInjection.Extensions;
 namespace IdentityServer8.EntityFramework.Stores
 {
     /// <summary>
@@ -81,7 +82,7 @@ public virtual async Task<Client> FindClientByIdAsync(string clientId)
 
             var model = client.ToModel();
 
-            Logger.LogDebug("{clientId} found in database: {clientIdFound}", clientId, model != null);
+            Logger.LogDebug("{clientId} found in database: {clientIdFound}", Ioc.Sanitizer.Log.Sanitize(clientId), model != null);
 
             return model;
         }

diff --git a/src/EntityFramework/src/Services/CorsPolicyService.cs b/src/EntityFramework/src/Services/CorsPolicyService.cs
@@ -22,6 +22,7 @@ copies or substantial portions of the Software.
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.DependencyInjection.Extensions;
 
 namespace IdentityServer8.EntityFramework.Services
 {
@@ -64,7 +65,7 @@ public async Task<bool> IsOriginAllowedAsync(string origin)
 
             var isAllowed = await query.AnyAsync();
 
-            _logger.LogDebug("Origin {origin} is allowed: {originAllowed}", origin, isAllowed);
+            _logger.LogDebug("Origin {origin} is allowed: {originAllowed}", Ioc.Sanitizer.Log.Sanitize(origin), isAllowed);
 
             return isAllowed;
         }

diff --git a/src/IdentityServer8.sln b/src/IdentityServer8.sln
@@ -95,6 +95,19 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "workflows", "workflows", "{
 		..\.github\workflows\release.yml = ..\.github\workflows\release.yml
 	EndProjectSection
 EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "docs", "docs", "{21B3EAAF-1A7D-4D46-AB3F-843296EDBDC2}"
+	ProjectSection(SolutionItems) = preProject
+		..\docs\CHANGELOG.md = ..\docs\CHANGELOG.md
+	EndProjectSection
+EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Security", "Security", "{9B9C47C4-560E-4F2E-8F53-97F9FDE46008}"
+EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Security", "Security", "{19B652D0-0AA1-415C-AA62-065EE8C77182}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "IdentityServer8.Security", "Security\IdentityServer8.Security\IdentityServer8.Security.csproj", "{284DF9E0-8007-4E84-949D-5B610D76B1CF}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "IdentityServer8.Sanitizer.Tests", "Security\test\IdentityServer8.Santizer.Tests\IdentityServer8.Sanitizer.Tests.csproj", "{00BDF864-B06A-4A48-B8B4-85C219B33CD1}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -164,6 +177,14 @@ Global
 		{E3685B06-F135-4318-B841-889C35479D5C}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{E3685B06-F135-4318-B841-889C35479D5C}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{E3685B06-F135-4318-B841-889C35479D5C}.Release|Any CPU.Build.0 = Release|Any CPU
+		{284DF9E0-8007-4E84-949D-5B610D76B1CF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{284DF9E0-8007-4E84-949D-5B610D76B1CF}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{284DF9E0-8007-4E84-949D-5B610D76B1CF}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{284DF9E0-8007-4E84-949D-5B610D76B1CF}.Release|Any CPU.Build.0 = Release|Any CPU
+		{00BDF864-B06A-4A48-B8B4-85C219B33CD1}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{00BDF864-B06A-4A48-B8B4-85C219B33CD1}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{00BDF864-B06A-4A48-B8B4-85C219B33CD1}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{00BDF864-B06A-4A48-B8B4-85C219B33CD1}.Release|Any CPU.Build.0 = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE
@@ -199,6 +220,11 @@ Global
 		{A7171FEC-FEC1-4AF0-9625-E69D93F08A42} = {29DC1E06-3EC5-4F52-9EC1-0363BD571369}
 		{0BC5E76A-A280-49C8-8E96-A43FEA357A4B} = {A7171FEC-FEC1-4AF0-9625-E69D93F08A42}
 		{C5474352-0715-41C0-92C2-BABA1A4103A0} = {A7171FEC-FEC1-4AF0-9625-E69D93F08A42}
+		{21B3EAAF-1A7D-4D46-AB3F-843296EDBDC2} = {29DC1E06-3EC5-4F52-9EC1-0363BD571369}
+		{9B9C47C4-560E-4F2E-8F53-97F9FDE46008} = {5461C61B-B06E-46BA-B206-925B660BE727}
+		{19B652D0-0AA1-415C-AA62-065EE8C77182} = {45C22EDD-91B1-4AEF-8620-77F4E3E7C544}
+		{284DF9E0-8007-4E84-949D-5B610D76B1CF} = {9B9C47C4-560E-4F2E-8F53-97F9FDE46008}
+		{00BDF864-B06A-4A48-B8B4-85C219B33CD1} = {19B652D0-0AA1-415C-AA62-065EE8C77182}
 	EndGlobalSection
 	GlobalSection(ExtensibilityGlobals) = postSolution
 		SolutionGuid = {176723F7-4A9B-4F05-A9F3-3BA715E4BDC3}

diff --git a/src/IdentityServer8/host/Quickstart/Account/AccountController.cs b/src/IdentityServer8/host/Quickstart/Account/AccountController.cs
@@ -86,38 +86,11 @@ public async Task<IActionResult> Login(string returnUrl)
         /// </summary>
         [HttpPost]
         [ValidateAntiForgeryToken]
-        public async Task<IActionResult> Login(LoginInputModel model, string button)
+        public async Task<IActionResult> Login(LoginInputModel model)
         {
             // check if we are in the context of an authorization request
             var context = await _interaction.GetAuthorizationContextAsync(model.ReturnUrl);
 
-            // the user clicked the "cancel" button
-            if (button != "login")
-            {
-                if (context != null)
-                {
-                    // if the user cancels, send a result back into IdentityServer as if they 
-                    // denied the consent (even if this client does not require consent).
-                    // this will send back an access denied OIDC error response to the client.
-                    await _interaction.DenyAuthorizationAsync(context, AuthorizationError.AccessDenied);
-
-                    // we can trust model.ReturnUrl since GetAuthorizationContextAsync returned non-null
-                    if (context.IsNativeClient())
-                    {
-                        // The client is native, so this change in how to
-                        // return the response is for better UX for the end user.
-                        return this.LoadingPage("Redirect", model.ReturnUrl);
-                    }
-
-                    return Redirect(model.ReturnUrl);
-                }
-                else
-                {
-                    // since we don't have a valid context, then we just go back to the home page
-                    return Redirect("~/");
-                }
-            }
-
             if (ModelState.IsValid)
             {
                 // validate username/password against in-memory store
@@ -175,7 +148,7 @@ public async Task<IActionResult> Login(LoginInputModel model, string button)
                     }
                 }
 
-                await _events.RaiseAsync(new UserLoginFailureEvent(model.Username, "invalid credentials", clientId:context?.Client.ClientId));
+                await _events.RaiseAsync(new UserLoginFailureEvent(model.Username, "invalid credentials", clientId: context?.Client.ClientId));
                 ModelState.AddModelError(string.Empty, AccountOptions.InvalidCredentialsErrorMessage);
             }
 
@@ -184,7 +157,41 @@ public async Task<IActionResult> Login(LoginInputModel model, string button)
             return View(vm);
         }
 
-
+        /// <summary>
+        /// Handle postback from username/password login
+        /// </summary>
+        [HttpPost]
+        [ValidateAntiForgeryToken]
+        public async Task<IActionResult> LoginCancel(LoginInputModel model)
+        {
+            // check if we are in the context of an authorization request
+            var context = await _interaction.GetAuthorizationContextAsync(model.ReturnUrl);
+
+            if (context != null)
+            {
+                // if the user cancels, send a result back into IdentityServer as if they 
+                // denied the consent (even if this client does not require consent).
+                // this will send back an access denied OIDC error response to the client.
+                await _interaction.DenyAuthorizationAsync(context, AuthorizationError.AccessDenied);
+
+                // we can trust model.ReturnUrl since GetAuthorizationContextAsync returned non-null
+                if (context.IsNativeClient())
+                {
+                    // The client is native, so this change in how to
+                    // return the response is for better UX for the end user.
+                    return this.LoadingPage("Redirect", model.ReturnUrl);
+                }
+
+                return Redirect(model.ReturnUrl);
+            }
+            else
+            {
+                // since we don't have a valid context, then we just go back to the home page
+                return Redirect("~/");
+            }
+
+        }
+
         /// <summary>
         /// Show logout page
         /// </summary>