From 37137b0c10b322a393cbf1f6bbd7e02ad00bfccd Mon Sep 17 00:00:00 2001 From: Mark McDowall Date: Thu, 28 Mar 2019 19:20:40 -0700 Subject: [PATCH] HTTPS certificate validation options New: Enable HTTPS certificate validation by default New: Option to disable certificate validation for all or only local addresses --- .../src/Settings/General/SecuritySettings.js | 34 +++++++-- .../Config/HostConfigResource.cs | 3 + .../IPAddressExtensionsFixture.cs | 30 ++++++++ .../Extensions/IpAddressExtensions.cs | 29 ++++++++ .../Http/Dispatchers/ManagedHttpDispatcher.cs | 6 -- .../Security/SecurityProtocolPolicy.cs | 66 ----------------- .../X509CertificateValidationPolicy.cs | 44 ------------ .../Configuration/ConfigService.cs | 4 ++ .../Configuration/IConfigService.cs | 2 + src/NzbDrone.Core/{Security.cs => Hashing.cs} | 2 +- .../Security/CertificateValidationType.cs | 9 +++ .../X509CertificateValidationPolicy.cs | 72 +++++++++++++++++++ src/NzbDrone.Host/Bootstrap.cs | 6 +- src/NzbDrone.Update/UpdateApp.cs | 4 -- 14 files changed, 179 insertions(+), 132 deletions(-) create mode 100644 src/NzbDrone.Common.Test/ExtensionTests/IPAddressExtensionsFixture.cs create mode 100644 src/NzbDrone.Common/Extensions/IpAddressExtensions.cs delete mode 100644 src/NzbDrone.Common/Security/SecurityProtocolPolicy.cs delete mode 100644 src/NzbDrone.Common/Security/X509CertificateValidationPolicy.cs rename src/NzbDrone.Core/{Security.cs => Hashing.cs} (96%) create mode 100644 src/NzbDrone.Core/Security/CertificateValidationType.cs create mode 100644 src/NzbDrone.Core/Security/X509CertificateValidationPolicy.cs diff --git a/frontend/src/Settings/General/SecuritySettings.js b/frontend/src/Settings/General/SecuritySettings.js index 82ed39d0c..d7a2009a8 100644 --- a/frontend/src/Settings/General/SecuritySettings.js +++ b/frontend/src/Settings/General/SecuritySettings.js @@ -10,6 +10,18 @@ import FormInputGroup from 'Components/Form/FormInputGroup'; import FormInputButton from 'Components/Form/FormInputButton'; import ConfirmModal from 'Components/Modal/ConfirmModal'; +const authenticationMethodOptions = [ + { key: 'none', value: 'None' }, + { key: 'basic', value: 'Basic (Browser Popup)' }, + { key: 'forms', value: 'Forms (Login Page)' } +]; + +const certificateValidationOptions = [ + { key: 'enabled', value: 'Enabled' }, + { key: 'disabledForLocalAddresses', value: 'Disabled for Local Addresses' }, + { key: 'disabled', value: 'Disabled' } +]; + class SecuritySettings extends Component { // @@ -57,15 +69,10 @@ class SecuritySettings extends Component { authenticationMethod, username, password, - apiKey + apiKey, + certificateValidation } = settings; - const authenticationMethodOptions = [ - { key: 'none', value: 'None' }, - { key: 'basic', value: 'Basic (Browser Popup)' }, - { key: 'forms', value: 'Forms (Login Page)' } - ]; - const authenticationEnabled = authenticationMethod && authenticationMethod.value !== 'none'; return ( @@ -146,6 +153,19 @@ class SecuritySettings extends Component { /> + + Certificate Validation + + + + = 16; + case 192: + return bytes[1] == 168; + default: + return false; + } + } + } +} diff --git a/src/NzbDrone.Common/Http/Dispatchers/ManagedHttpDispatcher.cs b/src/NzbDrone.Common/Http/Dispatchers/ManagedHttpDispatcher.cs index f32ac8845..7cd8e6d9b 100644 --- a/src/NzbDrone.Common/Http/Dispatchers/ManagedHttpDispatcher.cs +++ b/src/NzbDrone.Common/Http/Dispatchers/ManagedHttpDispatcher.cs @@ -9,7 +9,6 @@ using NzbDrone.Common.EnvironmentInfo; using NzbDrone.Common.Extensions; using NzbDrone.Common.Http.Proxy; using NzbDrone.Common.Instrumentation.Extensions; -using NzbDrone.Common.Security; namespace NzbDrone.Common.Http.Dispatchers { @@ -83,11 +82,6 @@ namespace NzbDrone.Common.Http.Dispatchers } catch (WebException e) { - if (e.Status == WebExceptionStatus.SecureChannelFailure && OsInfo.IsWindows) - { - SecurityProtocolPolicy.DisableTls12(); - } - httpWebResponse = (HttpWebResponse)e.Response; if (httpWebResponse == null) diff --git a/src/NzbDrone.Common/Security/SecurityProtocolPolicy.cs b/src/NzbDrone.Common/Security/SecurityProtocolPolicy.cs deleted file mode 100644 index c0ebc56eb..000000000 --- a/src/NzbDrone.Common/Security/SecurityProtocolPolicy.cs +++ /dev/null @@ -1,66 +0,0 @@ -using System; -using System.Net; -using NLog; -using NzbDrone.Common.EnvironmentInfo; -using NzbDrone.Common.Instrumentation; - -namespace NzbDrone.Common.Security -{ - public static class SecurityProtocolPolicy - { - private static readonly Logger Logger = NzbDroneLogger.GetLogger(typeof(SecurityProtocolPolicy)); - - private const SecurityProtocolType Tls11 = (SecurityProtocolType)768; - private const SecurityProtocolType Tls12 = (SecurityProtocolType)3072; - - public static void Register() - { - if (OsInfo.IsNotWindows) - { - // This was never meant to be used on mono, and will cause issues with mono 5 and higher if btls is enabled. - return; - } - - try - { - // TODO: In v3 we should drop support for SSL3 because its very insecure. Only leaving it enabled because some people might rely on it. - var protocol = SecurityProtocolType.Ssl3 | SecurityProtocolType.Tls; - - if (Enum.IsDefined(typeof(SecurityProtocolType), Tls11)) - { - protocol |= Tls11; - } - - // Enabling Tls1.2 invalidates certificates using md5, so we disable Tls12 on the fly if that happens. - if (Enum.IsDefined(typeof(SecurityProtocolType), Tls12)) - { - protocol |= Tls12; - } - - ServicePointManager.SecurityProtocol = protocol; - } - catch (Exception ex) - { - Logger.Debug(ex, "Failed to set TLS security protocol."); - } - } - - public static void DisableTls12() - { - try - { - var protocol = ServicePointManager.SecurityProtocol; - if (protocol.HasFlag(Tls12)) - { - Logger.Warn("Disabled Tls1.2 due to remote certificate error."); - - ServicePointManager.SecurityProtocol = protocol & ~Tls12; - } - } - catch (Exception ex) - { - Logger.Debug(ex, "Failed to disable TLS 1.2 security protocol."); - } - } - } -} diff --git a/src/NzbDrone.Common/Security/X509CertificateValidationPolicy.cs b/src/NzbDrone.Common/Security/X509CertificateValidationPolicy.cs deleted file mode 100644 index 1ef25694e..000000000 --- a/src/NzbDrone.Common/Security/X509CertificateValidationPolicy.cs +++ /dev/null @@ -1,44 +0,0 @@ -using System.Net; -using System.Net.Security; -using System.Security.Cryptography.X509Certificates; -using NLog; -using NzbDrone.Common.Instrumentation; - -namespace NzbDrone.Common.Security -{ - public static class X509CertificateValidationPolicy - { - private static readonly Logger Logger = NzbDroneLogger.GetLogger(typeof(X509CertificateValidationPolicy)); - - public static void Register() - { - ServicePointManager.ServerCertificateValidationCallback = ShouldByPassValidationError; - } - - private static bool ShouldByPassValidationError(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors) - { - var request = sender as HttpWebRequest; - - if (request == null) - { - return true; - } - - var req = sender as HttpWebRequest; - var cert2 = certificate as X509Certificate2; - if (cert2 != null && req != null && cert2.SignatureAlgorithm.FriendlyName == "md5RSA") - { - Logger.Error("https://{0} uses the obsolete md5 hash in it's https certificate, if that is your certificate, please (re)create certificate with better algorithm as soon as possible.", req.RequestUri.Authority); - } - - if (sslPolicyErrors == SslPolicyErrors.None) - { - return true; - } - - Logger.Debug("Certificate validation for {0} failed. {1}", request.Address, sslPolicyErrors); - - return true; - } - } -} diff --git a/src/NzbDrone.Core/Configuration/ConfigService.cs b/src/NzbDrone.Core/Configuration/ConfigService.cs index 0043d3cb3..cbed9c7e1 100644 --- a/src/NzbDrone.Core/Configuration/ConfigService.cs +++ b/src/NzbDrone.Core/Configuration/ConfigService.cs @@ -9,6 +9,7 @@ using NzbDrone.Core.MediaFiles; using NzbDrone.Core.Messaging.Events; using NzbDrone.Common.Http.Proxy; using NzbDrone.Core.Qualities; +using NzbDrone.Core.Security; namespace NzbDrone.Core.Configuration { @@ -409,6 +410,9 @@ namespace NzbDrone.Core.Configuration public int BackupRetention => GetValueInt("BackupRetention", 28); + public CertificateValidationType CertificateValidation => + GetValueEnum("CertificateValidation", CertificateValidationType.Enabled); + private string GetValue(string key) { return GetValue(key, string.Empty); diff --git a/src/NzbDrone.Core/Configuration/IConfigService.cs b/src/NzbDrone.Core/Configuration/IConfigService.cs index fc03d5344..7720e2e00 100644 --- a/src/NzbDrone.Core/Configuration/IConfigService.cs +++ b/src/NzbDrone.Core/Configuration/IConfigService.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using NzbDrone.Core.MediaFiles; using NzbDrone.Common.Http.Proxy; using NzbDrone.Core.Qualities; +using NzbDrone.Core.Security; namespace NzbDrone.Core.Configuration { @@ -97,5 +98,6 @@ namespace NzbDrone.Core.Configuration int BackupInterval { get; } int BackupRetention { get; } + CertificateValidationType CertificateValidation { get; } } } diff --git a/src/NzbDrone.Core/Security.cs b/src/NzbDrone.Core/Hashing.cs similarity index 96% rename from src/NzbDrone.Core/Security.cs rename to src/NzbDrone.Core/Hashing.cs index 840466e98..bf7b9a52b 100644 --- a/src/NzbDrone.Core/Security.cs +++ b/src/NzbDrone.Core/Hashing.cs @@ -4,7 +4,7 @@ using System.Text; namespace NzbDrone.Core { - public static class Security + public static class Hashing { public static string SHA256Hash(this string input) { diff --git a/src/NzbDrone.Core/Security/CertificateValidationType.cs b/src/NzbDrone.Core/Security/CertificateValidationType.cs new file mode 100644 index 000000000..affb2c040 --- /dev/null +++ b/src/NzbDrone.Core/Security/CertificateValidationType.cs @@ -0,0 +1,9 @@ +namespace NzbDrone.Core.Security +{ + public enum CertificateValidationType + { + Enabled = 0, + DisabledForLocalAddresses = 1, + Disabled = 2 + } +} diff --git a/src/NzbDrone.Core/Security/X509CertificateValidationPolicy.cs b/src/NzbDrone.Core/Security/X509CertificateValidationPolicy.cs new file mode 100644 index 000000000..2dea7f7f0 --- /dev/null +++ b/src/NzbDrone.Core/Security/X509CertificateValidationPolicy.cs @@ -0,0 +1,72 @@ +using System.Linq; +using System.Net; +using System.Net.Security; +using System.Security.Cryptography.X509Certificates; +using NLog; +using NzbDrone.Common.Extensions; +using NzbDrone.Core.Configuration; + +namespace NzbDrone.Core.Security +{ + public interface IX509CertificateValidationPolicy + { + void Register(); + } + + public class X509CertificateValidationPolicy : IX509CertificateValidationPolicy + { + private readonly IConfigService _configService; + private readonly Logger _logger; + + public X509CertificateValidationPolicy(IConfigService configService, Logger logger) + { + _configService = configService; + _logger = logger; + } + + public void Register() + { + ServicePointManager.ServerCertificateValidationCallback = ShouldByPassValidationError; + } + + private bool ShouldByPassValidationError(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors) + { + var request = sender as HttpWebRequest; + + if (request == null) + { + return true; + } + + var req = sender as HttpWebRequest; + var cert2 = certificate as X509Certificate2; + if (cert2 != null && req != null && cert2.SignatureAlgorithm.FriendlyName == "md5RSA") + { + _logger.Error("https://{0} uses the obsolete md5 hash in it's https certificate, if that is your certificate, please (re)create certificate with better algorithm as soon as possible.", req.RequestUri.Authority); + } + + if (sslPolicyErrors == SslPolicyErrors.None) + { + return true; + } + + var host = Dns.GetHostEntry(req.Host); + var certificateValidation = _configService.CertificateValidation; + + if (certificateValidation == CertificateValidationType.Disabled) + { + return true; + } + + if (certificateValidation == CertificateValidationType.DisabledForLocalAddresses && host.AddressList.All(i => i.IsLocalAddress())) + { + return true; + } + + + _logger.Error("Certificate validation for {0} failed. {1}", request.Address, sslPolicyErrors); + + return false; + } + } +} diff --git a/src/NzbDrone.Host/Bootstrap.cs b/src/NzbDrone.Host/Bootstrap.cs index b3fa8ca99..d894c359b 100644 --- a/src/NzbDrone.Host/Bootstrap.cs +++ b/src/NzbDrone.Host/Bootstrap.cs @@ -8,9 +8,9 @@ using NzbDrone.Common.EnvironmentInfo; using NzbDrone.Common.Exceptions; using NzbDrone.Common.Instrumentation; using NzbDrone.Common.Processes; -using NzbDrone.Common.Security; using NzbDrone.Core.Configuration; using NzbDrone.Core.Instrumentation; +using NzbDrone.Core.Security; namespace NzbDrone.Host { @@ -23,9 +23,6 @@ namespace NzbDrone.Host { try { - SecurityProtocolPolicy.Register(); - X509CertificateValidationPolicy.Register(); - Logger.Info("Starting Lidarr - {0} - Version {1}", Assembly.GetCallingAssembly().Location, Assembly.GetExecutingAssembly().GetName().Version); if (!PlatformValidation.IsValidate(userAlert)) @@ -43,6 +40,7 @@ namespace NzbDrone.Host var appMode = GetApplicationMode(startupContext); Start(appMode, startupContext); + _container.Resolve().Register(); if (startCallback != null) { diff --git a/src/NzbDrone.Update/UpdateApp.cs b/src/NzbDrone.Update/UpdateApp.cs index 25bb5ab63..67aa21b71 100644 --- a/src/NzbDrone.Update/UpdateApp.cs +++ b/src/NzbDrone.Update/UpdateApp.cs @@ -7,7 +7,6 @@ using NzbDrone.Common.EnvironmentInfo; using NzbDrone.Common.Extensions; using NzbDrone.Common.Instrumentation; using NzbDrone.Common.Processes; -using NzbDrone.Common.Security; using NzbDrone.Update.UpdateEngine; namespace NzbDrone.Update @@ -30,9 +29,6 @@ namespace NzbDrone.Update { try { - SecurityProtocolPolicy.Register(); - X509CertificateValidationPolicy.Register(); - var startupContext = new StartupContext(args); NzbDroneLogger.Register(startupContext, true, true);