Nmap Development mailing list archives

Previous By Date Next
Previous By Thread Next

Re: [NSE] KNX Gateway Discover Script

From: Niklaus Schiess <nschiess () adversec com>
Date: Wed, 12 Aug 2015 20:43:15 +0200
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>
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>
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>
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 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/


-- 
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/
Previous By Date Next
Previous By Thread Next

Current thread: