diff --git a/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreHandlers.cs b/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreHandlers.cs index be02bb98..6320c475 100644 --- a/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreHandlers.cs +++ b/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreHandlers.cs @@ -215,7 +215,8 @@ public static partial class OpenIddictClientAspNetCoreHandlers } /// - /// Contains the logic responsible for validating the correlation cookie that serves as a CSRF protection. + /// Contains the logic responsible for validating the correlation cookie that serves as a + /// protection against state token injection, forged requests and session fixation attacks. /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. /// public class ValidateCorrelationCookie : IOpenIddictClientHandler @@ -255,8 +256,8 @@ public static partial class OpenIddictClientAspNetCoreHandlers // Resolve the request forgery protection from the state token principal. // If the claim cannot be found, this means the protection was disabled // using a custom event handler. In this case, bypass the validation. - var claim = context.StateTokenPrincipal.GetClaim(Claims.RequestForgeryProtection); - if (string.IsNullOrEmpty(claim)) + var identifier = context.StateTokenPrincipal.GetClaim(Claims.RequestForgeryProtection); + if (string.IsNullOrEmpty(identifier)) { return default; } @@ -268,15 +269,19 @@ public static partial class OpenIddictClientAspNetCoreHandlers // and the random request forgery protection claim restored from the state. var name = new StringBuilder(builder.Name) .Append(Separators.Dot) - .Append(claim) + .Append(identifier) .ToString(); // Try to find the cookie matching the request forgery protection stored in the state. // // If the cookie cannot be found, this may indicate that the authorization response // is unsolicited and potentially malicious. This may also be caused by an unadequate - // same-site configuration. In any case, the authentication demand MUST be rejected - // as it's impossible to ensure it's not a session fixation attack without the cookie. + // same-site configuration. The correlation cookie also serves as a binding mechanism + // ensuring that a state token stolen from an authorization response with the other + // parameters cannot be validly used without sending the matching correlation identifier. + // + // In any case, the authentication demand MUST be rejected as it's impossible to ensure + // it's not an injection or session fixation attack without the correlation cookie. var value = request.Cookies[name]; if (string.IsNullOrEmpty(value) || !string.Equals(value, "v1", StringComparison.Ordinal)) { @@ -347,7 +352,7 @@ public static partial class OpenIddictClientAspNetCoreHandlers // abort the authorization flow by rejecting the token request that may be eventually sent // with the original redirect_uri, many servers are known to incorrectly implement this // redirect_uri validation logic. This check also offers limited protection as it cannot - // prevent the client credentials from being leaked to a malicious authorization server. + // prevent the authorization code from being leaked to a malicious authorization server. // By comparing the redirect_uri directly in the client, a first layer of protection is // provided independently of whether the authorization server will enforce this check. // @@ -455,7 +460,8 @@ public static partial class OpenIddictClientAspNetCoreHandlers } /// - /// Contains the logic responsible for creating a correlation cookie that serves as a CSRF countermeasure. + /// Contains the logic responsible for creating a correlation cookie that serves as a + /// protection against state token injection, forged requests and session fixation attacks. /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. /// public class GenerateCorrelationCookie : IOpenIddictClientHandler @@ -485,8 +491,8 @@ public static partial class OpenIddictClientAspNetCoreHandlers throw new ArgumentNullException(nameof(context)); } - // Note: using a correlation cookie serves as an antiforgery protection as the request will - // always be rejected if a cookie corresponding to the request forgery protection claim + // Note: using a correlation cookie serves as an injection/antiforgery protection as the request + // will always be rejected if a cookie corresponding to the request forgery protection claim // persisted in the state token cannot be found. This protection is considered essential // in OpenIddict and cannot be disabled via the options. Applications that prefer implementing // a different protection strategy can set the request forgery protection claim to null or @@ -498,7 +504,6 @@ public static partial class OpenIddictClientAspNetCoreHandlers } Debug.Assert(context.StateTokenPrincipal is { Identity: ClaimsIdentity }, SR.GetResourceString(SR.ID4006)); - Debug.Assert(!string.IsNullOrEmpty(context.StateToken), SR.GetResourceString(SR.ID4010)); // This handler only applies to ASP.NET Core requests. If the HTTP context cannot be resolved, // this may indicate that the request was incorrectly processed by another server stack. diff --git a/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreOptions.cs b/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreOptions.cs index 2e41147c..f11ec658 100644 --- a/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreOptions.cs +++ b/src/OpenIddict.Client.AspNetCore/OpenIddictClientAspNetCoreOptions.cs @@ -40,14 +40,15 @@ public class OpenIddictClientAspNetCoreOptions : AuthenticationSchemeOptions public bool EnableStatusCodePagesIntegration { get; set; } /// - /// Gets or sets the cookie builder used to create the cookies that are - /// used to protect against forged requests/session fixation attacks. + /// Gets or sets the cookie builder used to create the cookies that are used to + /// bind authorization responses with their original request and help mitigate + /// authorization code injection, forged requests and session fixation attacks. /// public CookieBuilder CookieBuilder { get; set; } = new() { HttpOnly = true, IsEssential = true, - Name = "OpenIddict.Client.RequestForgeryProtection", + Name = "OpenIddict.Client.State", SameSite = SameSiteMode.None, SecurePolicy = CookieSecurePolicy.Always // Note: same-site=none requires using HTTPS. }; diff --git a/src/OpenIddict.Client.Owin/OpenIddictClientOwinHandlers.cs b/src/OpenIddict.Client.Owin/OpenIddictClientOwinHandlers.cs index 3a219a35..5b39ea4b 100644 --- a/src/OpenIddict.Client.Owin/OpenIddictClientOwinHandlers.cs +++ b/src/OpenIddict.Client.Owin/OpenIddictClientOwinHandlers.cs @@ -212,7 +212,8 @@ public static partial class OpenIddictClientOwinHandlers } /// - /// Contains the logic responsible for validating the correlation cookie that serves as a CSRF protection. + /// Contains the logic responsible for validating the correlation cookie that serves as a + /// protection against state token injection, forged requests and session fixation attacks. /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. /// public class ValidateCorrelationCookie : IOpenIddictClientHandler @@ -252,8 +253,8 @@ public static partial class OpenIddictClientOwinHandlers // Resolve the request forgery protection from the state token principal. // If the claim cannot be found, this means the protection was disabled // using a custom event handler. In this case, bypass the validation. - var claim = context.StateTokenPrincipal.GetClaim(Claims.RequestForgeryProtection); - if (string.IsNullOrEmpty(claim)) + var identifier = context.StateTokenPrincipal.GetClaim(Claims.RequestForgeryProtection); + if (string.IsNullOrEmpty(identifier)) { return default; } @@ -267,15 +268,19 @@ public static partial class OpenIddictClientOwinHandlers // and the random request forgery protection claim restored from the state. var name = new StringBuilder(_options.CurrentValue.CookieName) .Append(Separators.Dot) - .Append(claim) + .Append(identifier) .ToString(); // Try to find the cookie matching the request forgery protection stored in the state. // // If the cookie cannot be found, this may indicate that the authorization response // is unsolicited and potentially malicious. This may also be caused by an unadequate - // same-site configuration. In any case, the authentication demand MUST be rejected - // as it's impossible to ensure it's not a session fixation attack without the cookie. + // same-site configuration. The correlation cookie also serves as a binding mechanism + // ensuring that a state token stolen from an authorization response with the other + // parameters cannot be validly used without sending the matching correlation identifier. + // + // In any case, the authentication demand MUST be rejected as it's impossible to ensure + // it's not an injection or session fixation attack without the correlation cookie. var value = manager.GetRequestCookie(request.Context, name); if (string.IsNullOrEmpty(value) || !string.Equals(value, "v1", StringComparison.Ordinal)) { @@ -353,7 +358,7 @@ public static partial class OpenIddictClientOwinHandlers // abort the authorization flow by rejecting the token request that may be eventually sent // with the original redirect_uri, many servers are known to incorrectly implement this // redirect_uri validation logic. This check also offers limited protection as it cannot - // prevent the client credentials from being leaked to a malicious authorization server. + // prevent the authorization code from being leaked to a malicious authorization server. // By comparing the redirect_uri directly in the client, a first layer of protection is // provided independently of whether the authorization server will enforce this check. // @@ -454,7 +459,8 @@ public static partial class OpenIddictClientOwinHandlers } /// - /// Contains the logic responsible for creating a correlation cookie that serves as a CSRF countermeasure. + /// Contains the logic responsible for creating a correlation cookie that serves as a + /// protection against state token injection, forged requests and session fixation attacks. /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. /// public class GenerateCorrelationCookie : IOpenIddictClientHandler @@ -484,8 +490,8 @@ public static partial class OpenIddictClientOwinHandlers throw new ArgumentNullException(nameof(context)); } - // Note: using a correlation cookie serves as an antiforgery protection as the request will - // always be rejected if a cookie corresponding to the request forgery protection claim + // Note: using a correlation cookie serves as an injection/antiforgery protection as the request + // will always be rejected if a cookie corresponding to the request forgery protection claim // persisted in the state token cannot be found. This protection is considered essential // in OpenIddict and cannot be disabled via the options. Applications that prefer implementing // a different protection strategy can set the request forgery protection claim to null or @@ -497,7 +503,6 @@ public static partial class OpenIddictClientOwinHandlers } Debug.Assert(context.StateTokenPrincipal is { Identity: ClaimsIdentity }, SR.GetResourceString(SR.ID4006)); - Debug.Assert(!string.IsNullOrEmpty(context.StateToken), SR.GetResourceString(SR.ID4010)); // This handler only applies to OWIN requests. If the HTTP context cannot be resolved, // this may indicate that the request was incorrectly processed by another server stack. diff --git a/src/OpenIddict.Client.Owin/OpenIddictClientOwinOptions.cs b/src/OpenIddict.Client.Owin/OpenIddictClientOwinOptions.cs index 2a553626..2fc93527 100644 --- a/src/OpenIddict.Client.Owin/OpenIddictClientOwinOptions.cs +++ b/src/OpenIddict.Client.Owin/OpenIddictClientOwinOptions.cs @@ -45,13 +45,16 @@ public class OpenIddictClientOwinOptions : AuthenticationOptions public ICookieManager CookieManager { get; set; } = new CookieManager(); /// - /// Gets or sets the name of the cookie used to protect against forged requests/session fixation attacks. + /// Gets or sets the name of the correlation cookie used to bind authorization + /// responses with their original request and help mitigate authorization + /// code injection, forged requests and session fixation attacks. /// - public string CookieName { get; set; } = "OpenIddict.Client.RequestForgeryProtection"; + public string CookieName { get; set; } = "OpenIddict.Client.State"; /// - /// Gets or sets the cookie options used to create the cookies that are - /// used to protect against forged requests/session fixation attacks. + /// Gets or sets the cookie options used to create the cookies that are used to + /// bind authorization responses with their original request and help mitigate + /// authorization code injection, forged requests and session fixation attacks. /// public CookieOptions CookieOptions { get; set; } = new() { diff --git a/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.Userinfo.cs b/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.Userinfo.cs index 0e2d5e0f..61ead133 100644 --- a/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.Userinfo.cs +++ b/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.Userinfo.cs @@ -23,6 +23,7 @@ public static partial class OpenIddictClientSystemNetHttpHandlers */ PrepareGetHttpRequest.Descriptor, AttachBearerAccessToken.Descriptor, + AttachQueryStringParameters.Descriptor, SendHttpRequest.Descriptor, DisposeHttpRequest.Descriptor, @@ -44,7 +45,7 @@ public static partial class OpenIddictClientSystemNetHttpHandlers = OpenIddictClientHandlerDescriptor.CreateBuilder() .AddFilter() .UseSingletonHandler() - .SetOrder(AttachFormParameters.Descriptor.Order - 1000) + .SetOrder(AttachQueryStringParameters.Descriptor.Order - 500) .SetType(OpenIddictClientHandlerType.BuiltIn) .Build(); @@ -97,6 +98,12 @@ public static partial class OpenIddictClientSystemNetHttpHandlers throw new ArgumentNullException(nameof(context)); } + // Don't overwrite the response if one was already provided. + if (context.Response is not null || !string.IsNullOrEmpty(context.UserinfoToken)) + { + return; + } + // This handler only applies to System.Net.Http requests. If the HTTP response cannot be resolved, // this may indicate that the request was incorrectly processed by another client stack. var response = context.Transaction.GetHttpResponseMessage() ?? diff --git a/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.cs b/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.cs index 2cca67c0..b9920aa4 100644 --- a/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.cs +++ b/src/OpenIddict.Client.SystemNetHttp/OpenIddictClientSystemNetHttpHandlers.cs @@ -345,6 +345,12 @@ public static partial class OpenIddictClientSystemNetHttpHandlers throw new ArgumentNullException(nameof(context)); } + // Don't overwrite the response if one was already provided. + if (context.Transaction.Response is not null) + { + return; + } + // This handler only applies to System.Net.Http requests. If the HTTP response cannot be resolved, // this may indicate that the request was incorrectly processed by another client stack. var response = context.Transaction.GetHttpResponseMessage() ?? diff --git a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs index 22db0ff8..9772e18e 100644 --- a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs +++ b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs @@ -344,6 +344,12 @@ public static partial class OpenIddictValidationSystemNetHttpHandlers throw new ArgumentNullException(nameof(context)); } + // Don't overwrite the response if one was already provided. + if (context.Transaction.Response is not null) + { + return; + } + // This handler only applies to System.Net.Http requests. If the HTTP response cannot be resolved, // this may indicate that the request was incorrectly processed by another client stack. var response = context.Transaction.GetHttpResponseMessage() ??