-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fixed certificate validation for servers providing incomplete chains #3159
Fixed certificate validation for servers providing incomplete chains #3159
Conversation
@@ -388,6 +418,28 @@ - (void)testPolicyWithCertificatePinningAllowsHTTPBinOrgServerTrustWithHTTPBinOr | |||
XCTAssertTrue([policy evaluateServerTrust:AFUTHTTPBinOrgServerTrust() forDomain:@"httpbin.org"], @"Policy should allow server trust"); | |||
} | |||
|
|||
- (void)testPolicyWithCertificatePinningAllowsGoogleComServerTrustIncompleteChainWithRootCertificatePinnedAndValidDomainName { | |||
// google.com sends an incomplete certificate chain consisting of 3 certificates ( *.google.com, Google Internet Authority G2 and GeoTrust Global CA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the comment with a reference to the pull request number? Thanks for the docs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Are there any negative tests we should consider here? |
I think the negative tests are the same as the "classic" certificate pinning case, i.e. if none of the server´s certificates is pinned, the connection should be cancelled. The same goes for expired and invalid certificates, I guess. I can't think of a specific negative case for this particular issue. Any idea? |
I couldn't think of any either, just wanted to pose the question. |
Some servers send incomplete certificate chains (without the Root CA), which cause issues in the latest version's (2.6.3) certificate pinning mechanism. In these cases, when `AFSecurityPolicy`s `-evaluateServerTrust:` invokes `AFCertificateTrustChainForServerTrust(serverTrust)` (prior to setting the anchor), the Root CA isn't present (the server didn't send it). Afterwards, even though `AFServerTrustIsValid(serverTrust)` succeeds, the next validation fails since no pinned certificate matches any of the `serverCertificates`. By fetching the `AFCertificateTrustChainForServerTrust(serverTrust)` *after* the `AFServerTrustIsValid(serverTrust)` validation, the complete chain is obtained and the Root CAs match (pinned vs validated). A slight optimization was also made when validating if the `serverTrust` certificates contain the `pinnedCertificates`, looping in reverse order and returning after finding the first match.
cda9072
to
468c2ba
Compare
…e-validation-3_0_0 Fixed certificate validation for servers providing incomplete chains
🎉 🍻 🎉 |
@p4checo it looks like expiration day has come so the test suite is now failing. Can you walk me through where you grabbed these two chains so I can get them updated? |
Hi @kcharwood, I used https://www.ssllabs.com/ssltest/analyze.html?d=google.com to validate whether it was using two chains, and then I got the certificates using OpenSSL:
However, I've now checked that google.com seems to no longer have 2 certification paths 😓 |
How should we proceed here? I don't want to disable the test if I don't have too. |
Ideally, we should find another case of a multiple certification path host and update the test. 😄 At the time I was lucky google.com itself was an example. But surely there are other cases. |
K. I don't have a lot of time to dig on this issue at the moment, so I may need to disable it in the near term to allow the test suite to pass for some other issues I need to push through. |
Me neither 😞. If I manage to put aside some time for this I'll let you know. |
If we've set our pinned certs as the exclusive anchors in our trust store, why do we need to explicitly compare cert data at all?
What problem is the additional code solving? |
The issue this PR was fixing was due to incomplete certificate chains being sent by the server and making certificate pinning fail precisely because of this certificate data comparison. However, the point you raise is valid since the certificates are already defined as anchors, we shouldn't have to compare the data ourselves manually. That should be (and already is) done more efficiently by So, that data comparison check (and my complete "fix") would be unnecessary because I heard they welcome PR's in this project, @RadimC 😉 Cheers! 🍻 |
Some servers send incomplete certificate chains (without the Root CA), which
cause issues in the latest version's (2.6.3) certificate pinning mechanism.
In these cases, when
AFSecurityPolicy
s-evaluateServerTrust:
invokesAFCertificateTrustChainForServerTrust(serverTrust)
(prior to setting theanchor), the Root CA isn't present (the server didn't send it). Afterwards,
even though
AFServerTrustIsValid(serverTrust)
succeeds, the next validationfails since no pinned certificate matches any of the
serverCertificates
.By fetching the
AFCertificateTrustChainForServerTrust(serverTrust)
afterthe
AFServerTrustIsValid(serverTrust)
validation, the complete chain isobtained and the Root CAs match (pinned vs validated).
A slight optimization was also made when validating if the
serverTrust
certificates contain the
pinnedCertificates
, looping in reverse order andreturning after finding the first match.
Follow up of PR #2921, but rebased for 3.0.0