From 5ac0f28fffe9706c92260fbac06ed0f2158cd15d Mon Sep 17 00:00:00 2001 From: Taloth Saldono Date: Thu, 7 Dec 2017 19:44:27 +0100 Subject: [PATCH] Fixed: Security Vulnerabilities allowing authentication to be bypassed (discovered by Kyle Neideck) --- .../Pipelines/CacheHeaderPipeline.cs | 4 +- .../Extensions/Pipelines/CorsPipeline.cs | 62 ++++++++++++++----- .../Extensions/Pipelines/GZipPipeline.cs | 5 +- .../Extensions/RequestExtensions.cs | 6 ++ .../Frontend/Mappers/StaticResourceMapper.cs | 7 ++- src/NzbDrone.Integration.Test/CorsFixture.cs | 56 +++++++++++++---- 6 files changed, 110 insertions(+), 30 deletions(-) diff --git a/src/NzbDrone.Api/Extensions/Pipelines/CacheHeaderPipeline.cs b/src/NzbDrone.Api/Extensions/Pipelines/CacheHeaderPipeline.cs index 94c738d9b..d8e9266ad 100644 --- a/src/NzbDrone.Api/Extensions/Pipelines/CacheHeaderPipeline.cs +++ b/src/NzbDrone.Api/Extensions/Pipelines/CacheHeaderPipeline.cs @@ -23,6 +23,8 @@ public void Register(IPipelines pipelines) private void Handle(NancyContext context) { + if (context.Request.Method == "OPTIONS") return; + if (_cacheableSpecification.IsCacheable(context)) { context.Response.Headers.EnableCache(); @@ -33,4 +35,4 @@ private void Handle(NancyContext context) } } } -} \ No newline at end of file +} diff --git a/src/NzbDrone.Api/Extensions/Pipelines/CorsPipeline.cs b/src/NzbDrone.Api/Extensions/Pipelines/CorsPipeline.cs index b8c83298a..b63c2f19b 100644 --- a/src/NzbDrone.Api/Extensions/Pipelines/CorsPipeline.cs +++ b/src/NzbDrone.Api/Extensions/Pipelines/CorsPipeline.cs @@ -2,6 +2,7 @@ using System.Linq; using Nancy; using Nancy.Bootstrapper; +using NzbDrone.Common.Extensions; namespace NzbDrone.Api.Extensions.Pipelines { @@ -11,10 +12,25 @@ public class CorsPipeline : IRegisterNancyPipeline 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 @@ private void Handle(NancyContext context) 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 = string.Join(", ", request.Headers[AccessControlHeaders.RequestHeaders]); + + response.Headers.Add(AccessControlHeaders.AllowHeaders, requestedHeaders); + } } } } diff --git a/src/NzbDrone.Api/Extensions/Pipelines/GZipPipeline.cs b/src/NzbDrone.Api/Extensions/Pipelines/GZipPipeline.cs index 12293f23c..8aa9f4ad2 100644 --- a/src/NzbDrone.Api/Extensions/Pipelines/GZipPipeline.cs +++ b/src/NzbDrone.Api/Extensions/Pipelines/GZipPipeline.cs @@ -33,7 +33,8 @@ private void CompressResponse(NancyContext context) 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) @@ -80,4 +81,4 @@ private static bool AlreadyGzipEncoded(Response response) return false; } } -} \ No newline at end of file +} diff --git a/src/NzbDrone.Api/Extensions/RequestExtensions.cs b/src/NzbDrone.Api/Extensions/RequestExtensions.cs index 6c112c900..925070036 100644 --- a/src/NzbDrone.Api/Extensions/RequestExtensions.cs +++ b/src/NzbDrone.Api/Extensions/RequestExtensions.cs @@ -36,5 +36,11 @@ public static bool IsContentRequest(this Request request) { return request.Path.StartsWith("/Content/", StringComparison.InvariantCultureIgnoreCase); } + + 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/NzbDrone.Api/Frontend/Mappers/StaticResourceMapper.cs b/src/NzbDrone.Api/Frontend/Mappers/StaticResourceMapper.cs index 61ed14e9b..4b3b939a1 100644 --- a/src/NzbDrone.Api/Frontend/Mappers/StaticResourceMapper.cs +++ b/src/NzbDrone.Api/Frontend/Mappers/StaticResourceMapper.cs @@ -1,3 +1,4 @@ +using System; using System.IO; using NLog; using NzbDrone.Common.Disk; @@ -28,7 +29,9 @@ public override string Map(string resourceUrl) public override bool CanHandle(string resourceUrl) { - return resourceUrl.StartsWith("/Content") || + resourceUrl = resourceUrl.ToLowerInvariant(); + + return resourceUrl.StartsWith("/content") || resourceUrl.EndsWith(".js") || resourceUrl.EndsWith(".map") || resourceUrl.EndsWith(".css") || @@ -37,4 +40,4 @@ public override bool CanHandle(string resourceUrl) 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 2d9d8ac4f..a37936a2a 100644 --- a/src/NzbDrone.Integration.Test/CorsFixture.cs +++ b/src/NzbDrone.Integration.Test/CorsFixture.cs @@ -8,30 +8,37 @@ namespace NzbDrone.Integration.Test [TestFixture] public class CorsFixture : IntegrationTest { - private RestRequest BuildRequest() + private RestRequest BuildGet(string route = "series") { - var request = new RestRequest("series"); + var request = new RestRequest(route, Method.GET); request.AddHeader(AccessControlHeaders.RequestMethod, "POST"); return request; } + private RestRequest BuildOptions(string route = "series") + { + 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); } [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 @@ public void should_have_allow_headers_in_response_when_included_in_the_request() [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 @@ public void should_have_allow_origin_in_response() [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); + } } }