Re: [NSE] nmap library problems

[David Fifield <david () bamsoftware com>]

On Thu, Dec 11, 2008 at 08:43:14PM -0700, Patrick Donnelly wrote:
> I've found some problems with the nmap library while trying to clean
> up some of its code in nse_nmaplib.cc.

Hi Patrick. I'm sorry it took me so long to get to reviewing this patch.
Some of the smaller changes like rearranging the order of functions you
could have committed directly, and some others stand on their own and
could have been separate patches. That would have made the review
easier. I'm not going to complain about receiving code improvements
though, especially when they are as sensible and well-implemented as
these are.

> 2) nmap.print_debug_unformatted is not documented at all, possibly
> intentionally? I feel that this function could be reduced to
> nmap.print_log_stdout, a wrapper around:
>
> log_write(LOG_STDOUT, ...);
>
> We can use stdnse (as we apparently already are doing) to make
> stdnse.print_verbose() and stdnse.print_debug(), the latter already
> exists. However, there is confusion in that the current implementation
> of stdnse.print_debug() uses nmap.print_debug_unformatted which checks
> verbosity and not debugging. This seems very odd to me. I propose that
> following changes are made:
>
> o nmap.print_debug_unformatted is replaced with nmap.print_log_stdout
> (no scripts directly use the former, currently).
> o stdnse.print_verbose([level,] fmt, ...) is added. It uses
> nmap.verbosity() to check if the verbosity level is high enough and
> then prints using nmap.print_log_stdout.
> o stdnse.print_debug([level,] fmt, ...) works similarly to
> print_verbose() except uses nmap.debugging().

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.

> 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?

> 4) The functions nmap.get_port_state, nmap.set_port_version, and
> nmap.set_port_state all require that the calling function's name
> (checked via debugging facilities) must match the nmap library entry.
> That is, if one were to do:
>
> function action(host, port)
>   local f = nmap.get_port_state;
>   print(f(host, port))
> end
>
> The call to the local f will silently fail because the activation
> record on the stack will show the name of the function as "f" rather
> than "get_port_state". I have corrected this problem in the attached
> patch.

Those functions look much improved. Much clearer. Thanks.

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

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

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

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

So to summarize, you're good to apply this patch, with these changes:
        print_verbose (not needed)
        "initial" in nmap.set_port_version (use "hardmatched" instead)
        clock_ms (make it return integers only, then start a thread on
                having it return non-integers if you want)
        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)

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.

David Fifield

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