Re: Several SNMP script additions

[Patrik Karlsson <patrik () cqure net>]

On Sat, Dec 24, 2011 at 3:19 AM, Brendan Byrd <sineswiper () gmail com> wrote:

> On Sat, Dec 17, 2011 at 8:55 PM, Brendan Byrd <sineswiper () gmail com>
> wrote:
>
> > Got a bunch of library and script changes.  Here's the list of changes:
>
> Anybody willing to put in this into the SVN, or at least discuss the code?
> I know it's X-Mas season, but I figured I'd get a response by now.
>
> --
> Brendan Byrd/SineSwiper <SineSwiper () GMail com>
> _______________________________________________
> Sent through the nmap-dev mailing list
> http://cgi.insecure.org/mailman/listinfo/nmap-dev
> Archived at http://seclists.org/nmap-dev/

Hi Brendan,

First and foremost, thanks for your contribution! I've spent quite some time
going through it, and there's some good things in there. The snmp-system
script
looks really interesting and I agree that it could be a candidate for
replacing
the snmp-sysdescr. I've tested it and added a fingerprint for my Mac Os X
Lion
system. While doing so, I found a bug that would prevent the script from
completing. Also, I think the extrainfo should probably be left out as
there is
a lot of redundant info in there. Also in my case, this section is truncated
and barely readable. I've attached a patch to make these changes.

On that same subject I would personally appreciate if you could send the
next
set of changes you make as patches against subversion. This makes it a lot
easier to review. Also, I noticed that there were quite a few changes in
whitespace, line feeds and minor modifications to things like string matches
and length checks, eg:
* string length checks have been changed from #str to string.len(str)
* string matching has been changed from str:match to string.match (or even
  match in ipOps)

Initially I thought they were all your changes but I'm no longer sure as I
noticed that some of the files may not have been the current versions?
Anyway,
while I guess those changes are probably OK, it would be a lot easier to
handle
them in separate code style patches. Also, if you do changes to code style
make
sure they are consistent across the whole script or library.

Please find comments for each of the files in the zip-archive below:
* asn1.lua
  - This file looks unchanged to me, is that correct?

* ipOps.lua
  - The CIDR additions look very interesting though I haven't been able to
do
    any testing of them yet. In regards to moving them to the C, I'm
probably
    not the right person to comment on that.

* snmp-brute.nse
  - It looks as if most changes make sense, unfortunately Duarte Silva
was/is
    working on some changes to the script at the same time as you. He posted
    an update while is was working on your answer, and I haven't had a
chance
    to look at it yet. One thing, in addition to the changes you've already
    made, there is a problem with the script silently failing when it can't
    load a community list. Even though the debug message is there, it should
    really return an error instead of nothing.

* snmp-routing.nse
  - This script looks impressive, what devices will it work with? I'm not
sure
    I have anything at hand to test it against. If you let me/the list know
    what's needed maybe someone can give it a go?

* snmp-system.nse
  - Like I mentioned, this script looks really nice and I've added a
    fingerprint for Darwin, which in my case is really OS X. I guess this
could
    call for some lookup table for OS:es, so that things get translated
    properly. Also, like I already mentioned, I think the extrainfo should
be
    left out.

* snmp.lua
  - I've merged some of the changes and left the ones that I wasn't sure of.
    There are a few if statements that were changed, that no longer reflect
    what they used to and I'm not sure that all of those are actually
correct.
    Also, I'm not sure whether it's best to return a partial "half-broken"
    result, instead of an error indicating a timeout which would allow a
    script to retry the query. As the function buildGetBulkRequest was not
yet
    working, I left it out. I've included a patch that shows the diff
between
    your version and what's currently in subversion.

* stdnse.lua
  - I don't know if you made any changes to this library, but I'm guessing
that
    if you did, they were made against an old version. There is a function
    missing and also some functions look different now which will break some
    scripts.

* target.lua
  - At first I didn't understand the problem with duplicates but after
reading
    through the code and doing some testing I do. The problem occurs when
    adding ranges and I think your analysis is probably right, this should
    probably be handled elsewhere and not by the library.

In regards to duplicate IP's and ranges and such there's really some
discrepancies as you can add duplicate ip's or overlapping ranges on both
the
command line or in an input list using -iL but the target library attempts
to
prevent it. So maybe this is more of a Nmap question than a library
question?
I mean should you be able to add duplicate ip's and ranges and if so why
should they be treated any different if added through the target library?

So to summarize:
* Great work!
* Could you please submit your changes as diffs and make sure they're
against
  what's in subversion.
* Let's see if we can get some clarity in the duplicates issues and what
other
  people think.

Thanks,
Patrik

-- 
Patrik Karlsson
http://www.cqure.net
http://twitter.com/nevdull77

Attachment: snmp.diff
Description:

Attachment: snmp-system.diff
Description:

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