Re: [NSE] nmap library problems

["Patrick Donnelly" <batrick.donnelly () gmail com>]

Hi David,

On Sun, Dec 28, 2008 at 10:52 PM, David Fifield <david () bamsoftware com> wrote:
> Good eye on this one. nmap.print_debug_unformatted gets away with
> checking verbosity because verbosity is increased whenever debugging is
> increased; debugging is always less than or equal to verbosity (except
> perhaps as a result of runtime interaction).
>
> That said, your redesign is a good one. Go ahead and make that change
> when you wish. Except don't worry about print_verbose. Scripts shouldn't
> print anything other than debug output directly to the terminal.

Ok sure.

> > 3) nmap.newtry(), when passed no catch handler function, returns a
> > function which does nothing for all inputs. This is inconsistent with
> > the documentation which requires it raise an error if the first
> > argument is false. The catch function is called only before the error.
> >
> > Additionally, the documentation states:
> >
> > The script "will stop immediately and produce no output". This is very
> > misleading because an error can be caught by the user script. I
> > believe the documentation should be more clear in that: If the first
> > argument is true (or casts to true) then the handler shall return all
> > but the first argument passed to it, otherwise, it shall run the catch
> > handler passed in nmap.newtry(), if present, and raise an error on the
> > second argument (usually a string describing the error).
>
> Good point. The documentation was not clear enough. I have improved it
> following your suggestions. What do you think of r11537?

Looks good to me.

> > 5) It seems the intention of nmap.set_port_version was that if the
> > third argument is omitted then the probestate is PROBESTATE_INITIAL.
> > This is evidenced by:
> >
> >   luaL_checktype(L, 3, LUA_TSTRING);
> >   char* c_probestate = strdup(lua_tostring(L, -1));
> >   lua_pop(L, 1);
> > /* ... */
> >   if(c_probestate == NULL)
> >     probestate = PROBESTATE_INITIAL;
> > /* ...*/
> >
> > However c_probestate is guaranteed to not be NULL because the third
> > argument is checked to be a string. I have changed it so that the
> > string "initial" switches to PROBESTATE_INITIAL.
>
> Let's forget about "initial" (can't see that being useful) and make the
> default "hardmatched".

Ok done.

> > 6) I moved l_clock_ms from nse_nsock.cc (where it was oddly placed)
> > over to nse_nmaplib.cc
>
> Not only that, the old clock_ms seems to have been way broken. With a
> simple script that just calls clock_ms and returns, I got the results
>
> |_ ms: -2129219375
> |_ ms: -2129210778
> |_ ms: -2129197203
>
> The cause was an easy-to-miss integer overflow. Your new code is
> correct, more concise and understandable, but its behavior has changed
> in an significant way: it no longer returns only integers:
>
> |_ ms: 1230526530786.8
> |_ ms: 1230526536528
> |_ ms: 1230526543972.5
>
> This may be good or bad, but have you tested it with existing scripts?
> It only appears to be used in snmp.lua. From a perusal of the code it
> looks like there is no problem because snmp rounds to an int itself, but
> I would like you (or anyone) to test it.

I changed it to return the ceiling of the result.

> > In total, I have cleaned up a lot of the code in the nmap library by
> > shortening code into equivalent sections or correcting code to
> > properly handle any number of arguments. A good example of improper
> > use of arguments is nmap.fetchfile which would use the last argument
> > passed to it (it assumes only one argument is passed) is the filename
> > string to "fetch". The difference in lines of code is almost 100 lines
> > (652 -> 561).
>
> Those refactoring changes are great. Nice job of improving the code.
> It's easier to review, though, when little cleanup changes are separated
> from changes that potentiall change behavior, like the get_port_state
> and clock_ms changes.

Ya sorry about it ending up being lumped in one big patch.

> You changed existing behavior in a significant way in newtry_finalize
> (formerly l_exc_finalize) too. l_exc_finalize handled an exception on
> false or nil, continued successfully on true (and only true), and quit
> on any other value. newtry_finalize treats any value that is not false
> or nil as true. I can see this causing problems for a programmer new to
> newtry, especially as is is different from Lua's assert in the true
> case. Someone might wrongly return just a host name and a port number on
> success, say, instead of returning true first. With l_exc_finalize Nmap
> would crash with an error message (an easy bug). With the new
> implementation the script's host name would be a number and the port
> number would be nil (a harder bug).

I changed it to print an error message if the first value is not a
boolean. Having the script cause Nmap to exit is too strict in my
opinion.

> So to summarize, you're good to apply this patch, with these changes:
>        print_verbose (not needed)

Ok I removed the commented print_verbose function and did not add
stdnse.print_verbose.

>        "initial" in nmap.set_port_version (use "hardmatched" instead)

Ok I removed initial and changed the default to hardmatched.

>        clock_ms (make it return integers only, then start a thread on
>                having it return non-integers if you want)

Integer is fine for me.

>        newtry_finalize (have it quit on anything other than true,
>                false, and nil. I'm open to changing my mind about it,
>                so anyone can start a thread on that too. I just don't
>                want the dicussion to hold up the rest of the patch)

Like I said above, having Nmap exit because of an inappropriate script
is too harsh but I did add a warning error message.

> Would you rename l_newtry to l_new_try to match its Lua name? Also get
> rid of the SCRIPT_ENGINE_GETSTRING function; your new code is better and
> doesn't use it.

Sure, I changed the name.

Thanks for reviewing the patch.

Cheers,

-- 
-Patrick Donnelly

"One of the lessons of history is that nothing is often a good thing
to do and always a clever thing to say."

-Will Durant

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