Broken HTTP redirect check for host/domain #829

Closed
nnposter opened this Issue Apr 5, 2017 · 0 comments

Comments

Projects
None yet
1 participant

nnposter commented Apr 5, 2017

One of the default HTTP redirect checks, located here, is intended to "approve" a redirect if the destination the same host or at least a domain:

  function (url, host, port)
    local hostname = stdnse.get_hostname(host)
    if ( hostname == host.ip and host.ip == url.host.ip ) then
      return true
    end
    local domain = hostname:match("^[^%.]-%.(.*)") or hostname
    local match = ("^.*%s$"):format(domain)
    if ( url.host:match(match) ) then
      return true
    end
    return false
  end,

There are several issue with this implementation:

  • url.host is a string, not a table with ip as a member. As a result the condition to "approve" a redirect when the IP addresses match (host.ip == url.host.ip) would never fire.
  • The redirect is not immediately rejected when the current hostname and the destination are different IP addresses. The code instead proceeds with a domain-matching logic, which effectively means that two IP addresses are considered a match if they differ at most in the first octet while the remaining three are identical.
  • The domains should not be compared case-sensitively.
  • The domain-matching logic has a weakness in that it does not treat the current location and the destination symmetrically. As an example, it "approves" a redirect from www.foo.com to www.test.foo.com but not the opposite direction.
  • The logic approves unwarranted redirects, such as foo.com to bar.com (because they share the most specific domain portion, which in this case is .com) or www.bank.com to my.fakebank.com (because the second name ends with bank.com)
  • The domain matching is using the domain suffix as a Lua pattern, which is problematic when the domain contains a dash or, quite naturally, a dot. As an example, f-bomb.org can be redirected to bomb.com or foo.bar.rink.net to bardrink.net.

The following patch resolves these issues:

--- a/nselib/http.lua
+++ b/nselib/http.lua
@@ -1485,15 +1485,12 @@
   -- Check if the location is within the domain or host
   function (url, host, port)
     local hostname = stdnse.get_hostname(host)
-    if ( hostname == host.ip and host.ip == url.host.ip ) then
-      return true
+    if hostname == host.ip then
+      return url.host == hostname
     end
-    local domain = hostname:match("^[^%.]-%.(.*)") or hostname
-    local match = ("^.*%s$"):format(domain)
-    if ( url.host:match(match) ) then
-      return true
-    end
-    return false
+    local srcdomain = (hostname:match("%..+%..+") or hostname):lower()
+    local dstdomain = (url.host:match("%..+%..+") or url.host):lower()
+    return srcdomain == dstdomain
   end,
 
   -- Check whether the new location has the same port number

The following caveats apply:

  • Domain portions must match exactly. A redirect from www.foo.com to www.test.foo.com will be rejected in either direction.
  • The domain match must be at least two levels deep. Sharing a TLD is not good enough.
  • ccTLDs are not treated as such.

Please let me know if you have any questions or concerns. Otherwise I will commit the patch in a few weeks.

nmap-bot closed this in ab96f9c Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment