Re: Several SNMP script additions

[Brendan Byrd <sineswiper () gmail com>]

On Sun, Dec 25, 2011 at 4:15 PM, Patrik Karlsson <patrik () cqure net> wrote:

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

I figured the extrainfo was good for the XML output, since this thing also
doubles as a version script.  Stuff like the full sysDescr might still be
useful.


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

Yeah, I know.  Sorry, I was getting lazy, but I figured I would just put
out a feeler to see if anybody could take a look at the code in general,
since I've been working on a bunch of different files within the library.
I have TortoiseSVN, so I'll see about downloading it directly from SVN and
getting some patches together.


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

I try to, though I generally use 3 spaces / no tabs, KNF, etc.  I also tend
to run through code consistency checks to make sure similar code matches
throughout, even if it's changing original code.  Anyway, I'll patch
through those and ID which ones are mere "cosmetic" changes.

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

Aye, sorry.  Added a debug statement and took it out, so the date changed.


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

Tested a lot of them indirectly from target.lua and using some test scripts
via a Lua interpreter.  Do you guys have any sort of test harness to write
test cases?  I'm a Perl guy, so I'm used to a t/ directory.


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

I've actually made some more changes to this file recently, trying to get
it to work better with creds to get a permanent database (see other
thread).  Still currently having some problems with total lockups, though
I'm not sure if it's a pcap problem or another stuck while loop via cond
signals.


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

Should work with any Cisco router or really anything that implements the
standard SNMP set well enough.  The main problem is the speed and
per-interface repetition, which is why I'd like for target.lua to have some
better accessibility to the initial targets and excludes.


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

I agree with a separate DB NSE file, but I just wanted something started
first.  I was actually kinda surprised that something like that wasn't part
of the version set yet, since SNMP provides a ton of definitive information
about the device in question that makes IDing a box easy.  On that note, do
you know where the uptime variables are stored?  SNMP gives it out as one
of the basic OIDs, but I don't know where to store it in NSE.


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

Okay, I'll take a look at that and the SVN to compare.  Might want to put
the GetBulk stuff in as a commented-out code or something.  It seems like
I'm also there with that function, but I just don't know why I'm not
getting results back.

As far as the partial results, this was because scripts like routing would
grab large tables and timeout because NMap said it was taking too long.
It's not like the server never responded, but the full query itself took
longer than, say, 10 seconds.  NMap doesn't seem to be able to tell the
difference between a timeout because of no packets coming in and a timeout
because the connection has been open for over X seconds.  The partial data
is still useful, and it's better than wasting 10 seconds for nothing.


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

I was actually trying to get some version of a "standalone" environment for
NSE.  For example, while coding the ipOps changes, it seemed silly to keep
running NMap when all I want to test are simple input/output function
cases.  So, I grabbed Lua for Windows and pointed it towards the NSE
libraries.  Doing this required some (small) changes to stdnse and a "stub"
version of nmap.lua.  Kind of a separate deal, but still useful code to try
to build a full-fledged "standalone" environment.  (I know there have been
some conversations about it back in 2007, but I can't seem to find the
thread right now.)


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

Aye, but this might be a good band-aid in the meantime.  The entirety of
ipOps could also be a C library, but that all depends on who would take
that mantle to code it.  I think the biggest issue with target is the lack
of accessibility to initial targets/excludes, and the ability to remove
targets (even if they were added earlier).


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

Well, the biggest concern was having something like snmp-routing finding
the same results over and over again and adding the same targets 100
times.  Frankly, I don't think NMap should really accept duplicate IPs or
ranges either, but checking those blocks as thoroughly as the new
target.lua does it would require translating that Lua code (from both
target and ipOps) to C.  Raw C/C++ code is too low level for me, so that
would be up to somebody else to properly code it, if anybody is motivated
enough to tackle it.

Thanks for checking things out!

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