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

Security hole / configuration problem in AFSecurityPolicy.m #2590

Closed
duttski opened this Issue Mar 12, 2015 · 2 comments

Comments

Projects
None yet
1 participant
@duttski
Copy link
Contributor

commented Mar 12, 2015

[Introduced in the line below, between 2.5.0 and 2.5.1; tested on iOS 8.1.3]

When establishing a security policy for a connection you have two basic options: "pinned" or "not pinned" (default). Selecting pinned mode will then force validatesDomainName to NO.

When it comes to making a decision on whether to trust a domain there is one path that goes as follows:

if (self.SSLPinningMode != AFSSLPinningModeNone && !AFServerTrustIsValid(serverTrust) && !self.allowInvalidCertificates) {
    return NO;
}

...

switch (self.SSLPinningMode) {
    case AFSSLPinningModeNone:
        return YES;

So if you disable Pinning mode it will ALWAYS accept the certificate. The only way to establish that a certificate is invalid is to set up SSLPinningMode[something, not 'None'].

So the default policy is validatesDomainName = YES, SSLPinningModeNone. This set up results in ALWAYS accepting certificates.

I have verified that a malicious proxy server can sniff all the contents of HTTPS communication in this case.

Please make sure this confusing state of affairs doesn't make it into production applications -- I would love to write a test / fix to help with this but I'm simply not aux fait enough with the libraries involved.

@duttski

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2015

PS. I would suggest that the Readme encourages people not to use 2.5.1 in production for this reason... Currently it comes by default following the install instructions through Pods.

@duttski

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2015

PPS. The commit above is designed to test the actual cases, and represent an attempt to set up a test case that matches a sensible (and indeed the previous) set of behaviours. I have tried to create a test case that forces the library to behave in a 'secure is better' way. NB. Initially, changing the behaviour of the function didn't impact the test results significantly, which does suggest that there could be a problem with the tests. either way, I think a stern look at this is appropriate given the implications, and hopefully I have already provided something sensible to work with, if not the actual solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.