HTTP option any_af breaks TLS SNI

[nnposter]

NSE HTTP requests with option any_af result in TLS requests that populate SNI with an address, instead of the hostname. Example code:

http.get("foo.bar.com", 443, "/", {any_af=true})

The reason is that the hostname gets replaced with an address in function request (a local function in http.lua), losing the original name:

  if type(host) == "string" and options.any_af then
    local status, addrs = nmap.resolve(host)
    host = addrs[1] or host
  end

Subsequent code then unsurprisingly populates SNI with the new host value.

The following patch seems to resolve the issue by converting the host string into a table and preserving the original name as targetname:

--- a/nselib/http.lua
+++ b/nselib/http.lua
@@ -1194,7 +1194,9 @@
 
   if type(host) == "string" and options.any_af then
     local status, addrs = nmap.resolve(host)
-    host = addrs[1] or host
+    if status then
+      host = {ip = addrs[1], targetname = host}
+    end
   end
 
   local socket, partial, opts = comm.tryssl(host, port, data, { timeout = optio

Please let me know if you see any issues with the patch or you believe that the problem should be resolved differently. Otherwise I will commit the patch in a few weeks.

[dmiller-nmap]

I can see the problem and how this is a good fix. But I wonder if it would be more useful to add the any_afoption and its associated name resolution to comm.lua and then have http.lua simply pass the option through. I guess this could be done as a separate commit after this fix, but it just makes sense as a function of the comm library.

[nnposter]

I definitely agree that this option is squarely relevant to the transport, not HTTP, so comm.lua is a more logical place where the address resolution should take place.

At the same time, I have felt that modifying the comm-level options is not something that I should put forward in the context of this issue so I kept the change as localized as possible.

Let me see what it would take to implement the address resolution in comm.lua.

[nnposter]

Here is the broader patch for http.lua (to be applied on top of the previous one):

--- nselib/http.lua.orig	2017-02-26 15:30:38.045727900 -0700
+++ nselib/http.lua	2017-02-28 17:28:00.334505800 -0700
@@ -72,7 +72,7 @@
 -- * <code>bypass_cache</code>: Do not perform a lookup in the local HTTP cache.
 -- * <code>no_cache</code>: Do not save the result of this request to the local HTTP cache.
 -- * <code>no_cache_body</code>: Do not save the body of the response to the local HTTP cache.
--- * <code>any_af</code>: Allow connecting to any address family, inet or inet6. By default, these functions will only use the same AF as nmap.address_family to resolve names.
+-- * <code>any_af</code>: Allow connecting to any address family, inet or inet6. By default, these functions will only use the same AF as nmap.address_family to resolve names. (This option is a straight pass-thru to <code>comm.lua</code> functions.)
 -- * <code>redirect_ok</code>: Closure that overrides the default redirect_ok used to validate whether to follow HTTP redirects or not. False, if no HTTP redirects should be followed. Alternatively, a number may be passed to change the number of redirects to follow.
 --   The following example shows how to write a custom closure that follows 5 consecutive redirects, without the safety checks in the default redirect_ok:
 --   <code>
@@ -1192,14 +1192,7 @@
 
   method = string.match(data, "^(%S+)")
 
-  if type(host) == "string" and options.any_af then
-    local status, addrs = nmap.resolve(host)
-    if status then
-      host = {ip = addrs[1], targetname = host}
-    end
-  end
-
-  local socket, partial, opts = comm.tryssl(host, port, data, { timeout = options.timeout })
+  local socket, partial, opts = comm.tryssl(host, port, data, {timeout = options.timeout, any_af = options.any_af})
 
   if not socket then
     stdnse.debug1("http.request socket error: %s", partial)

and for comm.lua:

--- nselib/comm.lua.orig	2016-12-09 08:24:04.000000000 -0700
+++ nselib/comm.lua	2017-02-28 17:24:21.949505800 -0700
@@ -14,6 +14,7 @@
 -- * <code>connect_timeout</code> - socket timeout for connection. Default: same as <code>stdnse.get_timeout</code>
 -- * <code>request_timeout</code> - additional socket timeout for requests. This is added to the connect_timeout to get a total time for a request to receive a response. Default: 6000ms
 -- * <code>recv_before</code> - boolean, receive data before sending first payload
+-- * <code>any_af</code> - boolean, allow connecting to any address family, inet or inet6. By default, these functions will only use the same AF as nmap.address_family to resolve names.
 --
 -- If both <code>"bytes"</code> and <code>"lines"</code> are provided,
 -- <code>"lines"</code> takes precedence. If neither are given, the functions
@@ -64,6 +65,13 @@
 
   sock:set_timeout(connect_timeout)
 
+  if type(host) == "string" and opts.any_af then
+    local status, addrs = nmap.resolve(host)
+    if status then
+      host = {ip = addrs[1], targetname = host}
+    end
+  end
+
   local status, err = sock:connect(host, port, opts.proto)
 
   if not status then

[nnposter]

Please let me know if you have any questions or concerns with the broader patch, which is hopefully aligned with Dan's feedback. Otherwise I will combine and commit both patches in about a week.

[dmiller-nmap]

Looks good!

[nnposter]

Committed as 99fa808 (I forgot to tag the commit message.)

P.S. I did look for any legacy script code that invokes nmap.resolve() and could therefore benefit from this change but all these invocations specify the address family explicitly.