diff --git a/src/OpenIddict.Abstractions/OpenIddictConstants.cs b/src/OpenIddict.Abstractions/OpenIddictConstants.cs index b078ab77..9d4c9d57 100644 --- a/src/OpenIddict.Abstractions/OpenIddictConstants.cs +++ b/src/OpenIddict.Abstractions/OpenIddictConstants.cs @@ -439,6 +439,10 @@ public static class OpenIddictConstants public static readonly char[] Ampersand = { '&' }; public static readonly char[] Dash = { '-' }; public static readonly char[] Dot = { '.' }; + public static readonly char[] EqualsSign = { '=' }; + public static readonly char[] Hash = { '#' }; + public static readonly char[] QuestionMark = { '?' }; + public static readonly char[] Semicolon = { ';' }; public static readonly char[] Space = { ' ' }; } diff --git a/src/OpenIddict.Abstractions/OpenIddictResources.resx b/src/OpenIddict.Abstractions/OpenIddictResources.resx index a38aba6c..7fa4af06 100644 --- a/src/OpenIddict.Abstractions/OpenIddictResources.resx +++ b/src/OpenIddict.Abstractions/OpenIddictResources.resx @@ -1587,6 +1587,12 @@ To apply redirection responses, create a class implementing 'IOpenIddictClientHa The '{0}' claim returned in the specified userinfo response/token doesn't match the expected value. + + Callback URLs cannot contain an "{0}" parameter. + + + The '{0}' parameter must not include a '{1}' component. + The '{0}' parameter shouldn't be null or empty at this point. @@ -2132,6 +2138,9 @@ This may indicate that the hashed entry is corrupted or malformed. The redirection request was successfully validated. + + The authorization request was rejected because the '{Parameter}' contained a forbidden parameter: {Name}. + https://documentation.openiddict.com/errors/{0} diff --git a/src/OpenIddict.Abstractions/Primitives/OpenIddictResponse.cs b/src/OpenIddict.Abstractions/Primitives/OpenIddictResponse.cs index 12671a6e..e2cc9836 100644 --- a/src/OpenIddict.Abstractions/Primitives/OpenIddictResponse.cs +++ b/src/OpenIddict.Abstractions/Primitives/OpenIddictResponse.cs @@ -153,6 +153,15 @@ public class OpenIddictResponse : OpenIddictMessage set => SetParameter(OpenIddictConstants.Parameters.IdToken, value); } + /// + /// Gets or sets the "iss" parameter. + /// + public string? Iss + { + get => (string?) GetParameter(OpenIddictConstants.Parameters.Iss); + set => SetParameter(OpenIddictConstants.Parameters.Iss, value); + } + /// /// Gets or sets the "refresh_token" parameter. /// diff --git a/src/OpenIddict.Client/OpenIddictClientConfiguration.cs b/src/OpenIddict.Client/OpenIddictClientConfiguration.cs index bfc0c45b..04de69ef 100644 --- a/src/OpenIddict.Client/OpenIddictClientConfiguration.cs +++ b/src/OpenIddict.Client/OpenIddictClientConfiguration.cs @@ -65,7 +65,7 @@ public class OpenIddictClientConfiguration : IPostConfigureOptions null, + + // If the issuer URI doesn't contain any path/query/fragment, allow both http://www.fabrikam.com + // and http://www.fabrikam.com/ (the recommended URI representation) to be considered valid. + // See https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.3 for more information. + { AbsolutePath: "/", Query.Length: 0, Fragment.Length: 0 } issuer => new[] + { + issuer.AbsoluteUri, // Uri.AbsoluteUri is normalized and always contains a trailing slash. + issuer.AbsoluteUri.Substring(0, issuer.AbsoluteUri.Length - 1) + }, + + Uri issuer => new[] { issuer.AbsoluteUri } + }; + + parameters.ValidateIssuer = parameters.ValidIssuers is not null; // Combine the signing keys registered statically in the token validation parameters // with the signing keys resolved from the OpenID Connect server configuration. diff --git a/src/OpenIddict.Client/OpenIddictClientHandlers.Userinfo.cs b/src/OpenIddict.Client/OpenIddictClientHandlers.Userinfo.cs index 0ee86817..a14aeeec 100644 --- a/src/OpenIddict.Client/OpenIddictClientHandlers.Userinfo.cs +++ b/src/OpenIddict.Client/OpenIddictClientHandlers.Userinfo.cs @@ -129,7 +129,7 @@ public static partial class OpenIddictClientHandlers // Resolve the issuer that will be attached to the claims created by this handler. // Note: at this stage, the optional issuer extracted from the response is assumed // to be valid, as it is guarded against unknown values by the ValidateIssuer handler. - var issuer = (string?) context.Response[Claims.Issuer] ?? configuration.Issuer!.OriginalString; + var issuer = (string?) context.Response[Claims.Issuer] ?? configuration.Issuer!.AbsoluteUri; foreach (var parameter in context.Response.GetParameters()) { diff --git a/src/OpenIddict.Client/OpenIddictClientHandlers.cs b/src/OpenIddict.Client/OpenIddictClientHandlers.cs index 0a50834f..8f25bc61 100644 --- a/src/OpenIddict.Client/OpenIddictClientHandlers.cs +++ b/src/OpenIddict.Client/OpenIddictClientHandlers.cs @@ -445,7 +445,8 @@ public static partial class OpenIddictClientHandlers } // If the two values don't match, this may indicate a mix-up attack attempt. - if (!string.Equals(issuer, context.Issuer.AbsoluteUri, StringComparison.Ordinal)) + if (!Uri.TryCreate(issuer, UriKind.Absolute, out Uri? uri) || + !uri.IsWellFormedOriginalString() || uri != configuration.Issuer) { context.Reject( error: Errors.InvalidRequest, diff --git a/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs b/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs index 4b8c67bd..9625b8af 100644 --- a/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs +++ b/src/OpenIddict.Core/Managers/OpenIddictApplicationManager.cs @@ -1226,6 +1226,19 @@ public class OpenIddictApplicationManager : IOpenIddictApplication break; } + + // To prevent issuer fixation attacks where a malicious client would specify an "iss" parameter + // in the callback URL, ensure the query - if present - doesn't include an "iss" parameter. + if (!string.IsNullOrEmpty(uri.Query) && uri.Query.TrimStart(Separators.QuestionMark[0]) + .Split(new[] { Separators.Ampersand[0], Separators.Semicolon[0] }, StringSplitOptions.RemoveEmptyEntries) + .Select(parameter => parameter.Split(Separators.EqualsSign, StringSplitOptions.RemoveEmptyEntries)) + .Select(parts => parts[0] is string value ? Uri.UnescapeDataString(value) : null) + .Contains(Parameters.Iss, StringComparer.OrdinalIgnoreCase)) + { + yield return new ValidationResult(SR.FormatID2134(Parameters.Iss)); + + break; + } } } diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs index 9d8b3789..b054fd3e 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs @@ -240,7 +240,7 @@ public static partial class OpenIddictServerAspNetCoreHandlers .AddFilter() .AddFilter() .UseSingletonHandler() - .SetOrder(InferEndpointType.Descriptor.Order + 1_000) + .SetOrder(InferIssuerFromHost.Descriptor.Order + 1_000) .SetType(OpenIddictServerHandlerType.BuiltIn) .Build(); diff --git a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs index b9f2cb0a..5ab20b15 100644 --- a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs +++ b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs @@ -231,7 +231,7 @@ public static partial class OpenIddictServerOwinHandlers .AddFilter() .AddFilter() .UseSingletonHandler() - .SetOrder(InferEndpointType.Descriptor.Order + 1_000) + .SetOrder(InferIssuerFromHost.Descriptor.Order + 1_000) .SetType(OpenIddictServerHandlerType.BuiltIn) .Build(); diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs index b2914674..ad2cad99 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs @@ -56,7 +56,8 @@ public static partial class OpenIddictServerHandlers */ AttachRedirectUri.Descriptor, InferResponseMode.Descriptor, - AttachResponseState.Descriptor); + AttachResponseState.Descriptor, + AttachIssuer.Descriptor); /// /// Contains the logic responsible of extracting authorization requests and invoking the corresponding event handlers. @@ -532,6 +533,27 @@ public static partial class OpenIddictServerHandlers return default; } + // To prevent issuer fixation attacks where a malicious client would specify an "iss" parameter + // in the redirect_uri, ensure the query - if present - doesn't include an "iss" parameter. + // + // Note: while OAuth 2.0 parameters are case-sentitive, the following check deliberately + // uses a case-insensitive comparison to ensure that all variations of "iss" are rejected. + if (!string.IsNullOrEmpty(uri.Query) && uri.Query.TrimStart(Separators.QuestionMark[0]) + .Split(new[] { Separators.Ampersand[0], Separators.Semicolon[0] }, StringSplitOptions.RemoveEmptyEntries) + .Select(parameter => parameter.Split(Separators.EqualsSign, StringSplitOptions.RemoveEmptyEntries)) + .Select(parts => parts[0] is string value ? Uri.UnescapeDataString(value) : null) + .Contains(Parameters.Iss, StringComparer.OrdinalIgnoreCase)) + { + context.Logger.LogInformation(SR.GetResourceString(SR.ID6181), Parameters.RedirectUri, Parameters.Iss); + + context.Reject( + error: Errors.InvalidRequest, + description: SR.FormatID2135(Parameters.RedirectUri, Parameters.Iss), + uri: SR.FormatID8000(SR.ID2135)); + + return default; + } + return default; } } @@ -1723,8 +1745,11 @@ public static partial class OpenIddictServerHandlers throw new ArgumentNullException(nameof(context)); } - // Attach the request state to the authorization response. - if (string.IsNullOrEmpty(context.Response.State)) + // If the user agent is expected to be redirected to the client application, attach the request + // state to the authorization response to help the client mitigate CSRF/session fixation attacks. + // + // Note: don't override the state if one was already attached to the response instance. + if (!string.IsNullOrEmpty(context.RedirectUri) && string.IsNullOrEmpty(context.Response.State)) { context.Response.State = context.Request?.State; } @@ -1732,5 +1757,54 @@ public static partial class OpenIddictServerHandlers return default; } } + + /// + /// Contains the logic responsible of attaching an "iss" parameter + /// containing the address of the authorization server to the response. + /// + public class AttachIssuer : IOpenIddictServerHandler + { + /// + /// Gets the default descriptor definition assigned to this handler. + /// + public static OpenIddictServerHandlerDescriptor Descriptor { get; } + = OpenIddictServerHandlerDescriptor.CreateBuilder() + .UseSingletonHandler() + .SetOrder(AttachResponseState.Descriptor.Order + 1_000) + .SetType(OpenIddictServerHandlerType.BuiltIn) + .Build(); + + /// + public ValueTask HandleAsync(ApplyAuthorizationResponseContext context) + { + if (context is null) + { + throw new ArgumentNullException(nameof(context)); + } + + // If the user agent is expected to be redirected to the client application, attach the + // issuer address to the authorization response to help the client detect mix-up attacks. + // + // Note: this applies to all authorization responses, whether they represent valid or errored responses. + // For more information, see https://datatracker.ietf.org/doc/html/draft-ietf-oauth-iss-auth-resp-05. + + if (!string.IsNullOrEmpty(context.RedirectUri)) + { + // At this stage, throw an exception if the issuer cannot be retrieved. + if (context.Issuer is not { IsAbsoluteUri: true }) + { + throw new InvalidOperationException(SR.GetResourceString(SR.ID0023)); + } + + // Note: don't override the issuer if one was already attached to the response instance. + if (string.IsNullOrEmpty(context.Response.Iss)) + { + context.Response.Iss = context.Issuer.AbsoluteUri; + } + } + + return default; + } + } } } diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.Discovery.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.Discovery.cs index d76e9d7d..7f2eb59c 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.Discovery.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.Discovery.cs @@ -377,7 +377,7 @@ public static partial class OpenIddictServerHandlers } // At this stage, throw an exception if the issuer cannot be retrieved. - if (issuer is null || !issuer.IsAbsoluteUri) + if (issuer is not { IsAbsoluteUri: true }) { throw new InvalidOperationException(SR.GetResourceString(SR.ID0023)); } @@ -746,6 +746,11 @@ public static partial class OpenIddictServerHandlers context.Metadata[Metadata.RequestParameterSupported] = false; context.Metadata[Metadata.RequestUriParameterSupported] = false; + // As of 3.2.0, OpenIddict automatically returns an "iss" parameter containing its own address as + // part of authorization responses to help clients mitigate mix-up attacks. For more information, + // see https://datatracker.ietf.org/doc/html/draft-ietf-oauth-iss-auth-resp-05. + context.Metadata[Metadata.AuthorizationResponseIssParameterSupported] = true; + return default; } } diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.cs index 463d6250..bcdf9045 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.cs @@ -2953,7 +2953,7 @@ public static partial class OpenIddictServerHandlers } // At this stage, throw an exception if the issuer cannot be retrieved. - if (issuer is null || !issuer.IsAbsoluteUri) + if (issuer is not { IsAbsoluteUri: true }) { throw new InvalidOperationException(SR.GetResourceString(SR.ID0023)); } diff --git a/src/OpenIddict.Validation/OpenIddictValidationConfiguration.cs b/src/OpenIddict.Validation/OpenIddictValidationConfiguration.cs index 741cbd21..8e8cb15d 100644 --- a/src/OpenIddict.Validation/OpenIddictValidationConfiguration.cs +++ b/src/OpenIddict.Validation/OpenIddictValidationConfiguration.cs @@ -109,7 +109,7 @@ public class OpenIddictValidationConfiguration : IPostConfigureOptions null, + + // If the issuer URI doesn't contain any path/query/fragment, allow both http://www.fabrikam.com + // and http://www.fabrikam.com/ (the recommended URI representation) to be considered valid. + // See https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.3 for more information. + { AbsolutePath: "/", Query.Length: 0, Fragment.Length: 0 } issuer => new[] + { + issuer.AbsoluteUri, // Uri.AbsoluteUri is normalized and always contains a trailing slash. + issuer.AbsoluteUri.Substring(0, issuer.AbsoluteUri.Length - 1) + }, + + Uri issuer => new[] { issuer.AbsoluteUri } + }; + + parameters.ValidateIssuer = parameters.ValidIssuers is not null; // Combine the signing keys registered statically in the token validation parameters // with the signing keys resolved from the OpenID Connect server configuration. diff --git a/test/OpenIddict.Abstractions.Tests/Primitives/OpenIddictResponseTests.cs b/test/OpenIddict.Abstractions.Tests/Primitives/OpenIddictResponseTests.cs index 62fb8ba1..91210bbd 100644 --- a/test/OpenIddict.Abstractions.Tests/Primitives/OpenIddictResponseTests.cs +++ b/test/OpenIddict.Abstractions.Tests/Primitives/OpenIddictResponseTests.cs @@ -71,6 +71,13 @@ public class OpenIddictResponseTests /* value: */ new OpenIddictParameter("802A3E3E-DCCA-4EFC-89FA-7D82FE8C27E4") }; + yield return new object[] + { + /* property: */ nameof(OpenIddictResponse.Iss), + /* name: */ Parameters.Iss, + /* value: */ new OpenIddictParameter("802A3E3E-DCCA-4EFC-89FA-7D82FE8C27E4") + }; + yield return new object[] { /* property: */ nameof(OpenIddictResponse.RefreshToken), diff --git a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs index 51eb59d7..7eaf4cfa 100644 --- a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs +++ b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs @@ -241,6 +241,41 @@ public abstract partial class OpenIddictServerIntegrationTests Assert.Equal(SR.FormatID8000(message), response.ErrorUri); } + [Theory] + [InlineData("http://www.fabrikam.com/path?iss")] + [InlineData("http://www.fabrikam.com/path?iss=value")] + [InlineData("http://www.fabrikam.com/path?;iss")] + [InlineData("http://www.fabrikam.com/path?;iss=value")] + [InlineData("http://www.fabrikam.com/path?&iss")] + [InlineData("http://www.fabrikam.com/path?&iss=value")] + [InlineData("http://www.fabrikam.com/path?state;iss")] + [InlineData("http://www.fabrikam.com/path?state;iss=value")] + [InlineData("http://www.fabrikam.com/path?state&iss")] + [InlineData("http://www.fabrikam.com/path?state&iss=value")] + [InlineData("http://www.fabrikam.com/path?state=abc;iss")] + [InlineData("http://www.fabrikam.com/path?state=abc;iss=value")] + [InlineData("http://www.fabrikam.com/path?state=abc&iss")] + [InlineData("http://www.fabrikam.com/path?state=abc&iss=value")] + public async Task ValidateAuthorizationRequest_RedirectUriWithIssuerParameterCausesAnError(string address) + { + // Arrange + await using var server = await CreateServerAsync(options => options.EnableDegradedMode()); + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest + { + ClientId = "Fabrikam", + RedirectUri = address, + Scope = Scopes.OpenId + }); + + // Assert + Assert.Equal(Errors.InvalidRequest, response.Error); + Assert.Equal(SR.FormatID2135(Parameters.RedirectUri, Parameters.Iss), response.ErrorDescription); + Assert.Equal(SR.FormatID8000(SR.ID2135), response.ErrorUri); + } + [Fact] public async Task ValidateAuthorizationRequest_MissingResponseTypeCausesAnError() { @@ -2335,4 +2370,86 @@ public abstract partial class OpenIddictServerIntegrationTests // Assert Assert.Equal("custom_state", response.State); } + + [Theory] + [InlineData(null)] + [InlineData("http://www.fabrikam.com/")] + public async Task ApplyAuthorizationResponse_SetsIssuerParameter(string issuer) + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + + if (!string.IsNullOrEmpty(issuer)) + { + options.SetIssuer(new Uri(issuer, UriKind.Absolute)); + } + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) + .SetClaim(Claims.Subject, "Bob le Magnifique"); + + return default; + })); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest + { + ClientId = "Fabrikam", + RedirectUri = "http://www.fabrikam.com/path", + ResponseType = ResponseTypes.Code, + State = "af0ifjsldkj" + }); + + // Assert + Assert.Equal(issuer is not null ? issuer : "http://localhost/", response.Iss); + } + + [Fact] + public async Task ApplyAuthorizationResponse_DoesNotOverrideIssuerSetByApplicationCode() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + options.SetIssuer(new Uri("http://www.fabrikam.com/", UriKind.Absolute)); + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) + .SetClaim(Claims.Subject, "Bob le Magnifique"); + + return default; + })); + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Response.Iss = "http://www.contoso.com/"; + + return default; + })); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest + { + ClientId = "Fabrikam", + RedirectUri = "http://www.fabrikam.com/path", + ResponseType = ResponseTypes.Code, + State = "af0ifjsldkj" + }); + + // Assert + Assert.Equal("http://www.contoso.com/", response.Iss); + } } diff --git a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.cs b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.cs index d6bd375e..46d5a1d4 100644 --- a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.cs +++ b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.cs @@ -1201,7 +1201,12 @@ public abstract partial class OpenIddictServerIntegrationTests }); // Assert - Assert.Equal(0, response.Count); + Assert.Null(response.AccessToken); + Assert.Null(response.Code); + Assert.Null(response.DeviceCode); + Assert.Null(response.IdToken); + Assert.Null(response.RefreshToken); + Assert.Null(response.UserCode); } [Theory]