Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTP option any_af breaks TLS SNI #708

Closed
nnposter opened this issue Feb 26, 2017 · 6 comments
Closed

HTTP option any_af breaks TLS SNI #708

nnposter opened this issue Feb 26, 2017 · 6 comments

Comments

@nnposter
Copy link

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.
@nnposter nnposter mentioned this issue Feb 27, 2017
Closed
@dmiller-nmap
Copy link

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
Copy link
Author

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
Copy link
Author

nnposter commented Mar 1, 2017 •
edited

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
Copy link
Author

nnposter commented Mar 4, 2017

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
Copy link

Looks good!

@nnposter
Copy link
Author

nnposter commented Mar 5, 2017

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.

@nnposter nnposter closed this as completed Mar 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nnposter @dmiller-nmap