Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove tls handling override when no pinner provided #775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

myeyesareblind
Copy link

When no pinner was provided, it is expected that framework will use
whatever iOS will provide. But instead - it will allow ALL
connections, bypassing any verification.
This is breaking change, but most clients of the library probably
never had to do that.
Client can still allow selfSigned certs with FoundationSecurityError.allowSelfSigned.

When no pinner was provided, it is expected that framework will use
whatever iOS will provide. But instead - it will allow ALL
connections, bypassing any verification.
This is breaking change, but most clients of the library probably
never had to do that.
Client can still allow selfSigned certs with FoundationSecurityError.allowSelfSigned.
@myeyesareblind
Copy link
Author

The problem I encountered in the first place is:
Wildcard certificates are not validated properly.

I couldn't understand how to fix that particular issue, so looked for a workarounds.
The workaround with pinner == nil looks dangerous to me.

@myeyesareblind myeyesareblind changed the title Remove default tls handling override when no pinner provided Remove tls handling override when no pinner provided May 12, 2020
@lauri-paypay
Copy link

Hi @daltoniam @acmacalister,

As part of ongoing research, we found that the current behavior of Starscream does not use the normal iOS certificate verification logic if pinners are not provided. As a result, the default configuration is vulnerable to MITM attacks using, for example, valid TLS certificates for the wrong domain. This PR fixes the issue by not overriding the verifier if a pinner is not provided.

Could you please take a look at this PR? We believe it constitutes a vulnerability.

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