Unclear HTTP redirect check for looping #830

nnposter · 2017-04-05T23:50:42Z

One of the default HTTP redirect checks, located here, is to prevent a redirect onto itself:

  -- make sure we're actually being redirected somewhere and not to the same url
  function (url, host, port)
    -- path cannot be unchanged unless host has changed
    -- loc.path must be set if returning true
    if ( not url.path or url.path == "/" ) and url.host == ( host.targetname or host.ip) then return false end
    if not url.path then return true end
    return true
  end,

There is a discrepancy between a comment and the actual code about whether the path is required or not. I presume that the comment is correct and the code is not, in which case the following patch remediates the issue:

--- a/nselib/http.lua
+++ b/nselib/http.lua
@@ -1514,10 +1514,12 @@
 
   -- make sure we're actually being redirected somewhere and not to the same url
   function (url, host, port)
+    -- url.path must be set if returning true
     -- path cannot be unchanged unless host has changed
-    -- loc.path must be set if returning true
-    if ( not url.path or url.path == "/" ) and url.host == ( host.targetname or host.ip) then return false end
-    if not url.path then return true end
+    -- TODO: Since we do not know here what the actual old path was then
+    --       the effectiveness of this code is a bit unclear.
+    if not url.path then return false end
+    if url.path == "/" and url.host == (host.targetname or host.ip) then return false end
     return true
   end,
 }

This patch deserves scrutiny from other folks before committing.

Varunram · 2017-04-06T10:25:21Z

I seem to understand the if not url.path then return false end part of the patch. But I wonder whyurl.host == ( host.targetname or host.ip) would be needed since if we are present at "/" it is implied that we don't have a redirect URL to go to. Your thoughts @nnposter ?

nnposter · 2017-04-06T13:40:44Z

I am not sure I follow your note, namely "if we are present at "/" it is implied that we don't have a redirect URL to go to". Could you please elaborate?

This part of the code is carried forward from the original commit so I am only working off of an assumption of its full intent. The real net effect of the code has been to stop following redirects onto the root of itself.

Condition url.host == ( host.targetname or host.ip) is there to properly scope this undesirable looping. If it was not there then the rule would reject other redirects, such as from //www.foo.com/ to //login.foo.com/.

Varunram · 2017-04-06T16:07:16Z

I had doubts regarding the redirection part of it but your wonderful explanation cleared it up. Thanks!

nmap-bot closed this as completed in cfa5775 Apr 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unclear HTTP redirect check for looping #830

Unclear HTTP redirect check for looping #830

nnposter commented Apr 5, 2017

Varunram commented Apr 6, 2017 •

edited

nnposter commented Apr 6, 2017

Varunram commented Apr 6, 2017

Unclear HTTP redirect check for looping #830

Unclear HTTP redirect check for looping #830

Comments

nnposter commented Apr 5, 2017

Varunram commented Apr 6, 2017 • edited

nnposter commented Apr 6, 2017

Varunram commented Apr 6, 2017

Varunram commented Apr 6, 2017 •

edited