From 44b5de156886995fdcf881cbc1208505ad0e8b0e Mon Sep 17 00:00:00 2001 From: jade Date: Tue, 3 Jun 2025 14:22:30 -0700 Subject: [PATCH] Fix missing logging of connections by disallowed IPs (#14011) --- .../IpBasedAccessValidationMiddleware.cs | 20 ++++++-- .../Middleware/LanFilteringMiddleware.cs | 51 ------------------- .../ApiApplicationBuilderExtensions.cs | 10 ---- Jellyfin.Server/Startup.cs | 1 - .../Extensions/HttpContextExtensions.cs | 2 +- MediaBrowser.Common/Net/INetworkManager.cs | 4 +- .../Net/RemoteAccessPolicyResult.cs | 29 +++++++++++ .../Manager/NetworkManager.cs | 47 ++++++++++------- .../NetworkParseTests.cs | 43 ++++++++++++---- 9 files changed, 109 insertions(+), 98 deletions(-) delete mode 100644 Jellyfin.Api/Middleware/LanFilteringMiddleware.cs create mode 100644 MediaBrowser.Common/Net/RemoteAccessPolicyResult.cs diff --git a/Jellyfin.Api/Middleware/IpBasedAccessValidationMiddleware.cs b/Jellyfin.Api/Middleware/IpBasedAccessValidationMiddleware.cs index 842a69dd98..a0ed6c8126 100644 --- a/Jellyfin.Api/Middleware/IpBasedAccessValidationMiddleware.cs +++ b/Jellyfin.Api/Middleware/IpBasedAccessValidationMiddleware.cs @@ -1,8 +1,10 @@ using System.Net; using System.Threading.Tasks; +using System.Web; using MediaBrowser.Common.Extensions; using MediaBrowser.Common.Net; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; namespace Jellyfin.Api.Middleware; @@ -12,14 +14,17 @@ namespace Jellyfin.Api.Middleware; public class IPBasedAccessValidationMiddleware { private readonly RequestDelegate _next; + private readonly ILogger _logger; /// /// Initializes a new instance of the class. /// /// The next delegate in the pipeline. - public IPBasedAccessValidationMiddleware(RequestDelegate next) + /// The logger to log to. + public IPBasedAccessValidationMiddleware(RequestDelegate next, ILogger logger) { _next = next; + _logger = logger; } /// @@ -32,16 +37,23 @@ public class IPBasedAccessValidationMiddleware { if (httpContext.IsLocal()) { - // Running locally. + // Accessing from the same machine as the server. await _next(httpContext).ConfigureAwait(false); return; } - var remoteIP = httpContext.Connection.RemoteIpAddress ?? IPAddress.Loopback; + var remoteIP = httpContext.GetNormalizedRemoteIP(); - if (!networkManager.HasRemoteAccess(remoteIP)) + var result = networkManager.ShouldAllowServerAccess(remoteIP); + if (result != RemoteAccessPolicyResult.Allow) { // No access from network, respond with 503 instead of 200. + _logger.LogWarning( + "Blocking request to {Path} by {RemoteIP} due to IP filtering rule, reason: {Reason}", + // url-encode to block log injection + HttpUtility.UrlEncode(httpContext.Request.Path), + remoteIP, + result); httpContext.Response.StatusCode = StatusCodes.Status503ServiceUnavailable; return; } diff --git a/Jellyfin.Api/Middleware/LanFilteringMiddleware.cs b/Jellyfin.Api/Middleware/LanFilteringMiddleware.cs deleted file mode 100644 index 35b0a1dd06..0000000000 --- a/Jellyfin.Api/Middleware/LanFilteringMiddleware.cs +++ /dev/null @@ -1,51 +0,0 @@ -using System.Net; -using System.Threading.Tasks; -using MediaBrowser.Common.Extensions; -using MediaBrowser.Common.Net; -using MediaBrowser.Controller.Configuration; -using Microsoft.AspNetCore.Http; - -namespace Jellyfin.Api.Middleware; - -/// -/// Validates the LAN host IP based on application configuration. -/// -public class LanFilteringMiddleware -{ - private readonly RequestDelegate _next; - - /// - /// Initializes a new instance of the class. - /// - /// The next delegate in the pipeline. - public LanFilteringMiddleware(RequestDelegate next) - { - _next = next; - } - - /// - /// Executes the middleware action. - /// - /// The current HTTP context. - /// The network manager. - /// The server configuration manager. - /// The async task. - public async Task Invoke(HttpContext httpContext, INetworkManager networkManager, IServerConfigurationManager serverConfigurationManager) - { - if (serverConfigurationManager.GetNetworkConfiguration().EnableRemoteAccess) - { - await _next(httpContext).ConfigureAwait(false); - return; - } - - var host = httpContext.GetNormalizedRemoteIP(); - if (!networkManager.IsInLocalNetwork(host)) - { - // No access from network, respond with 503 instead of 200. - httpContext.Response.StatusCode = StatusCodes.Status503ServiceUnavailable; - return; - } - - await _next(httpContext).ConfigureAwait(false); - } -} diff --git a/Jellyfin.Server/Extensions/ApiApplicationBuilderExtensions.cs b/Jellyfin.Server/Extensions/ApiApplicationBuilderExtensions.cs index 6066893de3..a56baba33b 100644 --- a/Jellyfin.Server/Extensions/ApiApplicationBuilderExtensions.cs +++ b/Jellyfin.Server/Extensions/ApiApplicationBuilderExtensions.cs @@ -68,16 +68,6 @@ namespace Jellyfin.Server.Extensions return appBuilder.UseMiddleware(); } - /// - /// Adds LAN based access filtering to the application pipeline. - /// - /// The application builder. - /// The updated application builder. - public static IApplicationBuilder UseLanFiltering(this IApplicationBuilder appBuilder) - { - return appBuilder.UseMiddleware(); - } - /// /// Enables url decoding before binding to the application pipeline. /// diff --git a/Jellyfin.Server/Startup.cs b/Jellyfin.Server/Startup.cs index 688b169359..94ae54a359 100644 --- a/Jellyfin.Server/Startup.cs +++ b/Jellyfin.Server/Startup.cs @@ -208,7 +208,6 @@ namespace Jellyfin.Server mainApp.UseRouting(); mainApp.UseAuthorization(); - mainApp.UseLanFiltering(); mainApp.UseIPBasedAccessValidation(); mainApp.UseWebSocketHandler(); mainApp.UseServerStartupMessage(); diff --git a/MediaBrowser.Common/Extensions/HttpContextExtensions.cs b/MediaBrowser.Common/Extensions/HttpContextExtensions.cs index a1056b7c84..739a53c7ad 100644 --- a/MediaBrowser.Common/Extensions/HttpContextExtensions.cs +++ b/MediaBrowser.Common/Extensions/HttpContextExtensions.cs @@ -12,7 +12,7 @@ namespace MediaBrowser.Common.Extensions /// Checks the origin of the HTTP context. /// /// The incoming HTTP context. - /// true if the request is coming from LAN, false otherwise. + /// true if the request is coming from the same machine as is running the server, false otherwise. public static bool IsLocal(this HttpContext context) { return (context.Connection.LocalIpAddress is null diff --git a/MediaBrowser.Common/Net/INetworkManager.cs b/MediaBrowser.Common/Net/INetworkManager.cs index d838144ff6..bd785bcbc6 100644 --- a/MediaBrowser.Common/Net/INetworkManager.cs +++ b/MediaBrowser.Common/Net/INetworkManager.cs @@ -127,7 +127,7 @@ namespace MediaBrowser.Common.Net /// Checks if has access to the server. /// /// IP address of the client. - /// True if it has access, otherwise false. - bool HasRemoteAccess(IPAddress remoteIP); + /// The result of evaluating the access policy, Allow if it should be allowed. + RemoteAccessPolicyResult ShouldAllowServerAccess(IPAddress remoteIP); } } diff --git a/MediaBrowser.Common/Net/RemoteAccessPolicyResult.cs b/MediaBrowser.Common/Net/RemoteAccessPolicyResult.cs new file mode 100644 index 0000000000..193d37228f --- /dev/null +++ b/MediaBrowser.Common/Net/RemoteAccessPolicyResult.cs @@ -0,0 +1,29 @@ +using System; + +namespace MediaBrowser.Common.Net; + +/// +/// Result of . +/// +public enum RemoteAccessPolicyResult +{ + /// + /// The connection should be allowed. + /// + Allow, + + /// + /// The connection should be rejected since it is not from a local IP and remote access is disabled. + /// + RejectDueToRemoteAccessDisabled, + + /// + /// The connection should be rejected since it is from a blocklisted IP. + /// + RejectDueToIPBlocklist, + + /// + /// The connection should be rejected since it is from a remote IP that is not in the allowlist. + /// + RejectDueToNotAllowlistedRemoteIP, +} diff --git a/src/Jellyfin.Networking/Manager/NetworkManager.cs b/src/Jellyfin.Networking/Manager/NetworkManager.cs index 80a5741df9..126d9f15cf 100644 --- a/src/Jellyfin.Networking/Manager/NetworkManager.cs +++ b/src/Jellyfin.Networking/Manager/NetworkManager.cs @@ -690,33 +690,42 @@ public class NetworkManager : INetworkManager, IDisposable } /// - public bool HasRemoteAccess(IPAddress remoteIP) + public RemoteAccessPolicyResult ShouldAllowServerAccess(IPAddress remoteIP) { var config = _configurationManager.GetNetworkConfiguration(); - if (config.EnableRemoteAccess) + if (IsInLocalNetwork(remoteIP)) { - // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely. - // If left blank, all remote addresses will be allowed. - if (_remoteAddressFilter.Any() && !IsInLocalNetwork(remoteIP)) - { - // remoteAddressFilter is a whitelist or blacklist. - var matches = _remoteAddressFilter.Count(remoteNetwork => NetworkUtils.SubnetContainsAddress(remoteNetwork, remoteIP)); - if ((!config.IsRemoteIPFilterBlacklist && matches > 0) - || (config.IsRemoteIPFilterBlacklist && matches == 0)) - { - return true; - } - - return false; - } + return RemoteAccessPolicyResult.Allow; } - else if (!IsInLocalNetwork(remoteIP)) + + if (!config.EnableRemoteAccess) { // Remote not enabled. So everyone should be LAN. - return false; + return RemoteAccessPolicyResult.RejectDueToRemoteAccessDisabled; } - return true; + if (!_remoteAddressFilter.Any()) + { + // No filter on remote addresses, allow any of them. + return RemoteAccessPolicyResult.Allow; + } + + // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely. + // If left blank, all remote addresses will be allowed. + + // remoteAddressFilter is a whitelist or blacklist. + var anyMatches = _remoteAddressFilter.Any(remoteNetwork => NetworkUtils.SubnetContainsAddress(remoteNetwork, remoteIP)); + if (config.IsRemoteIPFilterBlacklist) + { + return anyMatches + ? RemoteAccessPolicyResult.RejectDueToIPBlocklist + : RemoteAccessPolicyResult.Allow; + } + + // Allow-list + return anyMatches + ? RemoteAccessPolicyResult.Allow + : RemoteAccessPolicyResult.RejectDueToNotAllowlistedRemoteIP; } /// diff --git a/tests/Jellyfin.Networking.Tests/NetworkParseTests.cs b/tests/Jellyfin.Networking.Tests/NetworkParseTests.cs index ef87e46a78..38208476f8 100644 --- a/tests/Jellyfin.Networking.Tests/NetworkParseTests.cs +++ b/tests/Jellyfin.Networking.Tests/NetworkParseTests.cs @@ -281,11 +281,11 @@ namespace Jellyfin.Networking.Tests } [Theory] - [InlineData("185.10.10.10,200.200.200.200", "79.2.3.4", true)] - [InlineData("185.10.10.10", "185.10.10.10", false)] - [InlineData("", "100.100.100.100", false)] + [InlineData("185.10.10.10,200.200.200.200", "79.2.3.4", RemoteAccessPolicyResult.RejectDueToNotAllowlistedRemoteIP)] + [InlineData("185.10.10.10", "185.10.10.10", RemoteAccessPolicyResult.Allow)] + [InlineData("", "100.100.100.100", RemoteAccessPolicyResult.Allow)] - public void HasRemoteAccess_GivenWhitelist_AllowsOnlyIPsInWhitelist(string addresses, string remoteIP, bool denied) + public void HasRemoteAccess_GivenWhitelist_AllowsOnlyIPsInWhitelist(string addresses, string remoteIP, RemoteAccessPolicyResult expectedResult) { // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely. // If left blank, all remote addresses will be allowed. @@ -299,15 +299,38 @@ namespace Jellyfin.Networking.Tests var startupConf = new Mock(); using var nm = new NetworkManager(NetworkParseTests.GetMockConfig(conf), startupConf.Object, new NullLogger()); - Assert.NotEqual(nm.HasRemoteAccess(IPAddress.Parse(remoteIP)), denied); + Assert.Equal(expectedResult, nm.ShouldAllowServerAccess(IPAddress.Parse(remoteIP))); } [Theory] - [InlineData("185.10.10.10", "79.2.3.4", false)] - [InlineData("185.10.10.10", "185.10.10.10", true)] - [InlineData("", "100.100.100.100", false)] + [InlineData("185.10.10.10,200.200.200.200", "79.2.3.4", RemoteAccessPolicyResult.RejectDueToRemoteAccessDisabled)] + [InlineData("185.10.10.10", "127.0.0.1", RemoteAccessPolicyResult.Allow)] + [InlineData("", "100.100.100.100", RemoteAccessPolicyResult.RejectDueToRemoteAccessDisabled)] - public void HasRemoteAccess_GivenBlacklist_BlacklistTheIPs(string addresses, string remoteIP, bool denied) + public void HasRemoteAccess_GivenRemoteAccessDisabled_IgnoresAllowlist(string addresses, string remoteIP, RemoteAccessPolicyResult expectedResult) + { + // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely. + // If left blank, all remote addresses will be allowed. + var conf = new NetworkConfiguration() + { + EnableIPv4 = true, + EnableRemoteAccess = false, + RemoteIPFilter = addresses.Split(','), + IsRemoteIPFilterBlacklist = false + }; + + var startupConf = new Mock(); + using var nm = new NetworkManager(NetworkParseTests.GetMockConfig(conf), startupConf.Object, new NullLogger()); + + Assert.Equal(expectedResult, nm.ShouldAllowServerAccess(IPAddress.Parse(remoteIP))); + } + + [Theory] + [InlineData("185.10.10.10", "79.2.3.4", RemoteAccessPolicyResult.Allow)] + [InlineData("185.10.10.10", "185.10.10.10", RemoteAccessPolicyResult.RejectDueToIPBlocklist)] + [InlineData("", "100.100.100.100", RemoteAccessPolicyResult.Allow)] + + public void HasRemoteAccess_GivenBlacklist_BlacklistTheIPs(string addresses, string remoteIP, RemoteAccessPolicyResult expectedResult) { // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely. // If left blank, all remote addresses will be allowed. @@ -321,7 +344,7 @@ namespace Jellyfin.Networking.Tests var startupConf = new Mock(); using var nm = new NetworkManager(NetworkParseTests.GetMockConfig(conf), startupConf.Object, new NullLogger()); - Assert.NotEqual(nm.HasRemoteAccess(IPAddress.Parse(remoteIP)), denied); + Assert.Equal(expectedResult, nm.ShouldAllowServerAccess(IPAddress.Parse(remoteIP))); } [Theory]