NSE Libraries and Global Accesses

[Patrick Donnelly <batrick () batbytes com>]

NSE Libraries have had a difficult time lately with inappropriate (and
probably accidental) use of global variables. Besides being much
slower than local access, global variables (within a library) are
shared by all scripts. That is, two or more scripts running some
function in a library may be setting/using globals simultaneously. I
of course use "simultaneously" to mean that while one script is
yielded on some socket operation, another script may be changing the
variables the yielded script intends to use again. To give a concrete
example, we recently had this problem with the nselib/comm.lua
library. The get_banner function would save a socket variable to the
global "socket" variable:

 get_banner = function(host, port, opts)
       opts = initopts(opts)
       opts.recv_before = true
       socket, nothing, correct, banner = tryssl(host, port, "", opts)

When multiple scripts use this get_banner function, each ends up
overwriting the socket being used by the previous script. This is a
serious problem and many other libraries had similar mistakes. The
patch in this case is pretty simple:

-       socket, nothing, correct, banner = tryssl(host, port, "", opts)
+       local socket, nothing, correct, banner = tryssl(host, port, "", opts)

That patch was applied in r13963. Other libraries were corrected in r14080.

These globals variables were found using a fairly simple command:
`luac -l -p library.lua | awk '{ if ($1 == "function") exit; print $0;
}' | grep SETGLOBAL`

That only catches one aspect of the problem, setting globals. That is
to say, whenever a script sets a global outside the file scope (in a
function). Naturally the above example sets the "socket" global. We
also want to catch cases where a script uses a global that does not
exist. In Lua, we say that we "index" a global. Often we hear we index
a hash table; remember that global variables are actually key/value
pairs in a table, the global table.

What makes a global qualify as "not existing"? Well, we consider all
Lua globals (e.g. print), Nmap libraries (e.g., "nmap"), and nselib
libraries (e.g. "http") as legitimate globals (they exist and are
readily available). Next, we also consider the globals set by the file
(e.g., http.request) as valid globals to be indexed within any
function _in that library_. By that I mean, only the http library can
index the "request" global. The http.get function, for example, uses
the request function in this way:

get = function( host, port, path, options )
  options = options or {}
  -- ...
  return request( host, port, data, mod_options )
end

In order to catch cases where a script indexes a global that does not
(or should not) exist, I have created a bash script which will locate
these problems (attached). Here is example output from before patch
r14080.

batrick@batbytes:~/nmap/svn/nmap$ ./check_globals  | less
Checking nselib/datafiles.lua for bad global accesses
        Found set global,'_', at line number 186.
# ...
Checking nselib/imap.lua for bad global accesses
        Found set global,'line', at line number 24.
        Found set global,'status', at line number 24.
        Found indexed global,'line', at line number 25.
        Found set global,'line', at line number 28.
        Found set global,'status', at line number 28.
        Found indexed global,'status', at line number 29.
        Found indexed global,'status', at line number 32.
        Found indexed global,'line', at line number 33.
        Found indexed global,'line', at line number 34.
        Found set global,'line', at line number 34.
        Found indexed global,'line', at line number 35.
        Found set global,'line', at line number 40.
        Found set global,'status', at line number 40.
# ...

That is only a small excerpt of the actual output; as I said earlier,
the problem was widespread.

I hope developers in the future will avail themselves use of this
script so we no longer encounter this problem in the future. (Locals
are your friend!)

Also, before anyone asks, this problem does not concern scripts
outside of libraries because each script instantiation has its own
unique global table; however, I would still encourage using locals
instead due to performance considerations.

-- 
-Patrick Donnelly

"Let all men know thee, but no man know thee thoroughly: Men freely
ford that see the shallows."

- Benjamin Franklin

Attachment: check_globals
Description:

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