Re: [PATCH] showHTMLTitle.nse - bugfix and a few improvements

[David Fifield <david () bamsoftware com>]

On Wed, Sep 24, 2008 at 02:38:52AM +0100, jah wrote:
> The last time I sent a patch for showHTMLTitle I introduced the ability
> to follow a redirect from the default page.  If the server sends a
> location header which is not an absolute URI (as per HTML 1.1 spec) a
> print_debug statement fails, killing script execution.  My fault, I
> should have seen this coming.  The attached fixes this by handling
> relative URIs and making the script much more careful about handling the
> location header.

This looks like a worthwhile improvement.

Two different mechanisms for getting the redirected page are used. With
an absolute URI, it's
        data = http.get_url( loc )
and with a relative URI it's
        data = http.get( host, port, loc )
Could that be simplified, maybe by having the two cases assign temporary
variables representing the new host, port, and loc, then having a common
http.get outside of the if block? It doesn't seem right to parse the
URL, reconstruct it, and have http.get_url parse it again.

> The pattern to match the title tags is less greedy which means that,
> should there be more than one title in a response (which happens quite
> often, would you believe), it will return the first of them rather than
> everything between the opening of the first and the closing of the last.
>
> The pattern will also match title tags if they should have attributes of
> some form.

Those changes look good. I'm sure there are still ways to fool it, like
putting title tags inside a comment, or putting a '>' in an attribute to
the title. But we have to draw the line for how much of HTML we're
willing to parse.

> One thing that I didn't do because I thought it was overkill (?) was to
> use the dns library to check whether a hostname in a location header
> resolves to the target IP when the hostname does not match either
> host.targetname or host.name.  Instead, it prints debug info.

Yeah, that's a bit much.

What's the reason for this and similar changes?

-       if data.status == 301 or data.status == 302 then
+       if type( data ) == "table" and type( data.status ) == "number" and tostring( data.status ):match( "30%d" ) and 
type( data.header ) == "table" and
type( data.header.location ) == "string" then

Can the response from http.get be anything other than a table? If so,
maybe that should be changed in the http library rather than forcing
scripts to cope with different return types. I find the new condition
less expressive than the old one. What about
       if data.status ~= nil and tostring( data.status ):match( "30%d" ) and data.header.location ~= nil then
At least the very long line should be factored out into an is_redirect
function or something.

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org