Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 19 commits into from

Conversation

p-l-
Copy link

@p-l- p-l- commented May 1, 2015

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

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-
Copy link
Author

p-l- commented Jun 3, 2015

@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
Copy link

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

@p-l-
Copy link
Author

p-l- commented Jun 28, 2015

@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
Copy link

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

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-
Copy link
Author

p-l- commented Aug 12, 2015

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-
Copy link
Author

p-l- commented Aug 13, 2015

This PR is ready for another review. Thanks!

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

@h4ck3rk3y
Copy link

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-
Copy link
Author

p-l- commented Aug 13, 2015

  • 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
Copy link

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-
Copy link
Author

p-l- commented Aug 14, 2015

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.

The "ls" module is to be used by the scripts rendering file listings,
such as the *-ls scripts and ftp-anon.

The "http-ls" script is a simple script that uses this new module to
output Web servers rendering directories with "Index of" pages.
@nmap-bot nmap-bot closed this in 087fadf Sep 4, 2015
nmap-bot pushed a commit that referenced this pull request Sep 4, 2015
qha pushed a commit to qha/nmap that referenced this pull request Dec 16, 2015
qha pushed a commit to qha/nmap that referenced this pull request Dec 16, 2015
qha pushed a commit to qha/nmap that referenced this pull request Dec 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants