Re: [NSE] Better Handling for Require Errors

[Patrick Donnelly <batrick () batbytes com>]

On Thu, Jun 9, 2011 at 7:00 PM, Fyodor <fyodor () insecure org> wrote:
> On Wed, Jun 08, 2011 at 01:11:53PM -0400, Patrick Donnelly wrote:
> > > Running with -d shows a stack trace and makes it more obvious what the
> > > problem is ("module 'ipops' not found"), but can we do anything about
> > > this?
> >
> > Would artificially upping the debug level be acceptable when updating
> > the database?
>
> Since I'm not 100% sure I understand all the details, I'm going to
> back up for a moment and note the initial problem...
>
> There were scripts which required certain libraries or privileges
> which were failing when run without those.  There were two main
> offenders: scripts which would "require "openssl"' even though some
> people compile Nmap without that library, and scripts which required
> root privileges.
>
> The initial fix for this was a case-by-case system where we basically
> had a pcall template for each of those which we would paste into
> scripts where necessary.  They looked like:
>
>  if pcall(require,"openssl") then
>   require("tns")
>  else
>   portrule = function() return false end
>   action = function() end
>   stdnse.print_debug( 3, "Skipping %s script because OpenSSL is missing.",
>       SCRIPT_NAME)
>   return;
>  end
>
> and:
>
>  if not nmap.is_privileged() then
>    if not nmap.registry['firewalk'] then
>      nmap.registry['firewalk'] = {}
>    end
>
>    if nmap.registry['firewalk']['rootfail'] then
>      return false
>    end
>
>    nmap.registry['firewalk']['rootfail'] = true
>
>    if nmap.verbosity() > 0 then
>      nmap.log_write("stdout", SCRIPT_NAME .. ": not running for lack of privileges")
>    end
>
>    return false
>  end
>
> Your (Patrick) patch eliminated these bloated sections by (if I
> understand correctly) allowing scripts to silently cease running if a
> require fails rather than printing a big backtrace and existing Nmap.
> You also root a 'root' library people could 'require' if they need
> root access.  This reduced the blobs above to just:
>
> require "openssl"
>
> and
>
> require 'root'
>
> That looks a lot better!

Your analysis and understanding is correct.

>  But the issue is that this silent failure
> behavior can mask actual errors.  This discussion is about a case
> where someone miscapitalized ipOps.lua.  That caused script-updatedb
> to fail with a strange and unhelpful error message.  The errors caused
> from trying to run such scripts directly is the same.  When I change
> 'ipOps' to 'ipops' in http-tital.nse, I get:
>
> $ ./nmap -p80 --script scripts/http-title nmap.org
>
> Starting Nmap 5.52.IPv6.Beta2 ( http://nmap.org ) at 2011-06-09 15:52 PDT
> NSE: failed to initialize the script engine:
> /home/fyodor/nmap/nse_main.lua:603: attempt to index local 'script' (a nil value)
> stack traceback:
>        /home/fyodor/nmap/nse_main.lua:603: in function 'get_chosen_scripts'
>        /home/fyodor/nmap/nse_main.lua:1030: in main chunk
>        [C]: ?
>
> QUITTING!

This is actually a "bug". Script.new was designed to return nil in the
case of a silent error (whatever that may be; turns out the "silent
require" patch was the first use-case for that). I fixed this bug in
r23911.

> I'm not sure what the error looked like before the silent require
> change.  It might have been just as vague.  In both cases
> (--script-udpatedb and trying to run the broken script), I get the
> more informative error message (noting the failure to find ipops.lua)
> in -d mode.  So if the solution is increasing debug level, we should
> probably do so in general for this failed to initialize the script
> engine error rather than for --script-updatedb specifically.

Well, if we up the debug level everywhere then we have the error
message all the time (even when we wanted it "silent"). The reason
script writers used pcall was to avoid printing error output to users
that would get confused (did the script break?). The goal of this
patch was to make that unnecessary and to obsolete the hack being used
to get around not being able to run (empty action/hostrule).

As I see it we can do one of three things:

(a) Go back to the old system.
(b) Always print an error message.
(c) Accept that require may be silenced for some border cases where we
don't want it silenced. Script authors should be testing with -d...

I'm partial to (c). I don't really like (a). I think (b) defeats the
entire point.

I'm open to suggestions...

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