Nmap Development mailing list archives
Re: [NSE] KNX Gateway Discover Script
From: Daniel Miller <bonsaiviking () gmail com>
Date: Tue, 15 Sep 2015 10:12:16 -0500
Niklaus, Thanks for your patience with this submission process. I've just added your scripts in r35243. I made a couple minor changes to error reporting and scope of the output metatable, but functionality should be the same. A great addition, I look forward to using it! Dan On Wed, Aug 12, 2015 at 1:43 PM, Niklaus Schiess <nschiess () adversec com> wrote:
Hi Daniel, thanks again for the comprehensive feedback, I really appreciate it. On 12.08.2015 15:48, Daniel Miller wrote: Niklaus, So glad to see your messages are going through without attachment. I have a few more feedback items now that I've had a chance to look more closely at the scripts. I hope you'll have time to address them, as we are nearly ready to accept this into Nmap: 1. We are preparing to deprecate the "H" pack/unpack format, so it would be best to unpack those items as "A" (arbitrary bytes string) and then convert to an appropriate display format with other functions. Helpful functions for what you are dealing with are: stdnse.tohex, stdnse.format_mac, ipOps.str_to_ip I changed all unpacks to use A instead of H and adjusted proper output via the suggested functions. 2. Are there some checks that could be made to ensure that the script doesn't print junk output? For instance, if the knx_protocol_version is only supposed to be one value (or one of a set of values) we could check there and bail out early. Otherwise, a bad response (truncated packet, for instance) could cause a bin.unpack call to fail, crashing the script. There aren't. All of them have already been in the script (the only one which checks if the response service type is 0x0202). Some fields which have predefined values are also included already. 3. There is a check for knx_supp_svc_families_description == 0x02 that prevents any output if it is false. Is this always guaranteed to be present? Because it seems like we could still print out all the other information that was found even if it is a different value (and the supported families are not populated). I've removed this check because it was indeed unnecessary. The service families are always included in the responses. 4. This is a little obscure, but I think that stdnse.output_table is not a great choice for the service families list. Here's an alternative that just overrides the tostring metamethod for each entry in the list: local fam_meta = { __tostring = function (self) return ("%s version %d"):format( knxServiceFamilies[self.service_id] or self.service_id, self.Version ) end } <snip> for i=0,(knx_total_length-_),2 do local family = {} _, family.service_id, family.Version = bin.unpack('CC', knxMessage, _) setmetatable(family, fam_meta) knx_supp_svc_families[#knx_supp_svc_families+1] = family end <snip> search_response['Supported Services'] = knx_supp_svc_families Also included. 5. It's a good principle to keep the same "schema" of data output at lower verbosity levels, but just "flesh it out" at higher levels. In the case of these scripts, for example, I would prefer to be able to use the same XPATH expression to extract all the device friendly names from a scan with -d as without it. Right now, -d results in that being stored in '//script[@id="knx-gateway-discover"]/table/table[@key="Body"]/table[@key="DIB_DEV_INFO"]/elem[@key="Device friendly name"]' whereas without -d it is stored in '//script[@id="knx-gateway-discover"]/table/elem[@key="Device friendly name"]' I've fixed this by using the same output structure for both modes with and without -d. 6. Along the same lines as 2 above, it would be good if the script were insulated from crashes (lua errors) in the knxParseSearchResponse function. One way to do this would be to launch a worker thread [1] for each received packet. The thread would be responsible for parsing the packet and storing the info in a shared table. If it crashes, the main thread does not crash. At the end of the timeout when knxListen stops receiving packets, it can wait for all the workers to finish (likely they will all finish before the wait happens) and then return. The response parsing has moved into separate threads. I implemented it mostly according to the docs, but I would appreciate it if you would take a look at this at the end of the knxListen function. 7. I think the portrule for knx-gateway-info should be shortport.port_or_service(3671, "efcp", "udp"). The IANA registration for 3671/udp lists "e Field Control (EIBnet)" with a contact at eiba.com, which is registered to the KNX Association, so I think the protocol name is accurate. It only really matters if we ever come up with service probes and matches for knx/efcp. You are right, the service is indeed registered at IANA. I replaced the portrule with your suggestion. Thanks for spending so much time giving such detailed feedback! I'm also looking forward to get this commited :) All changes are included in commit 28e8934 in my GitHub repo [0]. Regards, Niklaus [0] https://github.com/takeshixx/knx-gateway-nse Thanks again, and I'm looking forward to getting this committed! Dan [1] https://nmap.org/book/nse-parallelism.html On Mon, Aug 10, 2015 at 11:24 AM, Niklaus Schiess <nschiess () adversec com> <nschiess () adversec com> wrote: Hi Daniel, thanks again for the feedback. I fixed the issues, tested the script again and pushed it to the GitHub repo [0]. Lets give it a try without an attachment :) Regards, Niklaus [0] https://github.com/takeshixx/knx-gateway-nse On 10.08.2015 15:20, Daniel Miller wrote: Niklaus, Thanks, this is looking better! Since the list keeps dropping your messages, you may want to just link to your Github repository, in case it is the attachments that are looking suspicious. We'll try to get it sorted out soon. Speaking of sorting, the method you've used will not work to sort the output table, unfortunately. Lua's table.sort [1] only handles sorting array-style tables; it won't look at non-integer keys at all. Plus, stdnse.output_table ensures that the keys are always returned in the order in which they were created. Here's a better way to do the same thing: -- whatever process generates your output goes here ip, result = get_some_results() -- store the result by ip in a regular table results = {} results[ip] = result -- store the ip in an array-style (integer key) table ips = {} ips[#ips+1] = ip -- at the end, sort the output table.sort(ips, function_derived_from_ipOps_compare_ip) output = stdnse.output_table() -- technically, ipairs doesn't have to yield results in sorted order, either :( for i in 1, #ips do local ip = ips[i] output[ip] = results[ip] end Dan [1] http://www.lua.org/manual/5.2/manual.html#pdf-table.sort On Sun, Aug 9, 2015 at 7:20 AM, Niklaus Schiess <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> wrote: Hi, I just fixed the output ordering in both scripts and attached them. Thanks for pointing this out, I was struggling with those Lua tables for quite some time. In the discover version I also added sorting by IP addresses. Regards, Niklaus On 09.08.2015 05:44, Daniel Miller wrote: Niklaus, Thanks for taking my suggestions! There's still a bit of a problem regarding the use of output tables: we need the output of subsequent runs of the script to be identical in ordering. That's what stdnse.output_table() provides. A regular Lua table will yield its keys in an unpredictable order, except for integer keys (array-style tables). This means that any place where you store result["label"] = value, the result table should be created as a stdnse.output_table(). Along with this, we should sort the gateways by address. The ipOps library has some useful comparison functions for sorting IP addresses. I'm sorry that your messages to the list keep getting spam-filtered. We will try to sort that out when Fyodor gets back from DEF CON. I'm really looking forward to testing your unicast script soon! Dan On Sat, Aug 8, 2015 at 10:09 AM, Niklaus Schiess <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> <nschiess () adversec com> wrote: Hi, thanks a lot for the feedback! I implemented all the suggestions and attached the new version of the script. Also, I already started to implement a unicast version which uses a different message type to reliably detect gateways based on the suggestion by Michael. Regards, Niklaus _______________________________________________ Sent through the dev mailing listhttps://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/ -- PGP FP: CB84 8C68 ADDB 6C50 7DF1 4227 F2A6 056A A799 76DA _______________________________________________ Sent through the dev mailing listhttps://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/ -- PGP FP: CB84 8C68 ADDB 6C50 7DF1 4227 F2A6 056A A799 76DA _______________________________________________ Sent through the dev mailing listhttps://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/ _______________________________________________ Sent through the dev mailing listhttps://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/ -- PGP FP: CB84 8C68 ADDB 6C50 7DF1 4227 F2A6 056A A799 76DA _______________________________________________ Sent through the dev mailing list https://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/
_______________________________________________ Sent through the dev mailing list https://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/
Current thread:
- [NSE] KNX Gateway Discover Script Niklaus Schiess (Aug 07)
- Re: [NSE] KNX Gateway Discover Script Michael T (Aug 07)
- Re: [NSE] KNX Gateway Discover Script Niklaus Schiess (Aug 07)
- Re: [NSE] KNX Gateway Discover Script Daniel Miller (Aug 08)
- Message not available
- Re: [NSE] KNX Gateway Discover Script Daniel Miller (Aug 08)
- Message not available
- Re: [NSE] KNX Gateway Discover Script Daniel Miller (Aug 10)
- Re: [NSE] KNX Gateway Discover Script Niklaus Schiess (Aug 10)
- Re: [NSE] KNX Gateway Discover Script Daniel Miller (Aug 12)
- Re: [NSE] KNX Gateway Discover Script Niklaus Schiess (Aug 12)
- Re: [NSE] KNX Gateway Discover Script Daniel Miller (Sep 15)
- Message not available
- Re: [NSE] KNX Gateway Discover Script Michael T (Aug 07)