Skip to content

SecTrust object now is conditionally unwrapped#349

Merged
daltoniam merged 1 commit into
daltoniam:masterfrom
giullo:Fix-SSL-Pinning-Crash
Jun 29, 2017
Merged

SecTrust object now is conditionally unwrapped#349
daltoniam merged 1 commit into
daltoniam:masterfrom
giullo:Fix-SSL-Pinning-Crash

Conversation

@giullo

@giullo giullo commented Jun 27, 2017

Copy link
Copy Markdown

This PR fixes an issue introduced by dbeb119#diff-b9ab27dc605d36ce558013387d2ba2d5

Now the SecTrust object is conditionally unwrapped and if not found , the pinning will fail*. I was never able to reproduce the scenario where the SecTrust object is null within a wss/https connection, i think this could be related to a failed SSL handshake (maybe due to networking issues) but these are just speculations at this point.

  • I consider the pinning failed, because if you end up in that code branch it's because:
  1. the url is either wss or https
  2. a security object was provided to perform the certificate pinning
    the client is clearly signalling is will for the pinning to be performed, so in absence of a trust that contains the certificates to be checked, i think it's better to reject the connection

@daltoniam

Copy link
Copy Markdown
Owner

Great work and thanks for fixing that issue!

@daltoniam daltoniam merged commit 08edd54 into daltoniam:master Jun 29, 2017
@giullo

giullo commented Jul 3, 2017

Copy link
Copy Markdown
Author

Hey thanks for the quick approval. i see that this is included in 2.1.0, but somehow the spec hasn't been pushed to cocoa pods , because the latest available is still 2.0.4 (and if you try to target directly 2.1.0 you will get an error that the dependency can't be satisfied)

Could you have a look? ah and btw, many thanks for the great library :)

@daltoniam

Copy link
Copy Markdown
Owner

Finally got it done. Had a fair amount of trouble with CocoaPods & SPM working together. Anyway, 2.1.0 has been released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants