Skip to content

Commit

Permalink
Deprecate SSL pinning and trust chain verification.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
richardjrossiii committed Aug 14, 2017
1 parent 877ac74 commit 3c53f16
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 21 deletions.
3 changes: 3 additions & 0 deletions SocketRocket/Internal/Security/SRPinningSecurityPolicy.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@

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;
Expand Down
6 changes: 6 additions & 0 deletions SocketRocket/Internal/Security/SRPinningSecurityPolicy.m
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 6 additions & 2 deletions SocketRocket/NSURLRequest+SRWebSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
11 changes: 4 additions & 7 deletions SocketRocket/NSURLRequest+SRWebSocket.m
Original file line number Diff line number Diff line change
Expand Up @@ -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:NSGenericException
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
Expand Down
9 changes: 7 additions & 2 deletions SocketRocket/SRSecurityPolicy.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
11 changes: 10 additions & 1 deletion SocketRocket/SRSecurityPolicy.m
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ + (instancetype)defaultPolicy

+ (instancetype)pinnningPolicyWithCertificates:(NSArray *)pinnedCertificates
{
return [[SRPinningSecurityPolicy alloc] initWithCertificates:pinnedCertificates];
[NSException raise:NSGenericException
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
Expand All @@ -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
Expand Down
8 changes: 6 additions & 2 deletions SocketRocket/SRWebSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<NSString *> *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates;
- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(nullable NSArray<NSString *> *)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.
Expand Down Expand Up @@ -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<NSString *> *)protocols allowsUntrustedSSLCertificates:(BOOL)allowsUntrustedSSLCertificates;
- (instancetype)initWithURL:(NSURL *)url protocols:(nullable NSArray<NSString *> *)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.
Expand Down
25 changes: 18 additions & 7 deletions SocketRocket/SRWebSocket.m
Original file line number Diff line number Diff line change
Expand Up @@ -186,13 +186,14 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NS
- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NSString *> *)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];
}
Expand All @@ -204,7 +205,12 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request securityPolicy:(SRSec

- (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NSString *> *)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
Expand All @@ -219,7 +225,12 @@ - (instancetype)initWithURL:(NSURL *)url;

- (instancetype)initWithURL:(NSURL *)url protocols:(NSArray<NSString *> *)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
Expand Down

0 comments on commit 3c53f16

Please sign in to comment.