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

Broken HTTP redirect check for host/domain #829

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

Broken HTTP redirect check for host/domain #829

nnposter opened this issue Apr 5, 2017 · 0 comments

Comments

@nnposter
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant