Server Name Indication for Rex::Socket::Parameters needed?

[T35R6braPwgDJKq]

If deduced correctly an SNI parameter is needed: https://github.com/rapid7/metasploit-framework/issues/14653?_pjax=%23js-repo-pjax-container#issuecomment-798914673

[smcintyre-r7]

That issue was closed, so I don't think there's any issue here that needs to be resolved. If that's incorrect, just let me know and we can reopen this ticket to discuss details.

[robhinds]

Hi @smcintyre-r7 I believe a wider solution to providing SNI (at a lower level than specific module - e.g. Rex::Socket::Tcp etc, as @T35R6braPwgDJKq mentions in linked ticket) is a good idea?

I have had similar SNI issues using other modules so assume being able to set it more globally would help - I had the same problem as https://github.com/rapid7/metasploit-framework/issues/14653 but with the scanner/http/ssl module, for example. I applied a similar patch as ssl_intercept which resolved it. Also having issues with robot_txt which I believe are also related to not passing through SNI.

(apologies if this is my oversight, I don't normally use Ruby and am new to Metasploit!)

[smcintyre-r7]

If I understand correctly, it sounds like the changes implemented in rapid7/metasploit-framework#14911 that closed rapid7/metasploit-framework#14653 should be moved into Rex::Sockets so that all modules creating SSL TCP client connections can properly handle SNI.

[robhinds]

Yes, I think so @smcintyre-r7 - I think maybe an advanced option to provide an SNI value that then gets passed down to the socket creation step.

I can see there is code that attempts to set the hostname for SNI here https://github.com/rapid7/rex-socket/blob/master/lib/rex/socket/ssl_tcp.rb#L129 but in my case that is not working, so having an option to explicitly provide that value seems like a good solution.

[cyberbutler]

All,

In my opinion, using a global datastore option when using metasploit to set the SNI field is undesirable as it limits the ability to route to multiple RHOSTS. Currently, Rex::Socket::Parameters has an existing parameter I believe to be better suited, the VHOST parameter. As quoted from RFC 6066, SNI was created to provide a means to "facilitate secure connections to servers that host multiple 'virtual' servers at a single underlying network address." I believe the VHOST parameter to fit the intent of the behavior with little compromise to existing code. Just to highlight though, this does mean that VHOST would be the option to set both the ssl_cn and ssl_sni_host attributes.

--- a/lib/rex/socket/parameters.rb
+++ b/lib/rex/socket/parameters.rb
@@ -127,6 +127,7 @@ class Rex::Socket::Parameters

     if (hash['VHOST'])
       self.ssl_cn = hash['VHOST']
+      self.ssl_sni_host = hash['VHOST']
     end

     if (hash['SSLCommonName'])
@@ -426,6 +427,10 @@ class Rex::Socket::Parameters
   # @return [String}
   attr_accessor :ssl_cn

+  # Which SNI host to use for the Client Hello message
+  # @return [String}
+  attr_accessor :ssl_sni_host
+
   # The SSL certificate, in pem format, stored as a string.  See
   # {Rex::Socket::SslTcpServer#makessl}
   # @return [String]
diff --git a/lib/rex/socket/ssl_tcp.rb b/lib/rex/socket/ssl_tcp.rb
index 2d9433b..92ddcfd 100644
--- a/lib/rex/socket/ssl_tcp.rb
+++ b/lib/rex/socket/ssl_tcp.rb
@@ -126,7 +126,7 @@ begin
     # If peerhost looks like a hostname, set the undocumented 'hostname'
     # attribute on sslsock, which enables the Server Name Indication (SNI)
     # extension
-    self.sslsock.hostname = self.peerhost if !Rex::Socket.dotted_ip?(self.peerhost)
+    self.sslsock.hostname = params.ssl_sni_host || (self.peerhost if !Rex::Socket.dotted_ip?(self.peerhost))

     # Force a negotiation timeout
     begin

This makes implementation easy in the existing metasploit modules by passing the VHOST parameter to existing Rex::Socket::TCP.create methods. For example, in https://github.com/rapid7/metasploit-framework/blob/master/lib/rex/proto/http/client.rb, the only necessary change would be the following:

--- a/lib/rex/proto/http/client.rb
+++ b/lib/rex/proto/http/client.rb
@@ -180,7 +180,8 @@ class Client
       'SSLVersion' => self.ssl_version,
       'Proxies'    => self.proxies,
       'Timeout'    => timeout,
-      'Comm'       => self.comm
+      'Comm'       => self.comm,
+      'VHOST'      => self.config['vhost']
     )
   end

Some other quick notes

Because of the parsing mechanisms in the metasploit-framework for RHOSTS, all provided FQDNs get converted into their IP Address equivalents. During this process, the VHOST parameter is set automatically in the datastore to the FQDN the IP address was translated from. Additionally, if the user were to manually set the VHOST datastore parameter, this change would persist all the way down to the SNI field. These are additional reasons why I have selected the VHOST parameter for this change. Shout out to @ruddawg26 for his help debugging all of this.

If you all agree with what I have laid out here, I would be thrilled to submit a PR!

[smcintyre-r7]

Also possibly related to https://github.com/rapid7/metasploit-framework/issues/16213.

[gwillcox-r7]

Assigning this to myself since this may be related to another issue that I had that Spencer linked to above, though may need to land a few PRs before I can take a look at this so excuse any delays that may occur before I get a chance to dive into this deeper.