Re: [NSE] A Lua implementation of NSE--detailed review

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

On Fri, Jan 16, 2009 at 10:03 PM, David Fifield <david () bamsoftware com> wrote:
> > NSE is started via the open_nse (and closed using close_nse) procedure
> > in nse_main.cc. This function begins by opening all standard Lua
> > libraries and adding all Nmap standard C libraries to package.preload
> > (to be required in nse_main.lua).
>
> I was confused by all the places the standard NSE library is loaded.
> (I mean pcre, bit, openssl, etc.) It happens
>  * at the top of nse_main.lua,
>  * in the C function init_main, and
>  * twice in the C function script_updatedb (once through
>   set_nmap_libraries and once inside the Lua code embedded in a
>   character array.
> Is it necessary at each of these places? Will they all need to be
> updated when a library is added or removed? What does script_updatedb
> need, for example, with the pcre and openssl modules?

Loading each library function in the package.preload table allows
libraries to be reloaded if needed and manually calling require in C
will not yield any real benefits over doing it in Lua.

The script database loads the libraries because when a script is
loaded to inspect its category table it may require these libraries
(many do).

> > nse_main.lua ends by returning a function that scans a host group. NSE
> > saves this function in the Registry for when NSE actually begins
> > scanning. This ends the initialization stage.
>
> I found nse_main.lua pretty easy to follow. I would like to see more
> function-level comments on functions like check_rules, get_chosen_scripts,
> run, and especially the anonymous main function--the fact that it
> doesn't have a name means a block comment is especially important.

I'll try to get this added in soon.

> > Next nse_main.lua is loaded and called with the private nse library
> > (different from the one discussed in Section IV) and the array of
> > rules (--script) are passed as arguments. The private nse library is
> > used to access some functionality needed through C such as
> > keyWasPressed() or nsock_loop().
> >
> > When NSE (the returned function by nse_main.lua) is used to scan a
> > host group, a new nse library is created for scripts (replacing the
> > old one).
>
> This is confusing, to have one "nse" library with the functions
>  { fetchfile_absolute, dump_dir, nsock_loop, key_was_pressed, ref,
>    unref, updatedb, scan_progress_meter }
> and an unrelated "nse" library containing the functions
>  { new_thread, push_handler, pop_handler, get_host, get_port }.
> The internal library needs a different name.

I agree. How about "cnse" or similar?

> But apart from that, I think introducing a brand new "nse" library and
> API is too big a change. While there are no scripts that use it, can it
> be removed? I'm not opposed to adding them after the rest of the
> changeover, but it should be after some API discussion and perhaps they
> should be in the "nmap" namespace.

I have no problems postponing adding the nse API. I felt it would be
useful because, as I understand it, the smb library would,
immediately, greatly benefit (nse.push_handler and nse.pop_handler).

I disagree with adding the functions to the nmap namespace. I feel
functions that directly manipulate or inspect nmap data (such as
targets and ports) belong there. Functions that affect how NSE or
scripts operate belong in a separate library.

> I think the implementation makes the idea of timeout clocks intrude
> too much upon the basic management of coroutines though.

It seems like a necessity to me.

> There are only two places (out of ten) that I found where nse_prepare_yield
> is called with charge set to false. Would it be possible to just stop
> the timeout clock in C++ code in those two places? Then always make sure
> the clock is restarted when a coroutine is resumed. The management of
> script coroutines is justifiably complex; adding timeout management to
> those parts of the code makes it harder to understand.

A host no longer needing to be charged (the thread blocking a mutex)
does not necessarily mean the timeout clock should be stopped. This
determination can only be made in the Lua mainloop. Moving this logic
to C++ would be very difficult (in both the current and Lua
implementations).

> I have trouble understanding this, even with your explanation. Does it
> mean NSE C++ code has to do this
>    udata->yield.thread_index = nse_prepare_yield(L, 1);
>    udata->yield.thread = L;
>    return lua_yield(L, 1);
> every time it wants to yield? I see in a few places it's a little more
> complicated than that. Is it possible to bundle the similar code into an
> nse_yield function? (That name would have a nice congruence with the
> magic NSE_YIELD value.) A little extra complexity is tolerable to solve
> this problem though.

The caller of nse_prepare_yield needs the integer it returns for
internal storage. Consider it the unique identifier for the NSE-level
thread. We can't know how the caller will save this integer for when
the thread is moved to running by nse_restore, so, preparing to yield
the thread is all we can really do.

The nsock library has callbacks that are called at arbitrary points in
time. These callbacks will not necessarily have access to a currently
running thread. So we pass a struct via the nsock userdata void * (not
a Lua userdata) for callbacks. The struct saves a thread and the
integer returned by nse_prepare_yield. This yield structure is:

struct yield {
  int thread_index; /* location in registry for thread, see nse_prepare_yield */
  lua_State *thread;
} yield;

which is actually in the nsock userdata (the Lua userdata used to
represent sockets):

struct l_nsock_udata {
    /* ... */
    struct yield yield; /* <---  */
    /* ... */
};

We must pass the pointer to this yield structure to nsock library for
any callbacks. The nsock library is unique in that it needs to save a
lua_State * thread for its callback routines in order to call
nse_restore.

> > == nse.new_thread(func, ...) ==
> > A coroutine run manually by a script thread will propagate the yield
> > through the parent back to NSE. To avoid this, you may use
> > nse.new_thread to create a thread which is autonomous from the parent.
> >
> > The function launches a new thread (coroutine) that is managed by NSE.
> > It inherits the host and port of the parent thread but these values
> > are not passed to func. Instead the extra parameters to new_thread are
> > passed. All errors are ignored and return values discarded by NSE.
>
> Does this mean that if I just do a plain yield in a coroutine from a
> script it gets propagated to NSE (with the NSE_YIELD mechanism)?

No, a yield in a child coroutine (that is, a coroutine being resumed
by an NSE thread), will only propagate to NSE if NSE yielded the
thread.

> I thought that only happened with nsock/mutex-type operations.

Right.

> I think I
> would really prefer it to be that way. If it's not, Lua programmers will
> have to relearn some concepts: an NSE coroutine is not a normal Lua
> coroutine; if you want a normal coroutine, you have to use nse.new_thread.

No this is not correct. I'm sorry for the confusion. nse.new_thread
actually spawns a thread that is _not_ managed by the NSE thread which
created it. NSE will resume the new thread just like any other NSE
level thread. The difference is all results and errors are ignored
(nothing is reported). Perhaps an example will make this more clear.
Using the cotest.nse script I described in my original post:

<file = "cotest.nse">

author = "patrick"
description = "coroutine test!"
categories = {"default"}

require "shortport"

portrule = shortport.port_or_service(22, "ssh")

function a (host, port)
 local try = nmap.new_try();
 local socket = nmap.new_socket();
 try(socket:connect(host.ip, port.number));
 return "connected!";
end

function action (host, port)
 local co = coroutine.create(a);
 print(coroutine.resume(co, host, port));
 print(coroutine.resume(co));
 return "done";
end
</file>

As it is now, the first resume will not catch the yield NSE does in
socket:connect(). You can see that in the printed results:

<output>
batrick@waterdeep:~/nmap/svn/patrick/nse-lua$ ./nmap --script
cotest.nse localhost

Starting Nmap 4.76 ( http://nmap.org ) at 2008-12-30 03:55 MST
true    connected!
false   cannot resume dead coroutine
Interesting ports on localhost (127.0.0.1):
Not shown: 998 closed ports
PORT     STATE SERVICE
22/tcp   open  ssh
|_ cotest: done
3690/tcp open  unknown

Nmap done: 1 IP address (1 host up) scanned in 0.14 seconds
</output>

Because the coroutine never _manually_ yielded anything, the returned
value, "connected!" is all that is printed. The second resume actually
fails because the coroutine is dead.

A different script which spawns a new thread using nse.new_thread:

<file="cotest2.nse">
author = "patrick"
description = "coroutine test!"
categories = {"default"}

require "shortport"
local nse = require "nse"

portrule = shortport.port_or_service(22, "ssh")

function a (host, port)
  print("starting child 'a'", os.time())
  local try = nmap.new_try();
  local socket = nmap.new_socket();
  try(socket:connect(host.ip, port.number));
  print("starting child 'a'", os.time())
  return "connected!";
end

function action (host, port)
  print("starting main thread", os.time())
  local co = nse.new_thread(a, host, port);
  print("main thread created child", os.time())
  os.execute("sleep 2");
  print("main thread done", os.time());
  return "done";
end
</file>

<output>
batrick@waterdeep:~/nmap/svn/patrick/nse-lua$ ./nmap --script
cotest2.nse localhost

Starting Nmap 4.76 ( http://nmap.org ) at 2009-01-17 09:46 MST
starting main thread    1232210797
main thread created child       1232210797
main thread done        1232210799
starting child 'a'      1232210799
starting child 'a'      1232210799
Interesting ports on localhost (127.0.0.1):
Not shown: 998 closed ports
PORT     STATE SERVICE
22/tcp   open  ssh
|_ cotest2: done
3690/tcp open  unknown

Nmap done: 1 IP address (1 host up) scanned in 2.15 seconds
</output>

You can see that the results from the new child thread are discarded
and it executes independently of the parent.

Of course the parent thread will probably wait for its children to
finish to collect results by using a condition variable rather than
exiting immediately. I hope this cleared up how it works.

> > == nse.push_handler(func) ==
> > This function pushes func on a handler stack. If the thread ends
> > normally or aborts due to an error, all functions on the handler stack
> > are called from the top of the stack down.
> > == nse.pop_handler() ==
> > This function pops a function from the thread's handler stack.
>
> These function names are too generic. Maybe name them after C's atexit,
> or something else to convey that they are exit handlers.

I was using pthread cleanup handlers for inspiration on this one.
Perhaps I should have used nse.cleanup_push and nse.cleanup_pop like
they did [1]. I wonder if I should also add the execute option to the
pop handler like they have for pthreads, it would certainly be
convenient.

> I like the general idea of providing safe access to Nmap's data
> structures from NSE, but the example of hosts and ports is not
> compelling. This kind of information is already made available in the
> port and host tables. It creates confusion if there is more than once
> source for the same information. For example, should I say
>  host.name
> or
>  host:HostName()
> It depends if "host" is a host table or a host object. What's more
> confusing is that if "host" is the current host table, and I do
>  host.name = "blah"
> the host.name and nse.get_host():HostName() aren't even the same
> anymore.

I originally intended to set it up so that the userdata would replace
the tables, but there would be problems with complexity and backwards
compatibility. The main motivation for the userdata approach is NSE
will have easy access to the host and ports for each thread. In
particular, timeouts and script output are more easily set. We could
_not_ expose these userdata to the scripts and make the totable()
method do all the work (that is, remove the other bound methods such
as HostName()). I got the impression though that users want access to
the internal data for these Targets and Ports [1], and userdata with
methods seemed like the best approach. While the "access" is mostly
redundant considering the garbage tables, the new userdata allow us to
easily bind other methods a script writer may need.

> I would like this part of the new code to be held back and have some
> further design. For one thing, it would be nice to fix the defect of
> Nmap's inconsistent method name capitalization in the interface we
> present to script writers.

Ok.

> I was thinking, maybe there's a way to preserve compatibility with
> presenting hosts and ports as tables, while also providing safe access
> to internal data structures and a really slick way to set host and port
> data. As it is now, the host and port tables are just dumb copies of
> Nmap's data structures; if you modify them ("blah"), nothing else takes
> any notice. What if instead of making copies, the table given to scripts
> had an __index that hooked into Nmap code to get the data? So you would
> say "host.name" and Nmap would internally make the Target->HostName()
> call.

This is doable and I considered it initially but what if they change
the table (__newindex)? How should that be reflected? There are ways
to approach this but the current system gives each script "garbage"
tables to use and suddenly giving them more meaning could be bad.
Personally, I would like to see host and port tables replaced with
these userdata but with an __index metamethod like the one you
describe. Some scripts change the port tables though and pass them to
nmap.set_port_state so this would not be compatible.

> This avoids having to create a new parallel API, and could also be a
> better way to set host and port information than exists currently.
> Whereas now to set the service name of a port you have to do
>  nmap.set_port_version(host, { number = 1234, protocol = "tcp", name = "foobar" }, "hardmatched")
> it could become as easy as
>  port.version.name = "foobar"

That would certainly be nicer but there is a lot of code that would
probably need to be implemented in C to get that "magic" to work.

I'm glad you see the opportunity to clean up the current way ports and
hosts are manipulated. Unfortunately, it is difficult to find the best
way to approach this. For this reason, I kept the current system of
passing garbage tables and added userdata which could add extra
functionality.

> If anyone wants to follow up on this we should probably start a separate
> thread. But anyway I think this part of the code bears more discussion
> so I'd like it help back from the integration. I see that you use some
> of these methods in nse_main.lua; I hope it won't be too hard to change
> them to the table representation or find another way to replace them.

We can keep the userdata as they are now and just not expose them to
scripts through the nse API (for now). If we _really_ want to reduce
the complexity we can remove most of the methods and actually do all
the work in the totable() method. The totable() method would become
similar to the set_hostinfo and set_portinfo tables that are in the
main branch nse_nmaplib.cc library (perhaps poorly placed). I don't
see too much benefit in doing that however.

> That's all I have to say, for this post anyway. You don't have to start
> tearing apart your code just yet; I want to allow time for more opinions
> than just mine. By the way, I know that when I get into code review mode
> I sometimes come across as more critical than I intend to be. I do think
> this is a fine bit of code you've developed, and from what I've seen
> worthy of inclusion.

Thanks!

[1] http://opengroup.org/onlinepubs/007908799/xsh/pthread_cleanup_pop.html

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