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

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

On Sun, Jan 18, 2009 at 11:03 PM, David Fifield <david () bamsoftware com> wrote:
> On Sat, Jan 17, 2009 at 10:28:02AM -0700, Patrick Donnelly wrote:
> > 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).
>
> Maybe I misunderstand. Don't the scripts bring in those libraries
> already? I deleted those "require" lines from script_updatedb and
> nse_main.lua (attached nse-lua-norequire.diff). Then ran
> "./nmap --datadir=. --script-updatedb":
>
> NSE: Updating rule database.
> NSE: error while updating Script Database:
> [string "local nse = ......"]:15: ./nselib/base64.lua:93: attempt to index global 'bin' (a nil value)
> stack traceback:
>        [C]: in function 'assert'
>        [string "local nse = ......"]:15: in main chunk
>
> However the error appears to be in base64.lua, which doesn't require the
> bin library even though it uses it. Adding a require line to base64.lua
> and trying to update the database again gives
>
> NSE: Updating rule database.
> NSE script database updated successfully.
>
> With those changes, there's only one piece of code that needs to be
> changed when a new C library is developed, which is the function
> set_nmap_libraries.

Many scripts use the nmap library without explicitly requiring it. It
is probably lucky chance that some script required nmap before another
used it. For example, banner.nse requires the nmap library but
asn-query.nse will use it without requiring it.

I'll change set_nmap_libraries to also require each library manually
so we don't need to update in multiple places.

> > > > 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?
>
> I would give it an ugly name like nse_internal to emphasize that it is a
> purely internal, glue binding to certain Nmap functions. "cnse" is fine
> too.

Alright.

> > > 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 see now why the determination can't be made at the level of an
> individual thread; each host may have more than one thread running and
> its timeout clock is stopped only when none of its threads are being
> charged time. (Are these the threads created with nse.new_thread?)

Most of the threads are made by Scrip:new_thread(). nse.new_thread
just allows you to spawn additional NSE level threads.

> So it's just like running another script, except that it can't produce
> any output; presumably it's doing work for whatever script created it.

Exactly.

> How do you see this facility being used?

Scripts that need to do a lot of networking tasks simultaneously. This
is the actual intended purpose, but a nice bonus is a script can now
make a worker thread that may error without causing the parent to quit
as well.

> > > > == 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.
>
> Yeah, those are better names. Don't worry about adding the execute
> parameter until there's a demonstrated need for it. Ron mentioned these
> would be useful to clean up mutexes.

Alright I'll make this change too.

> Did you have another use ni mind for them?

The main purpose is to give scripts the ability to handle errors since
they cannot use pcall and then yield (through a network operation,
mutex, etc.). An error will still cause their script to immediately
end but at least they can clean up.

> I'll follow up on the host/port userdata thing in a different thread.

Ok.

Thanks,

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