Nmap Development mailing list archives
Re: Several SNMP script additions
From: Patrik Karlsson <patrik () cqure net>
Date: Sun, 25 Dec 2011 22:15:11 +0100
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/
Current thread:
- Several SNMP script additions Brendan Byrd (Dec 19)
- Several SNMP script additions Brendan Byrd (Dec 18)
- Re: Several SNMP script additions Brendan Byrd (Dec 23)
- Re: Several SNMP script additions Patrik Karlsson (Dec 24)
- Re: Several SNMP script additions Patrik Karlsson (Dec 25)
- Re: Several SNMP script additions Brendan Byrd (Dec 28)