Repository navigation
Conversation
setEndpointCheckingEnabled(true) was only applied to the control socket in sslNegotiation; the data socket handshake in _openDataConnection_ ran without endpoint identification. The data socket has no peer host name (it is connected to an address or accepted), so the control host name is also supplied as the SNI server name for the trust manager to check.
This file contains hidden or 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
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.
setEndpointCheckingEnabled(true)only takes effect insslNegotiation, so the control connection gets HTTPS endpoint identification while the data socket opened for each transfer in_openDataConnection_completes its handshake without it. With the default trust manager, which checks validity but not the name, an in-path attacker on the data port can present any trusted certificate issued for another name and read or alter listings and file contents, even though the caller asked for endpoint checking (the same gap #404 closed for theHostnameVerifieroption).Switching on the algorithm alone is not enough: the control socket is layered with
_hostname_, but the data socket is connected to an address (or accepted in active mode), so the JDK would match the certificate against the peer IP only and every name based certificate fails with "No subject alternative names present" (that is what happens to the existingendpointCheckingEnabled=truetests with only the flag set). The change therefore also passes the control connection host name as the SNI server name, which the JDK trust manager uses for endpoint identification; IP literals are left out since they are not valid SNI names and are matched against the peer address as before.The new test fails on master (
expected: <HTTPS> but was: <null>) and passes with the change; the existing endpoint checking transfer tests exercise the actual name match against the test certificate. The default goal build (mvn) passes: 633 tests, checkstyle, PMD, japicmp and rat clean.mvn; that'smvnon the command line by itself.