From 1649f7175016390ad6d4ca5e33907e7a18ebcc66 Mon Sep 17 00:00:00 2001 From: Richard Ross Date: Mon, 14 Aug 2017 10:56:57 -0700 Subject: [PATCH] Deprecate SSL pinning and trust chain verification. Oh boy. Here's a controversial change. ![](http://i.imgur.com/t8JjQix.gif) Let's give a bit of backstory. A few weeks ago, Facebook was contacted by a whitehat hacker (the good guys) about a security vulnerability here in SocketRocket. For those of you who are truly interested in what that security flaw was, it is essentially the same flaw as outlined here: https://www.synopsys.com/blogs/software-security/ineffective-certificate-pinning-implementations/ So, we were faced with a choice - quietly push out a patch, and hope that eventually existing applications updated, or be transparent and admit we screwed up. This is us admititng we screwed up. And while yes, we could probably fix the implementation. But we talked internally, and decided that the best approach here is to completely remove the option for pinning. For all of our existing users that use certificate pinning, while we understand that in the past there has been a very large barrier to entry with getting a CA to issue a certificate. However, since the rollout of CAs like LetsEncrypt, there's become an ever-dwindling reason to actually use self-signed or unsigned certificates. For this reason, we're going to go ahead and deprecate the APIs that allow SSL pinning and disabling trust chain verification. The pinning APIs are now going to throw an exception when invoked, and the trust chain APIs have deprecation warnings. If you are a user of these APIs, and you for some reason **CANNOT** use a trust chain validated certificate, PLEASE speak up. While we cannot think of any reason to use those kinds of certificates, it's entirely possible we overlooked something. We'll leave this pullrequest unmerged for a two week period (Monday, August 28th, 2017), at which point, unless we have feedback convincing us otherwise, we will go ahead with this change. --- .../Security/SRPinningSecurityPolicy.h | 5 ++++ .../Security/SRPinningSecurityPolicy.m | 6 +++++ SocketRocket/NSURLRequest+SRWebSocket.h | 8 ++++-- SocketRocket/NSURLRequest+SRWebSocket.m | 11 +++----- SocketRocket/SRSecurityPolicy.h | 9 +++++-- SocketRocket/SRSecurityPolicy.m | 11 +++++++- SocketRocket/SRWebSocket.h | 8 ++++-- SocketRocket/SRWebSocket.m | 25 +++++++++++++------ 8 files changed, 62 insertions(+), 21 deletions(-) diff --git a/SocketRocket/Internal/Security/SRPinningSecurityPolicy.h b/SocketRocket/Internal/Security/SRPinningSecurityPolicy.h index 6ab5e1e96..9b387317c 100644 --- a/SocketRocket/Internal/Security/SRPinningSecurityPolicy.h +++ b/SocketRocket/Internal/Security/SRPinningSecurityPolicy.h @@ -13,6 +13,11 @@ NS_ASSUME_NONNULL_BEGIN +/** + * NOTE: While publicly, SocketRocket does not support configuring the security policy with pinned certificates, + * it is still possible to manually construct a security policy of this class. If you do this, note that you may + * be open to MitM attacks, and we will not support any issues you may have. Dive at your own risk. + */ @interface SRPinningSecurityPolicy : SRSecurityPolicy - (instancetype)initWithCertificates:(NSArray *)pinnedCertificates; diff --git a/SocketRocket/Internal/Security/SRPinningSecurityPolicy.m b/SocketRocket/Internal/Security/SRPinningSecurityPolicy.m index 0eecd2bed..0074ed8b6 100644 --- a/SocketRocket/Internal/Security/SRPinningSecurityPolicy.m +++ b/SocketRocket/Internal/Security/SRPinningSecurityPolicy.m @@ -25,8 +25,14 @@ @implementation SRPinningSecurityPolicy - (instancetype)initWithCertificates:(NSArray *)pinnedCertificates { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated" + // Do not validate certificate chain since we're pinning to specific certificates. self = [super initWithCertificateChainValidationEnabled:NO]; + +#pragma clang diagnostic pop + if (!self) { return self; } if (pinnedCertificates.count == 0) { diff --git a/SocketRocket/NSURLRequest+SRWebSocket.h b/SocketRocket/NSURLRequest+SRWebSocket.h index 32d78da36..4ccffd513 100644 --- a/SocketRocket/NSURLRequest+SRWebSocket.h +++ b/SocketRocket/NSURLRequest+SRWebSocket.h @@ -18,7 +18,9 @@ NS_ASSUME_NONNULL_BEGIN /** An array of pinned `SecCertificateRef` SSL certificates that `SRWebSocket` will use for validation. */ -@property (nullable, nonatomic, copy, readonly) NSArray *SR_SSLPinnedCertificates; +@property (nullable, nonatomic, copy, readonly) NSArray *SR_SSLPinnedCertificates + DEPRECATED_MSG_ATTRIBUTE("Using pinned certificates is neither secure nor supported in SocketRocket, " + "and leads to security issues. Please use a proper, trust chain validated certificate."); @end @@ -27,7 +29,9 @@ NS_ASSUME_NONNULL_BEGIN /** An array of pinned `SecCertificateRef` SSL certificates that `SRWebSocket` will use for validation. */ -@property (nullable, nonatomic, copy) NSArray *SR_SSLPinnedCertificates; +@property (nullable, nonatomic, copy) NSArray *SR_SSLPinnedCertificates + DEPRECATED_MSG_ATTRIBUTE("Using pinned certificates is neither secure nor supported in SocketRocket, " + "and leads to security issues. Please use a proper, trust chain validated certificate."); @end diff --git a/SocketRocket/NSURLRequest+SRWebSocket.m b/SocketRocket/NSURLRequest+SRWebSocket.m index 27f87434e..a26fc3c15 100644 --- a/SocketRocket/NSURLRequest+SRWebSocket.m +++ b/SocketRocket/NSURLRequest+SRWebSocket.m @@ -23,21 +23,18 @@ @implementation NSURLRequest (SRWebSocket) - (nullable NSArray *)SR_SSLPinnedCertificates { - return [NSURLProtocol propertyForKey:SRSSLPinnnedCertificatesKey inRequest:self]; + return nil; } @end @implementation NSMutableURLRequest (SRWebSocket) -- (nullable NSArray *)SR_SSLPinnedCertificates -{ - return [NSURLProtocol propertyForKey:SRSSLPinnnedCertificatesKey inRequest:self]; -} - - (void)setSR_SSLPinnedCertificates:(nullable NSArray *)SR_SSLPinnedCertificates { - [NSURLProtocol setProperty:[SR_SSLPinnedCertificates copy] forKey:SRSSLPinnnedCertificatesKey inRequest:self]; + [NSException raise:NSInvalidArgumentException + format:@"Using pinned certificates is neither secure nor supported in SocketRocket, " + "and leads to security issues. Please use a proper, trust chain validated certificate."]; } @end diff --git a/SocketRocket/SRSecurityPolicy.h b/SocketRocket/SRSecurityPolicy.h index 85e41c549..3fbd80933 100644 --- a/SocketRocket/SRSecurityPolicy.h +++ b/SocketRocket/SRSecurityPolicy.h @@ -28,7 +28,9 @@ NS_ASSUME_NONNULL_BEGIN @param pinnedCertificates Array of `SecCertificateRef` SSL certificates to use for validation. */ -+ (instancetype)pinnningPolicyWithCertificates:(NSArray *)pinnedCertificates; ++ (instancetype)pinnningPolicyWithCertificates:(NSArray *)pinnedCertificates + DEPRECATED_MSG_ATTRIBUTE("Using pinned certificates is neither secure nor supported in SocketRocket, " + "and leads to security issues. Please use a proper, trust chain validated certificate."); /** Specifies socket security and optional certificate chain validation. @@ -37,7 +39,10 @@ NS_ASSUME_NONNULL_BEGIN are considering using this method because your certificate was not issued by a recognized certificate authority, consider using `pinningPolicyWithCertificates` instead. */ -- (instancetype)initWithCertificateChainValidationEnabled:(BOOL)enabled NS_DESIGNATED_INITIALIZER; +- (instancetype)initWithCertificateChainValidationEnabled:(BOOL)enabled + DEPRECATED_MSG_ATTRIBUTE("Disabling certificate chain validation is unsafe. " + "Please use a proper Certificate Authority to issue your TLS certificates.") + NS_DESIGNATED_INITIALIZER; /** Updates all the security options for input and output streams, for example you diff --git a/SocketRocket/SRSecurityPolicy.m b/SocketRocket/SRSecurityPolicy.m index 3997c14fc..3759d26e4 100644 --- a/SocketRocket/SRSecurityPolicy.m +++ b/SocketRocket/SRSecurityPolicy.m @@ -27,7 +27,11 @@ + (instancetype)defaultPolicy + (instancetype)pinnningPolicyWithCertificates:(NSArray *)pinnedCertificates { - return [[SRPinningSecurityPolicy alloc] initWithCertificates:pinnedCertificates]; + [NSException raise:NSInvalidArgumentException + format:@"Using pinned certificates is neither secure nor supported in SocketRocket, " + "and leads to security issues. Please use a proper, trust chain validated certificate."]; + + return nil; } - (instancetype)initWithCertificateChainValidationEnabled:(BOOL)enabled @@ -42,7 +46,12 @@ - (instancetype)initWithCertificateChainValidationEnabled:(BOOL)enabled - (instancetype)init { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated" + return [self initWithCertificateChainValidationEnabled:YES]; + +#pragma clang diagnostic pop } - (void)updateSecurityOptionsInStream:(NSStream *)stream diff --git a/SocketRocket/SRWebSocket.h b/SocketRocket/SRWebSocket.h index 34b7d4360..a3806f1a0 100644 --- a/SocketRocket/SRWebSocket.h +++ b/SocketRocket/SRWebSocket.h @@ -156,7 +156,9 @@ extern NSString *const SRHTTPResponseErrorKey; @param protocols An array of strings that turn into `Sec-WebSocket-Protocol`. Default: `nil`. @param allowsUntrustedSSLCertificates Boolean value indicating whether untrusted SSL certificates are allowed. Default: `false`. */ -- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(nullable NSArray *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates; +- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(nullable NSArray *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates + DEPRECATED_MSG_ATTRIBUTE("Disabling certificate chain validation is unsafe. " + "Please use a proper Certificate Authority to issue your TLS certificates."); /** Initializes a web socket with a given `NSURLRequest`, list of sub-protocols and whether untrusted SSL certificates are allowed. @@ -197,7 +199,9 @@ extern NSString *const SRHTTPResponseErrorKey; @param protocols An array of strings that turn into `Sec-WebSocket-Protocol`. Default: `nil`. @param allowsUntrustedSSLCertificates Boolean value indicating whether untrusted SSL certificates are allowed. Default: `false`. */ -- (instancetype)initWithURL:(NSURL *)url protocols:(nullable NSArray *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates; +- (instancetype)initWithURL:(NSURL *)url protocols:(nullable NSArray *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates + DEPRECATED_MSG_ATTRIBUTE("Disabling certificate chain validation is unsafe. " + "Please use a proper Certificate Authority to issue your TLS certificates."); /** Unavailable initializer. Please use any other initializer. diff --git a/SocketRocket/SRWebSocket.m b/SocketRocket/SRWebSocket.m index 83f3e128f..4e30aef54 100644 --- a/SocketRocket/SRWebSocket.m +++ b/SocketRocket/SRWebSocket.m @@ -186,13 +186,14 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates { SRSecurityPolicy *securityPolicy; - NSArray *pinnedCertificates = request.SR_SSLPinnedCertificates; - if (pinnedCertificates) { - securityPolicy = [SRSecurityPolicy pinnningPolicyWithCertificates:pinnedCertificates]; - } else { - BOOL certificateChainValidationEnabled = !allowsUntrustedSSLCertificates; - securityPolicy = [[SRSecurityPolicy alloc] initWithCertificateChainValidationEnabled:certificateChainValidationEnabled]; - } + BOOL certificateChainValidationEnabled = !allowsUntrustedSSLCertificates; + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated" + + securityPolicy = [[SRSecurityPolicy alloc] initWithCertificateChainValidationEnabled:certificateChainValidationEnabled]; + +#pragma clang diagnostic pop return [self initWithURLRequest:request protocols:protocols securityPolicy:securityPolicy]; } @@ -204,7 +205,12 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request securityPolicy:(SRSec - (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray *)protocols { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated" + return [self initWithURLRequest:request protocols:protocols allowsUntrustedSSLCertificates:NO]; + +#pragma clang diagnostic pop } - (instancetype)initWithURLRequest:(NSURLRequest *)request @@ -219,7 +225,12 @@ - (instancetype)initWithURL:(NSURL *)url; - (instancetype)initWithURL:(NSURL *)url protocols:(NSArray *)protocols; { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated" + return [self initWithURL:url protocols:protocols allowsUntrustedSSLCertificates:NO]; + +#pragma clang diagnostic pop } - (instancetype)initWithURL:(NSURL *)url securityPolicy:(SRSecurityPolicy *)securityPolicy