Add an ls module to unify arguments and outputs for -ls scripts

[p-l-]

This PR adds a new module called ls and uses it to unify arguments and outputs from *-ls scripts (so far, afp-ls, nfs-ls and smb-ls plus a new script, http-ls).

What remains to do is add ftp-anon (I'm working on that part), but I think it's OK to merge this first so that it can be tested by more people.

I'm running scans to tests these scripts against random hosts and have seen no problem so far.

[dmiller-nmap]

I like this idea, and it looks like you've thought through the script-args part very well. I would like to see a couple things before I do a more thorough review:

1. Update the @output and @xmloutput sections of the *-ls scripts you modified so that we can review the changes.
2. Separate the http-ls script into a different PR so it can be discussed on its own merits. It will also need @output and @xmloutput NSEdoc sections (instead of example output in the description field).

[p-l-]

@dmiller-nmap since you do not actually merge the PR, is it OK for you if I leave the http-ls module in this one? That's easier for me to maintain this code as a single branch. I'll create another PR when (if) this one has been closed.

[dmiller-nmap]

@p-l- Yes, that's fine. Just comment again when you would like a second look at this PR.

[p-l-]

@dmiller-nmap if you have some time I'd like you to have another look and let me know how you feel about these changes. Thanks!

[h4ck3rk3y]

The library looks wonderful and very useful :D.
A couple of changes could be made before committing this to trunk though:

• Different functions, especially those exposed to other scripts in the ls library could use documentation specific to them. http.lua is a fairly well documented library, which you can use an example.
• The end_listing function has too many concatenations which is not a good sign especially when a directory listing is huge. You could use something like stdnse.strjoin() to replace the same.
• I ran the nse check script written on your branch. I found a few badly indexed variables in the smb-ls script, and missing requires in ls.lua and http-ls.lua
• I tried running the http-ls script with the ls.pattern set. I couldn't notice any change in the output, nor could I see any "pattern" specific code in ls.lua.
• http-ls could use an @args for http-ls.url. In general scripts using libraries don't need to to have @args for library arguments. NSEDoc generates documentation for library specific arguments automatically for scripts. For example, any script using the http library would have information about arguments used by the http library automatically in the nsedoc. Further as http.lua uses smb.lua, all http-* scripts have information about arguments used by the smb library,

Gyani

[dmiller-nmap]

Just a comment on one of @h4ck3rk3y's points regarding concatenations: every time you do text = text .. "something", you allocate a new string and copy the contents of text again. The standard ways to handle this are either to store all the substrings in a table and then table.concat (or stdnse.strjoin) them together, or to use a strbuf, which does the same thing behind the scenes, kind of like a StringBuilder does in Java.

[p-l-]

Thanks for this review!

For the point about using an ls.pattern argument, that does not exist. The ls module is meant to be used by scripts that may behave differently. Some accept a pattern argument, while some don't (same thing for checksum for example). That is why http-ls.pattern argument is not documented: it does not exist.

About http-ls.url argument, I simply had forgotten to document it. Nice catch! (PR updated)

For the other remarks, I'll update the PR as soon as I can. Thanks again!

[p-l-]

This PR is ready for another review. Thanks!

By the way I have not updated the changelog section found in some scripts.

[h4ck3rk3y]

I am yet to go through all the changes that you might have made since last time but I have some quick comments.

• The functions that are being exposed to other scripts could use @param and @return.
• Further as we are unifying 'ls', we could expect, future scripts to have pattern matching as well. If we could have the pattern matching feature inside the library this could save a lot of repeated code.
• The above point would apply to all other arguments as well, including checksum, maxdepth, maxpagecount etc.
• Maybe have a global function that takes the checksum type as argument and generates the checksum. This could be useful to other scripts and libraries that aren't using the entire ls functionality as well. I grepped through nselib and couldn't find a library that offers this, I could be wrong, you could check again.
• Having documentation for a feature that isn't library level(like ls.pattern) may confuse users.

The spidering library is a good example on how to handle options. It handles a lot of options like maxdepth, maxpagecount, blacklists, etc. One can override the default values while initializing a crawler.

[p-l-]

• Point 1: you're totally right, done.
• Point 2: you're half right. While it could be good to have a kind of pattern matching at the module level, what is implemented so far is that the SMB server does the pattern matching, so it is really SMB specific. Hence, I have moved the pattern-related code back to the smb-ls script, which also fixes Point 5.
• Points 3 & 4: I agree, but I think this could be achieved latter, since this PR as it is now brings a lot of improvements to existing scripts (particularly the homogenized outputs, and a very good structured output) and those points 3 & 4 would require a lot of extra work. Thoughts?

[h4ck3rk3y]

I agree. The PR brings a lot of good changes by homogenizing the output and I guess I am trying to be too picky. We can always push the present ls library, get feedback and make changes if needed later.

Just one more thing, line 12 talks about ls.checksum, if you could talk about it being a library level argument that works for scripts that implement checksum locally that would be great, else it's a bit confusing.

If the only use of the ls.checksum argument is that while running *-ls scripts we don't have to put the pattern arg individually for all scripts then I would suggest that using --script-args pattern serves the same purpose assuming all the scripts that take args do so using stdnse.get_script_args(SCRIPT_NAME .. ".pattern")

[p-l-]

Don't worry about being "too picky", I've learnt a lot thanks to you! Plus you're totally right to be demanding regarding submitted code.

I have fixed the documentation as you suggested.

Even if, for now, the only real benefits of handling .checksum at module level are to have an homogeneous way to request checksums for scripts that support it and to offer a simple option to request it for all the scripts at once, I do hope that in a near feature we will be able to factorize more code related to that.