[V2.0.4] Crashes with SSL Pinning

[rmbrgn]

Hi,

Since the last version, and particularly since the fix on the SSL Pinning (dbeb119), I experience a couple of crashes on my app, in production (App Store).

The issue

The line that causes the problem is this one (and especially the as! 😺):
https://github.com/daltoniam/Starscream/blob/master/Source/WebSocket.swift#L398

It means that in this case, the kCFStreamPropertySSLPeerTrust is null.

Other libs

I've looked at other implementations of SSL Pinning on other libraries, and they are all doing the pinning at the old location (open func stream(_ aStream: Stream, handle eventCode: Stream.Event)).

• https://github.com/facebook/SocketRocket/blob/master/SocketRocket/SRWebSocket.m#L1399
• https://github.com/mixpanel/mixpanel-iphone/blob/master/Mixpanel/MPWebSocket.m#L1420
• ...

And more important, they check if this variable is not-null.

Solution (or not !)

We could easily secure this code and avoid the crash, by changing it to the following one, but I'm not really sure that the SSL Pinning would be done anymore.

if let sec = s.security,
    !s.certValidated,
    let trust = outStream.property(forKey: kCFStreamPropertySSLPeerTrust as Stream.PropertyKey) as? SecTrust {

Questions

• In your opinion, why the kCFStreamPropertySSLPeerTrust would be null at this precise moment?
• What decision lead you to moving this code from open func stream(_ aStream: Stream, handle eventCode: Stream.Event)?

[daltoniam]

Hi @remybourgoin. Thanks for the quality write up. I haven't see that issue before, normally a trust comes along with a secure stream, but I guess it could be possible to fall into that block of code connecting to a non secure stream and having a valid security object set. I agree that state should be handled and will be updated accordingly.

The decision that lead to the moving of the code from the func stream was actually a report of an SSL pinning vulnerability here. Basically they found in their SSL pinning tests that they could bypass the checking in certain cases. The moving of the code fixed this vulnerability according to their reports and testing.

[giullo]

I opened #349 to fix the issue

[daltoniam]

2.1.0 release with #349 fix. Thanks!