[NSE] nmap library problems

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

I've found some problems with the nmap library while trying to clean
up some of its code in nse_nmaplib.cc. There are parts inconsistent
with the documentation:

1) nmap.print_debug_unformatted does not print output when the
specified (argument) verbosity is equal to o.verbosity. I fixed this
in r11303

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

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:

"Creates a new exception handler.

This function returns another function, an exception handler. The
returned function takes a variable number of arguments, which are
assumed to be the return values of another function. It checks the
return values for an exception, which is signaled when the first
return value is false. If there is an exception, the script will stop
immediately and produce no output. An optional handler function can be
called before the script is stopped. In the error handler function you
can perform any clean-up operations, such as closing a socket."

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

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.

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.

6) I moved l_clock_ms from nse_nsock.cc (where it was oddly placed)
over to nse_nmaplib.cc

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

There may be scripts that previously relied on the erroneous behavior
of the current library implementation. I have made every effort to
find such scripts and have found none. For those who wish to help to
test the patch, I would appreciate rigorous testing of the
get_port_state, set_port_state, and set_port_version functions.

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

Attachment: nse_nmaplib.patch
Description:

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