[SSL/TLS] Insecure defaults for server identity checking
Hi all,
By default the SSLSocket does not perform any server identity checks.
This means that unless the user explicitly enables the checks, the
connection will be vulnerable to man-in-the-middle attacks. Examples
of vulnerable implementation can be found in the Java documentation,
example links:
https://docs.oracle.com/javase/jp/11/security/sample-code-illustrating-secure-socket-connection-client-and-server.html#GUID-AA1C27A1-2CA8-4309-B281-D6199F60E666
https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sockets/client/SSLSocketClient.java
(the code samples are from older JDK releases, but even the recent
releases link to them).
Simplified version of the code from the above examples follows:
SSLSocketFactory factory = (SSLSocketFactory)SSLSocketFactory.getDefault();
SSLSocket socket = (SSLSocket)factory.createSocket("www.verisign.com", 443);
socket.startHandshake();
In order to enable the identity checks, the user has to add the
following code before starting the handshake:
SSLParameters params = new SSLParameters();
params.setEndpointIdentificationAlgorithm("HTTPS");
socket.setSSLParameters(params);
Without the added code, the client will happily accept server
certificates that are not related to verisign in any way, as long as
they are issued by a trusted CA. Thanks to letsencrypt.org, anyone can
get such a certificate for free.
This situation is less than ideal. It's way too easy to forget that
the identity checks are not done.
I think we should run the HTTPS-like identity checks by default, and
let the users opt out if indeed they want to run their own identity
checks. Thoughts?
Regards,
Daniel
Re: RFR: 8335288: SunPKCS11 initialization will call C_GetMechanismInfo on unsupported mechanisms [v2]
On Wed, 21 Aug 2024 00:09:25 GMT, Valerie Peng wrote: >> Can someone help review this fix? Changed the required-mechanism check by >> checking if the particular mechanism is inside the list of enabled supported >> mechanisms. This should be more reliable than calling C_GetMechanismInfo(..) >> on the required mechanism given vendors may return various sorts of error >> codes. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments and reverted back to iterating through all available > mechanisms. Looks good to me. - Marked as reviewed by mbalao (Committer). PR Review: https://git.openjdk.org/jdk/pull/20207#pullrequestreview-2251387115
Re: RFR: 8335288: SunPKCS11 initialization will call C_GetMechanismInfo on unsupported mechanisms [v2]
On Wed, 21 Aug 2024 16:48:23 GMT, Martin Balao wrote: > Looks good to me. Thanks Martin for the review~ - PR Comment: https://git.openjdk.org/jdk/pull/20207#issuecomment-2302556977
Re: RFR: 8338380: Update TLSCommon/interop/AbstractServer to specify an interface to listen for connections
On Wed, 14 Aug 2024 12:22:47 GMT, Matthew Donovan wrote: > This is a small PR to extend the AbstractServer class in > test/jdk/javax/net/ssl/TLSCommon/interop/ to enable users to specify a > specific interface on which to listen for incoming connections. The default > interface is now the loopback interface. The derived class, JdkServer, is > also updated to use the interface specified in AbstractServer. test/jdk/javax/net/ssl/TLSCommon/interop/JdkServer.java line 58: > 56: serverSocket > 57: = (SSLServerSocket) > serverFactory.createServerSocket(builder.getPort(), > 58: 5, builder.getListenInterface()); Please update `backlog` parameter to `0` so the default value is used. - PR Review Comment: https://git.openjdk.org/jdk/pull/20581#discussion_r1725471234
Re: RFR: 8319332: Security properties files inclusion [v20]
> The implementation of this proposal is based on the requirements, > specification and design choices described in the [JDK-8319332] ticket and > its respective CSR [JDK-8319333]. What follows are implementation notes > organized per functional component, with the purpose of assisting to navigate > the code changes in this pull-request. > > ## Security properties loading (overview) > > A new static class named `SecPropLoader` (nested within > `java.security.Security`) is introduced to handle the loading of all security > properties. Its method `loadAll` is the first one to be called, at > `java.security.Security` static class initialization. The master security > properties file is then loaded by `loadMaster`. When additional security > properties files are allowed (the security property > `security.overridePropertiesFile` is set to `true`) and the > `java.security.properties` system property is passed, the method `loadExtra` > handles the extra load. > > The master properties file is loaded in `OVERRIDE` mode, meaning that the map > of properties is originally empty. Any failure occurred while loading these > properties is considered fatal. The extra properties file > (`java.security.properties`) may be loaded in `OVERRIDE` or `APPEND` mode. > Any failure in this case is ignored. This behavior maintains compatibility > with the previous implementation. > > While the `java.security.properties` system property is documented to accept > an URL type of value, filesystem path values are supported in the same way > that they were prior to this enhancement. Values are then interpreted as > paths and, only if that fails, are considered URLs. In the latter case, there > is one more attempt after opening the stream to check if there is a local > file path underneath (e.g. the URL has the form of > `file:///path/to/a/local/file`). The reason for preferring paths over URLs is > to support relative path file inclusion in properties files. > > ## Loading security properties from paths (`loadFromPath` method) > > When loading a properties file from a path, the normalized file location is > stored in the static field `currentPath`. This value is the current base to > resolve any relative path encountered while handling an _include_ definition. > Normalized paths are also saved in the `activePaths` set to detect recursive > cycles. As we move down or up in the _includes_ stack, `currentPath` and > `activePaths` values are updated. > > ## Loading security properties from URLs (`loadFromUrl` method) > > The extra properties file can be loaded from a URL. ... Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision: Document list of reserved keys in java.security.Security::getProperty/setProperty APIs. Co-authored-by: Martin Balao Co-authored-by: Francisco Ferrari Bihurriet - Changes: - all: https://git.openjdk.org/jdk/pull/16483/files - new: https://git.openjdk.org/jdk/pull/16483/files/580d34a6..3b6f99ce Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16483&range=19 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16483&range=18-19 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16483.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16483/head:pull/16483 PR: https://git.openjdk.org/jdk/pull/16483
Re: RFR: 8338380: Update TLSCommon/interop/AbstractServer to specify an interface to listen for connections [v2]
> This is a small PR to extend the AbstractServer class in > test/jdk/javax/net/ssl/TLSCommon/interop/ to enable users to specify a > specific interface on which to listen for incoming connections. The default > interface is now the loopback interface. The derived class, JdkServer, is > also updated to use the interface specified in AbstractServer. Matthew Donovan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - fixed copyright and set backlog to 0(default) - Merge branch 'master' into 8338380 - 8338380: Update TLSCommon/interop/AbstractServer to specify an interface to listen for connections - Changes: - all: https://git.openjdk.org/jdk/pull/20581/files - new: https://git.openjdk.org/jdk/pull/20581/files/e018b30f..7af1e551 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20581&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20581&range=00-01 Stats: 13940 lines in 361 files changed: 9329 ins; 2849 del; 1762 mod Patch: https://git.openjdk.org/jdk/pull/20581.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20581/head:pull/20581 PR: https://git.openjdk.org/jdk/pull/20581
