Re: parsing of script-args is broken

[Patrick Donnelly <batrick () batbytes com>]

Hi Jah,

On Sun, Apr 26, 2009 at 5:20 PM, jah <jah () zadkiel plus com> wrote:
> Greetings,
>
> I haven't been able to pay as much attention lately as I would have
> liked, so apologies if this has in fact been addressed despite my
> efforts to find any mention of the problem.
>
> --script-args vhost=domain.co.uk is, I believe a perfectly legitimate
> argument for a script.  The intention is that the script can access the
> string assigned to vhost in the nmap.registry.args table.
>
> However, run this argument through loadstring() to check that the
> arguments are valid lua and to return a lua table like so:
>
> loadstring( 'return {vhost=domain.co.uk}' ) and what you would get back
> is: attempt to index global 'domain' (a nil value)
>
> instead of seeing the string "domain.co.uk" loadstring would see a lua
> value named "domain" and try to index the value with a lua value named "co".
>
> To prevent this in nse_main.lua, the script arguments string is passed
> through gsub() to place double-quotes around portions of it so that
> loadfile will instead interpret those portions as strings instead of
> global variables.  Trouble is, the pattern looks for "=([%w_]+)" which
> matches alphanumeric characters and the underscore and replaces them
> with "=\"%1\"".  So my vhost argument becomes vhost="domain".co.uk
> which, when passed to loadstring() as
> 'return {vhost="domain".co.uk}'
>  results in the error:  : '}' expected near '.'
>
> Hopefully it's clear that "=([%w_]+)" is insufficient to deal with all
> possible script arguments which, as I understand it, could be one or
> more name=value pairs where the value may be a string or a table [1].
> Either this method has to be made much more robust, or that replacement
> should be removed entirely and force users to ensure that, when
> providing script-args, each literal string in a name=value pair is quoted:
>
> --script-args " vhost='domain.co.uk' "
> --script-args " vhost={'domain.co.uk','domain.com'} "
>
> as long as the arguments are valid lua and the strings are quoted, then
> loadstring() will not error.
>
> I've made an attempt at improving the "stringification" of the string
> passed to loadstring() with the aim of allowing both the above examples
> and those in [1].  The attached patch:
> replaces any commas found within quoted (on windows: a single quote
> only) string literals in the script-args with the string "Ncomma".
> splits the resultant string, comma delimited
> quotes any trimmed, unquoted value if that value is not the key to
> another value (=> foo={bar="foobar"} )
> joins the resultant table and replaces any "Ncomma"s with their original
> comma.
>
> So, on windows, supplying --script-args:
> smbuser=admin,smbpass='P455,0rd',whois={whodb=nofollow+ripe},vhost={domain.co.uk,domain.com}
> the following will be passed to loadstring() :
> smbuser="admin",smbpass='P455,0rd',whois={whodb="nofollow+ripe"},vhost={"domain.co.uk","domain.com"}
>
> which I hope covers at least as many uses as --script-args currently gets.
>
> jah
>
> [1] - http://nmap.org/book/nse-usage.html#nse-args

Your problem raises valid concerns. I've taken a more detailed looked
at some of --script-args history and technical description and here is
what I've found:

Stoiko Ivanov's thread [1] describing --script-args shows that the
original functionality would allow:

   script-arguments (both key and values) may contain any characters,
   apart from '{', '}', '=' and ','

However, the original implementation neglected to allow this for keys
which lead me to suggest an alternative for Kris in [2]. Stoiko's code
also disallowed '$' for unknown reasons. It also allowed an '=' sign
so long as it followed an initial equal sign and preceded an invalid
character.

Last Summer, when I reviewed and rewrote much of nse_init.cc, I
changed the parsing of --script-args to allow '$'. I believe this was
the only significant change to the code.

When nse-lua was developed I significantly shortened the amount of
code (by writing it in Lua) needed for parsing the script-args. It is
short enough to include here:

do -- Load script arguments
  local args = gsub((cnse.scriptargs or ""), "=([%w_]+)", "=\"%1\"");
  local argsf, err = loadstring("return {"..args.."}", "Script Arguments");
  if not argsf then
    error("failed to parse --script-args:\n"..args.."\n"..err);
  else
    nmap.registry.args = argsf();
  end
end

For reasons I'm unsure of, I changed how the parsing was done to only
include alphanumeric characters and an underscore. As you showed, this
is inadequate.

To correct this, I have attached a small patch that correctly
implements (most of) Stoiko's original specified functionality. I have
however disallowed the use of spaces in any names. For example:

nmap --script-args " host = somehost "

will _not_ be interpreted as { [" host "] = " somehost " }

While that may not seem like a likely scenario, I could see people
using script-args written over multiple lines in shell scripts so,
naturally, including whitespace in names would be bad.

In my tests it works pretty well. Here is an "exhaustive" example of
arguments covering most use cases and the subsequent translation:

vhost=domain.co.uk,smbpass="P455,0rd",whois={whodb=nofollow+ripe},zone-transfer={pass=barbar}

nmap.registry.args =
{["vhost"]="domain.co.uk",["smbpass"]="P455,0rd",["whois"]={["whodb"]="nofollow+ripe"},["zone-transfer"]={["pass"]="barbar"}}

You'll notice that in smbpass="P455,0rd", the value is already quoted
so we leave it untouched. Technically this violates our rules (that a
key/value is allowed to contain quotation marks), but it is necessary
for forbidden characters.

So in summation, a key or value may contain any character except '{',
'}', ','', '=' and spaces. Values (not keys) may be surrounded by
quotes (single or double) to allow all characters. A value is also
allowed to be a nested table.

[1] http://seclists.org/nmap-dev/2007/q3/0146.html
[2] http://seclists.org/nmap-dev/2008/q2/0567.html

Cheers,

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

Attachment: sargs.patch
Description:

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