Re: Boolean Operators for --script (again)

[David Fifield <david () bamsoftware com>]

On Mon, Apr 27, 2009 at 04:41:02AM -0600, Patrick Donnelly wrote:
> On Fri, Apr 24, 2009 at 6:37 PM, David Fifield <david () bamsoftware com> wrote:
> > The new code requires a new format for script.db (you should have
> > mentioned that).
>
> I apologize that wasn't in my original post, but I did mention it in a
> follow-up posting [1].

Ah, sorry, I didn't see that.

> > Currently an entry looks like
> >        Entry { category = "discovery", filename = "banner.nse" }
> >        Entry { category = "safe", filename = "banner.nse" }
> > The patch changes it to
> >        Entry {
> >                filename = "banner.nse",
> >                categories = {
> >                        "discovery",
> >                        "safe",
> >                }
> >        }
> > I can understand why you did that, because you want to have all the
> > categories at once for each entry as you iterate through script.db. But
> > you can get the same effect without changing the format by preprocessing
> > script.db into a table in the form you want, and not doing all the work
> > in the Entry callback.
>
> The preprocessing is unnecessary at runtime and should be done during
> the --script-updatedb operation. We could have the script database
> return a large table including all the scripts (filenames +
> categories) and stop using the Entry function altogether. The current
> implementation creates a lot of garbage in memory, ϴ(nm), where n is
> the number of scripts and m is the average number of categories. If
> the size of the database gets large (very likely in the future), then
> this cost will make loading scripts more costly than it needs to be.
> There is no benefit to checking if a script will load in the Entry
> function but it can certainly be more convenient. There is also no
> dependency between scripts so there is no reason (other than the
> current structure of the database) to postpone determining whether a
> given script should load.
>
> The new database format will create fewer garbage tables than before
> (ϴ(n+n), n scripts, and n inner category tables) and works well for
> filtering scripts, with the new boolean operators, without
> necessitating the storage of previous entries in order to determine if
> a script should be loaded. For these reasons, I think we should
> proceed with using the database format in my patch.

Okay, you convinced me. script.db is a preprocessed cache anyway so we
should keep in whatever form is easiest to deal with. One small change:
can you put each Entry on a single line, so that it's easy to grep? Like

Entry {filename = "banner.nse", categories = {"discovery", "safe",}}

> > A side effect of having Lua execute the string is that we get Lua syntax
> > errors:
> >        $ nmap --script="a and and b" localhost -sP
> >        NSE: failed to initialize the script engine:
> >        ./nse_main.lua:355: [string "return __["a"] and and __["b"]"]:1: unexpected symbol near 'and'
> > Do you think we should pcall it and give a higher-level error message?
>
> We can output the original string "a and and b" instead of the arcane
> "./nse_main.lua:355: [string "return __["a"] and and __["b"]"]:1:", so
> it would look like:
>
> Bad script rules: "a and and b" -> unexpected symbol near 'and'
>
> Something like that?

Looks good to me.

> > Boolean expressions make trouble for our blacklisting of the "version"
> > category.
> >        nmap --script="safe,version"
> >        ./nse_main.lua:264: explicitly specifying rule 'version' is prohibited
> > while
> >        nmap --script="safe or version"
> > runs without complaint. What do you think we should do about that? I
> > don't really know why it's blacklisted anyway. I mean, you can always
> > list a "version" script by file name and it's not the end of the world.
>
> I noted this in the original post:
>
> "Categories are still case insensitive but the boolean operators must
> be lower case. The only possible problem with this new feature is one
> could explicitly specify "version", although I don't it as a big deal:
>
> ./nmap --script "not not version" localhost "
>
> I don't know the original intent of blacklisting version scripts.
> Because these scripts can be chosen for other categories, I think we
> should just remove that restriction altogether.

Again, sorry for not seeing your original explanation. I agree, let's
just get rid of the prohibition on version.

> >  * Instead of converting 'a' to '__["a"]', converts to 'm("a")'. The m
> >   function does matching of categories and wildcard names. I think it's
> >   clearer than using a metatable.
>
> Changing to a function call is ok with me.

All right, whatever you think is best. I think of each expression as
being a predicate that can be applied to a script entry, so a
representation as a function is natural.

Please make another version of the patch that
 1) Puts script.db entries on a single line,
 2) Fixes the duplicate running scripts (just a simple matter of
    updating files_loaded I think),
 3) Uses the user's typed expression in syntax error messages,
 4) Removes special handling of version, and
 5) Does whatever you think is best for textual substitution of
    expressions '__["a"]' 'm("a")'. That part of code is tricky so it
    needs good comments too.

Thanks Patrick.

David Fifield

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