Fix overlapping pinsets #200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test had an outdated pinset which caused it to fail. Updated the pinset to include a pin from the current intermediate CA certificate (Let's Encrypt Authority X3)
Observed behavior of TrustKit showed that, when a domain did not have an exact match for a pinset, the first matching pinset config with the IncludeSubdomains flag was selected. This led to unpredictable behavior because of the nature of iterating through dictionary keys. The change in this commit modifies TrustKit's selection algorithm to iterate through all pinset configs and then select the one that is the closest match (e.g, longest domain). This matches the industry best practice set by Google in their native Android pinning implementation, and brings TrustKit's behavior in line with that of TrustKit-Android.
|
@nabla-c0d3 have you had a chance to look at this? I'm happy to elaborate if that would be helpful. Thanks! |
Ishtiayk
approved these changes
Jun 2, 2019
|
It looks good - thanks! |
|
@nabla-c0d3 Could a formal release be created for this fix? This fix is something that would help me in my project and I would like to be able to incorporate this without having to reference this merge hash. Thanks! |
|
@SomeRandomiOSDev I released it just now as v1.6.2. Thanks again for the PR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed that TrustKit behaves unpredictably in a particular corner case--
If there are two overlapping pinsets that include subdomains (say, "foo.com" and "bar.foo.com"), and a host matches both pinsets (say, "baz.bar.foo.com"), the selected pinset cannot be predicted. It's simply determined by which of those two keys shows up first while TrustKit is iterating over its dictionary of domains.
The best example of industry best practice I could find was from Google.
As far as I know, Android is the only platform that provides cert pinning functionality out-of-the-box.
And, TrustKit-Android relies on the base Android pinning implementation.
Google has chosen to handle this corner case by choosing the "most specific (longest) matching domain rule".
See the following links for references:
This is the behavior I had originally expected TrustKit to adhere to.
I modified the selection code to use this approach, and wrote a unit test to validate it.