Re: Boolean Operators for --script (again)

[Patrick Donnelly <batrick () batbytes com>]

Hi David,

On Fri, Apr 24, 2009 at 6:37 PM, David Fifield <david () bamsoftware com> wrote:
> I don't have a problem with the implementation method of converting to a
> Lua string and executing it. It gets us "and", "or", and "not" for free.
> This works:
>        nmap --script="all and not (telnet-brute or sql-injection)"
> I noticed some corner cases where the code behaves unexpectedly and I
> have some suggestions.
>
> First, why allow nil, false, and true?

I had no particular use in mind. Keeping them felt appropriate.

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

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

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

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

> Checking for duplicate files got changed:
>        nmap --script="telnet-brute,telnet-brute.nse"
> runs two copies to telnet-brute.nse.

I will correct this.

> I attached a patch that addresses some of these issues. The main change
> as I see it is using the Entry function to preprocess script.db into a
> nice table, allowing better factoring of the script selection code and
> not requiring a new script.db. Other changes:
>  * Removes "true", "false", and "nil".

I don't really think this is necessary, but I'm fine with this change.

>  * The code acts like the "version" category doesn't exist if -sV isn't
>   given. That prevents any version script from being matched but makes
>   the error message worse: "'version' did not match a category,
>   filename, or directory".

As I noted above, I think we should just remove the restriction on
choosing version scripts unless someone can explain why we did that in
the first place

>  * Wildcard matches are anchored at the beginning and end, so that "a"
>   doesn't match "asn-query.nse".

This is good.

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

> Thanks for working on this. Your idea for how to implement it was a good
> one.

Thanks for the feedback. My only disagreement with your patch is the
(unnecessary) complexity introduced by doing post-processing on the
script database and the remodeling of the code that incidentally
causes the recreating of many invariants such as the
protected_lua_tokens table for each script entry.

[1] http://seclists.org/nmap-dev/2009/q2/0102.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