From 2ccc5af8d0e62c71b7c943299f15d1032cd11115 Mon Sep 17 00:00:00 2001 From: Qstick Date: Wed, 13 Dec 2017 21:46:44 -0500 Subject: [PATCH] Fixed: Security Vulnerabilities allowing authentication to be bypass --- .../Pipelines/CacheHeaderPipeline.cs | 6 +- .../Extensions/Pipelines/CorsPipeline.cs | 64 ++++++++++++++----- .../Extensions/Pipelines/GZipPipeline.cs | 7 +- .../Extensions/RequestExtensions.cs | 8 ++- .../Frontend/Mappers/StaticResourceMapper.cs | 12 ++-- src/NzbDrone.Integration.Test/CorsFixture.cs | 56 ++++++++++++---- 6 files changed, 116 insertions(+), 37 deletions(-) diff --git a/src/Lidarr.Http/Extensions/Pipelines/CacheHeaderPipeline.cs b/src/Lidarr.Http/Extensions/Pipelines/CacheHeaderPipeline.cs index febfc17c3..13a1e824e 100644 --- a/src/Lidarr.Http/Extensions/Pipelines/CacheHeaderPipeline.cs +++ b/src/Lidarr.Http/Extensions/Pipelines/CacheHeaderPipeline.cs @@ -1,4 +1,4 @@ -using System; +using System; using Nancy; using Nancy.Bootstrapper; using Lidarr.Http.Frontend; @@ -23,6 +23,8 @@ namespace Lidarr.Http.Extensions.Pipelines private void Handle(NancyContext context) { + if (context.Request.Method == "OPTIONS") return; + if (_cacheableSpecification.IsCacheable(context)) { context.Response.Headers.EnableCache(); @@ -33,4 +35,4 @@ namespace Lidarr.Http.Extensions.Pipelines } } } -} \ No newline at end of file +} diff --git a/src/Lidarr.Http/Extensions/Pipelines/CorsPipeline.cs b/src/Lidarr.Http/Extensions/Pipelines/CorsPipeline.cs index 3f980524f..3e21c3221 100644 --- a/src/Lidarr.Http/Extensions/Pipelines/CorsPipeline.cs +++ b/src/Lidarr.Http/Extensions/Pipelines/CorsPipeline.cs @@ -1,7 +1,8 @@ -using System; +using System; using System.Linq; using Nancy; using Nancy.Bootstrapper; +using NzbDrone.Common.Extensions; namespace Lidarr.Http.Extensions.Pipelines { @@ -11,10 +12,25 @@ namespace Lidarr.Http.Extensions.Pipelines public void Register(IPipelines pipelines) { - pipelines.AfterRequest.AddItemToEndOfPipeline((Action) Handle); + pipelines.BeforeRequest.AddItemToEndOfPipeline(HandleRequest); + pipelines.AfterRequest.AddItemToEndOfPipeline(HandleResponse); } - private void Handle(NancyContext context) + private Response HandleRequest(NancyContext context) + { + if (context == null || context.Request.Method != "OPTIONS") + { + return null; + } + + var response = new Response() + .WithStatusCode(HttpStatusCode.OK) + .WithContentType(""); + ApplyResponseHeaders(response, context.Request); + return response; + } + + private void HandleResponse(NancyContext context) { if (context == null || context.Response.Headers.ContainsKey(AccessControlHeaders.AllowOrigin)) { @@ -26,21 +42,39 @@ namespace Lidarr.Http.Extensions.Pipelines private static void ApplyResponseHeaders(Response response, Request request) { - var allowedMethods = "GET, OPTIONS, PATCH, POST, PUT, DELETE"; - - if (response.Headers.ContainsKey("Allow")) + if (request.IsApiRequest()) { - allowedMethods = response.Headers["Allow"]; + // Allow Cross-Origin access to the API since it's protected with the apikey, and nothing else. + ApplyCorsResponseHeaders(response, request, "*", "GET, OPTIONS, PATCH, POST, PUT, DELETE"); } - - var requestedHeaders = string.Join(", ", request.Headers[AccessControlHeaders.RequestHeaders]); - - response.Headers.Add(AccessControlHeaders.AllowOrigin, "*"); - response.Headers.Add(AccessControlHeaders.AllowMethods, allowedMethods); - - if (request.Headers[AccessControlHeaders.RequestHeaders].Any()) + else if (request.IsSharedContentRequest()) { - response.Headers.Add(AccessControlHeaders.AllowHeaders, requestedHeaders); + // Allow Cross-Origin access to specific shared content such as mediacovers and images. + ApplyCorsResponseHeaders(response, request, "*", "GET, OPTIONS"); + } + + // Disallow Cross-Origin access for any other route. + } + + private static void ApplyCorsResponseHeaders(Response response, Request request, string allowOrigin, string allowedMethods) + { + response.Headers.Add(AccessControlHeaders.AllowOrigin, allowOrigin); + + if (request.Method == "OPTIONS") + { + if (response.Headers.ContainsKey("Allow")) + { + allowedMethods = response.Headers["Allow"]; + } + + response.Headers.Add(AccessControlHeaders.AllowMethods, allowedMethods); + + if (request.Headers[AccessControlHeaders.RequestHeaders].Any()) + { + var requestedHeaders = request.Headers[AccessControlHeaders.RequestHeaders].Join(", "); + + response.Headers.Add(AccessControlHeaders.AllowHeaders, requestedHeaders); + } } } } diff --git a/src/Lidarr.Http/Extensions/Pipelines/GZipPipeline.cs b/src/Lidarr.Http/Extensions/Pipelines/GZipPipeline.cs index 782dd5b15..2a31a711a 100644 --- a/src/Lidarr.Http/Extensions/Pipelines/GZipPipeline.cs +++ b/src/Lidarr.Http/Extensions/Pipelines/GZipPipeline.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.IO; using System.IO.Compression; using System.Linq; @@ -33,7 +33,8 @@ namespace Lidarr.Http.Extensions.Pipelines try { if ( - !response.ContentType.Contains("image") + response.Contents != Response.NoBody + && !response.ContentType.Contains("image") && !response.ContentType.Contains("font") && request.Headers.AcceptEncoding.Any(x => x.Contains("gzip")) && !AlreadyGzipEncoded(response) @@ -84,4 +85,4 @@ namespace Lidarr.Http.Extensions.Pipelines return false; } } -} \ No newline at end of file +} diff --git a/src/Lidarr.Http/Extensions/RequestExtensions.cs b/src/Lidarr.Http/Extensions/RequestExtensions.cs index d9dc691a1..08ac6867c 100644 --- a/src/Lidarr.Http/Extensions/RequestExtensions.cs +++ b/src/Lidarr.Http/Extensions/RequestExtensions.cs @@ -1,4 +1,4 @@ -using System; +using System; using Nancy; namespace Lidarr.Http.Extensions @@ -48,5 +48,11 @@ namespace Lidarr.Http.Extensions return defaultValue; } + + public static bool IsSharedContentRequest(this Request request) + { + return request.Path.StartsWith("/MediaCover/", StringComparison.InvariantCultureIgnoreCase) || + request.Path.StartsWith("/Content/Images/", StringComparison.InvariantCultureIgnoreCase); + } } } diff --git a/src/Lidarr.Http/Frontend/Mappers/StaticResourceMapper.cs b/src/Lidarr.Http/Frontend/Mappers/StaticResourceMapper.cs index 32518a935..0799ef5d4 100644 --- a/src/Lidarr.Http/Frontend/Mappers/StaticResourceMapper.cs +++ b/src/Lidarr.Http/Frontend/Mappers/StaticResourceMapper.cs @@ -1,4 +1,4 @@ -using System.IO; +using System.IO; using NLog; using NzbDrone.Common.Disk; using NzbDrone.Common.EnvironmentInfo; @@ -28,13 +28,15 @@ namespace Lidarr.Http.Frontend.Mappers public override bool CanHandle(string resourceUrl) { - if (resourceUrl.StartsWith("/Content/Images/Icons/manifest") || - resourceUrl.StartsWith("/Content/Images/Icons/browserconfig")) + resourceUrl = resourceUrl.ToLowerInvariant(); + + if (resourceUrl.StartsWith("/content/images/icons/manifest") || + resourceUrl.StartsWith("/content/images/icons/browserconfig")) { return false; } - return resourceUrl.StartsWith("/Content") || + return resourceUrl.StartsWith("/content") || resourceUrl.EndsWith(".js") || resourceUrl.EndsWith(".map") || resourceUrl.EndsWith(".css") || @@ -43,4 +45,4 @@ namespace Lidarr.Http.Frontend.Mappers resourceUrl.EndsWith("oauth.html"); } } -} \ No newline at end of file +} diff --git a/src/NzbDrone.Integration.Test/CorsFixture.cs b/src/NzbDrone.Integration.Test/CorsFixture.cs index d27539ba8..6b0a7bc70 100644 --- a/src/NzbDrone.Integration.Test/CorsFixture.cs +++ b/src/NzbDrone.Integration.Test/CorsFixture.cs @@ -8,19 +8,26 @@ namespace NzbDrone.Integration.Test [TestFixture] public class CorsFixture : IntegrationTest { - private RestRequest BuildRequest() + private RestRequest BuildGet(string route = "artist") { - var request = new RestRequest("artist"); + var request = new RestRequest(route, Method.GET); request.AddHeader(AccessControlHeaders.RequestMethod, "POST"); return request; } - [Test] + private RestRequest BuildOptions(string route = "artist") + { + var request = new RestRequest(route, Method.OPTIONS); + + return request; + } + + [Test] public void should_not_have_allow_headers_in_response_when_not_included_in_the_request() { - var request = BuildRequest(); - var response = RestClient.Get(request); + var request = BuildOptions(); + var response = RestClient.Execute(request); response.Headers.Should().NotContain(h => h.Name == AccessControlHeaders.AllowHeaders); } @@ -28,10 +35,10 @@ namespace NzbDrone.Integration.Test [Test] public void should_have_allow_headers_in_response_when_included_in_the_request() { - var request = BuildRequest(); + var request = BuildOptions(); request.AddHeader(AccessControlHeaders.RequestHeaders, "X-Test"); - var response = RestClient.Get(request); + var response = RestClient.Execute(request); response.Headers.Should().Contain(h => h.Name == AccessControlHeaders.AllowHeaders); } @@ -39,8 +46,8 @@ namespace NzbDrone.Integration.Test [Test] public void should_have_allow_origin_in_response() { - var request = BuildRequest(); - var response = RestClient.Get(request); + var request = BuildOptions(); + var response = RestClient.Execute(request); response.Headers.Should().Contain(h => h.Name == AccessControlHeaders.AllowOrigin); } @@ -48,10 +55,37 @@ namespace NzbDrone.Integration.Test [Test] public void should_have_allow_methods_in_response() { - var request = BuildRequest(); - var response = RestClient.Get(request); + var request = BuildOptions(); + var response = RestClient.Execute(request); response.Headers.Should().Contain(h => h.Name == AccessControlHeaders.AllowMethods); } + + [Test] + public void should_not_have_allow_methods_in_non_options_request() + { + var request = BuildGet(); + var response = RestClient.Execute(request); + + response.Headers.Should().NotContain(h => h.Name == AccessControlHeaders.AllowMethods); + } + + [Test] + public void should_have_allow_origin_in_non_options_request() + { + var request = BuildGet(); + var response = RestClient.Execute(request); + + response.Headers.Should().Contain(h => h.Name == AccessControlHeaders.AllowOrigin); + } + + [Test] + public void should_not_have_allow_origin_in_non_api_request() + { + var request = BuildGet("../abc"); + var response = RestClient.Execute(request); + + response.Headers.Should().NotContain(h => h.Name == AccessControlHeaders.AllowOrigin); + } } }