ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11148

tristantarrant · 2023-07-21T18:06:33Z

https://issues.redhat.com/browse/ISPN-15069

This also enables TLS hostname validation by default according to RFC 2818, which also allows us to test it.

...ient/src/main/java/org/infinispan/client/hotrod/impl/transport/netty/ChannelInitializer.java

...d-client/src/main/java/org/infinispan/client/hotrod/impl/transport/netty/ChannelFactory.java

pruivo · 2023-08-11T17:44:05Z

@tristantarrant I think it needs to be rebased on top of main. The native tests are failing.

pruivo

Can't say I understand everything but it looks good to me. Waiting for the docs to give it a try (if it is urgent, it can be merged right away)

pruivo · 2023-08-11T17:56:26Z

...ient/src/main/java/org/infinispan/client/hotrod/impl/transport/netty/ChannelInitializer.java

@@ -173,16 +178,26 @@ private void initSsl(Channel channel) {
            throw new CacheConfigurationException(e);
         }
      } else {
-         sslContext = new JdkSslContext(ssl.sslContext(), true, ClientAuth.NONE);
+         sslContext = new JdkSslContext(ssl.sslContext(), true, null, IdentityCipherSuiteFilter.INSTANCE,


I think this will throw an NPE because JdkApplicationProtocolNegotiator is null. There is a null check in netty this.apn = (JdkApplicationProtocolNegotiator)ObjectUtil.checkNotNull(apn, "apn");

You missed the fact that the overload calls toNegotiator(apn, !isClient) which, if apn is null, will use the default instance

pruivo · 2023-08-11T18:02:38Z

client/hotrod/src/main/java/org/infinispan/hotrod/impl/transport/netty/ChannelInitializer.java

@@ -171,16 +176,25 @@ private void initSsl(Channel channel) {
            throw new CacheConfigurationException(e);
         }
      } else {
-         sslContext = new JdkSslContext(ssl.sslContext(), true, ClientAuth.NONE);
+         sslContext = new JdkSslContext(ssl.sslContext(), true, null, IdentityCipherSuiteFilter.INSTANCE,


possible NPE here too.

Same as above

ryanemerson · 2023-08-14T13:17:41Z

...client/src/main/java/org/infinispan/client/hotrod/configuration/SslConfigurationBuilder.java

@@ -223,6 +224,17 @@ public SslConfigurationBuilder ciphers(String... ciphers) {
      return enable();
   }

+   /**
+    * Configures whether to enable hostname validation according to <a href="https://datatracker.ietf.org/doc/html/rfc2818">RFC 2818</a>.
+    * This is enabled by default and requires that the server certificate includes a subjectAltName extension of type dNSName or iPAddress.


I assume we're going to disable this by default on the 14.0.x branch? This could break a lot of existing deployments.

I need to run this by PST

PST confirms that, because this is a CVE, it needs to be enabled by default. No way around it.

ryanemerson · 2023-08-14T13:20:00Z

documentation/src/main/asciidoc/topics/upgrading.adoc

@@ -27,6 +27,7 @@ The following defaults have changed for the Hot Rod client:
 * `socket_timeout` is now 2000 ms / 2 seconds (previously 60000 ms / 60 seconds)
 * `max_retries` is now 3 (was 10)
 * `min_evictable_idle_time` is now 180000 ms / 3 minutes (previously 1800000 ms / 30 minutes)
+* `ssl_hostname_validation` is a new property and defaults to `true`. This also requires setting the `sni_hostname`.


Should we also state that the server-side certs may also have to be updated?

Yes, I'm also going to add a warning on server startup if the certificate doesn't have the SubjectAlternativeNamesExtension

tbmoomau · 2023-08-25T18:00:00Z

Good afternoon. I know you are all busy, but just want to check if there is any plan to continue moving forward with this pull request. Thanks.

pruivo · 2023-08-25T18:10:18Z

@tbmoomau yes, after the holiday season.

domiborges

Hey @tristantarrant, I added two suggestions. Let me know if you want me to add my commit

domiborges · 2023-08-29T15:26:03Z

documentation/src/main/asciidoc/topics/proc_configuring_server_keystores.adoc

+Certificates should include the `subjectAltName` extension of type `dNSName` and/or `iPAddress` so that clients can correctly perform
+hostname validation, according to the rules defined by the https://datatracker.ietf.org/doc/html/rfc2818[RFC 2818] specification.
+The server will issue a warning if it is started with a certificate which does not include such an extension.


Suggested change

Certificates should include the `subjectAltName` extension of type `dNSName` and/or `iPAddress` so that clients can correctly perform

hostname validation, according to the rules defined by the https://datatracker.ietf.org/doc/html/rfc2818[RFC 2818] specification.

The server will issue a warning if it is started with a certificate which does not include such an extension.

Certificates must include the `subjectAltName` extension of the `dNSName` or `iPAddress` type, or both, to enable clients to perform hostname validation, following the rules defined by the https://datatracker.ietf.org/doc/html/rfc2818[RFC 2818] specification.

If the certificate does not include this extension, the server issues a warning upon startup.

domiborges · 2023-08-29T15:28:46Z

documentation/src/main/asciidoc/topics/proc_configuring_hotrod_encryption.adoc

+By default, Hot Rod clients will also perform hostname validation by matching the `dNSName` and/or `iPAddress` contained by the server certificate's `subjectAltName` extension with the expected hostname.
+


Suggested change

By default, Hot Rod clients will also perform hostname validation by matching the `dNSName` and/or `iPAddress` contained by the server certificate's `subjectAltName` extension with the expected hostname.

By default, Hot Rod clients also perform hostname validation by matching the `dNSName` or `iPAddress` or both types contained by the server certificate's `subjectAltName` extension with the expected hostname.

and/or can be ambiguous and causes translation problems

* Add TLS hostname validation * Per-cluster SNI * Implict SNI if not set * Also add a method to return current cluster * JMX equivalent of above

tristantarrant · 2023-09-08T16:06:43Z

Added @domiborges change and rebased. This is now ready

tristantarrant · 2023-09-11T07:28:46Z

Failures are unrelated

ryanemerson · 2023-09-12T16:46:49Z

Thanks @tristantarrant

tristantarrant force-pushed the ISPN-15069/cluster_sni branch from 317c584 to 2d30c45 Compare July 21, 2023 18:08

pruivo reviewed Aug 4, 2023

View reviewed changes

...ient/src/main/java/org/infinispan/client/hotrod/impl/transport/netty/ChannelInitializer.java Outdated Show resolved Hide resolved

pruivo reviewed Aug 4, 2023

View reviewed changes

...d-client/src/main/java/org/infinispan/client/hotrod/impl/transport/netty/ChannelFactory.java Outdated Show resolved Hide resolved

tristantarrant force-pushed the ISPN-15069/cluster_sni branch from 2d30c45 to b6ae403 Compare August 11, 2023 14:26

tristantarrant marked this pull request as ready for review August 11, 2023 14:27

pruivo requested changes Aug 11, 2023

View reviewed changes

tristantarrant force-pushed the ISPN-15069/cluster_sni branch 3 times, most recently from 9adaf4d to f365dc8 Compare August 14, 2023 13:04

ryanemerson reviewed Aug 14, 2023

View reviewed changes

tristantarrant force-pushed the ISPN-15069/cluster_sni branch 3 times, most recently from fc584fc to 52814be Compare August 29, 2023 07:22

tristantarrant changed the title ~~ISPN-15069 More flexbile SNI from the client~~ ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation Aug 29, 2023

pruivo approved these changes Aug 29, 2023

View reviewed changes

domiborges reviewed Aug 29, 2023

View reviewed changes

ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation

0c6b15e

* Add TLS hostname validation * Per-cluster SNI * Implict SNI if not set * Also add a method to return current cluster * JMX equivalent of above

tristantarrant force-pushed the ISPN-15069/cluster_sni branch from 52814be to 0c6b15e Compare September 8, 2023 16:06

ryanemerson added the 14.0.x Annotate a PR with this label if you want it to be backported to the 14.0.x branch label Sep 12, 2023

ryanemerson merged commit c5d945b into infinispan:main Sep 12, 2023
3 of 4 checks passed

github-actions bot mentioned this pull request Sep 12, 2023

[14.0.x] ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11148

ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11148

tristantarrant commented Jul 21, 2023 •

edited

pruivo commented Aug 11, 2023

pruivo left a comment

pruivo Aug 11, 2023

tristantarrant Aug 14, 2023

pruivo Aug 11, 2023

tristantarrant Aug 14, 2023

ryanemerson Aug 14, 2023

tristantarrant Aug 14, 2023

tristantarrant Aug 28, 2023

ryanemerson Aug 14, 2023

tristantarrant Aug 28, 2023

tbmoomau commented Aug 25, 2023

pruivo commented Aug 25, 2023

domiborges left a comment

domiborges Aug 29, 2023

domiborges Aug 29, 2023

domiborges Aug 29, 2023

tristantarrant commented Sep 8, 2023

tristantarrant commented Sep 11, 2023

ryanemerson commented Sep 12, 2023

		By default, Hot Rod clients will also perform hostname validation by matching the `dNSName` and/or `iPAddress` contained by the server certificate's `subjectAltName` extension with the expected hostname.

	By default, Hot Rod clients will also perform hostname validation by matching the `dNSName` and/or `iPAddress` contained by the server certificate's `subjectAltName` extension with the expected hostname.
	By default, Hot Rod clients also perform hostname validation by matching the `dNSName` or `iPAddress` or both types contained by the server certificate's `subjectAltName` extension with the expected hostname.

ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11148

ISPN-15069 CVE-2023-4586 Hot Rod TLS Hostname validation #11148

Conversation

tristantarrant commented Jul 21, 2023 • edited

pruivo commented Aug 11, 2023

pruivo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbmoomau commented Aug 25, 2023

pruivo commented Aug 25, 2023

domiborges left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristantarrant commented Sep 8, 2023

tristantarrant commented Sep 11, 2023

ryanemerson commented Sep 12, 2023

tristantarrant commented Jul 21, 2023 •

edited