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

Unclear HTTP redirect check for looping #830

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

Comments

Projects
None yet
2 participants
@nnposter

nnposter commented Apr 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Varunram

Varunram Apr 6, 2017

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 ?

Varunram commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nnposter

nnposter Apr 6, 2017

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/.

nnposter commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Varunram

Varunram Apr 6, 2017

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

Varunram commented Apr 6, 2017

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

@nmap-bot nmap-bot closed this in cfa5775 Apr 19, 2017

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