NSE for OpenWebNet discovery #915

Closed
wants to merge 46 commits into
from

Conversation

Projects
None yet
2 participants
@rewanth1997
Contributor

rewanth1997 commented Jun 21, 2017

The following script discovers the services running on OpenWebNet protocol.

The script currently fetches the Gateway address, Device type, Number of devices running, Addresses of all services. Handles the errors like Socket connection error, EOF as response and timeout errors.

I have tested it on 4 Indian servers and handled the errors properly.

Do let me know if anything needs to be changed.

@dmiller-nmap

A good start. Here are a few things to consider.

scripts/openwebnet-discovery.nse
+-- | Burglar Alarm: 1
+-- |_ Lighting: 114
+--
+-- Version: 0.1, Updated on 21/06/2017

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Don't do internal versioning. Git/SVN handles this for us, and it just ends up being clutter in the file.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Don't do internal versioning. Git/SVN handles this for us, and it just ends up being clutter in the file.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as 540b75e

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as 540b75e

scripts/openwebnet-discovery.nse
+portrule = shortport.port_or_service(20000, "openwebnet")
+
+local device = {}
+device[2] = "MHServer"

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

The preferred syntax for this is to allocate and create the table all in one statement. This is slightly more efficient since Lua can know the size of the table right away and doesn't have to keep growing and reallocating as we add elements:

local device = {
  [2] = "MHServer",
  [4] = "MH200",
  -- etc.
}
@dmiller-nmap

dmiller-nmap Jun 21, 2017

The preferred syntax for this is to allocate and create the table all in one statement. This is slightly more efficient since Lua can know the size of the table right away and doesn't have to keep growing and reallocating as we add elements:

local device = {
  [2] = "MHServer",
  [4] = "MH200",
  -- etc.
}

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as 540b75e

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as 540b75e

scripts/openwebnet-discovery.nse
+ return nil, nil, "Received a negative ACK as response."
+ end
+
+ stdnse.sleep(2)

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

What is the purpose of this sleep? Is it really needed, or are we just not synchronizing TCP streams very well?

@dmiller-nmap

dmiller-nmap Jun 21, 2017

What is the purpose of this sleep? Is it really needed, or are we just not synchronizing TCP streams very well?

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

I didn't get the responses properly when I made large number of requests. That's due to my poor internet connection which I realized now. So, I removed stdnse.sleep(2) from the code and its committed as 5fb09bb.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

I didn't get the responses properly when I made large number of requests. That's due to my poor internet connection which I realized now. So, I removed stdnse.sleep(2) from the code and its committed as 5fb09bb.

scripts/openwebnet-discovery.nse
+ sd:send(request)
+
+ local status, data = sd:receive_buf("*#*1##", false)
+ if data == "EOF" then

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Should be testing status here, not data. It doesn't matter what the error is, really, only whether we received a response or not. Pass the error string directly up the stack if you wish. Same below with "TIMEOUT"

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Should be testing status here, not data. It doesn't matter what the error is, really, only whether we received a response or not. Pass the error string directly up the stack if you wish. Same below with "TIMEOUT"

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as c7e24f4.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as c7e24f4.

scripts/openwebnet-discovery.nse
+ end
+end
+
+-- Removes *#*1## from the beginning and ending

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Rather than just trimming off the ACK responses, can we convert this to a readable string? If it's an IP address, extracting the data (after the WHO number) and replacing "#" with "." seems to be the way to do it. Other data types will require other parsing.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Rather than just trimming off the ACK responses, can we convert this to a readable string? If it's an IP address, extracting the data (after the WHO number) and replacing "#" with "." seems to be the way to do it. Other data types will require other parsing.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

It confirms that the data is received completely for a particular request.

What do you mean by if it's an IP address? , the response pattern will be same in all the cases, isn't it?

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

It confirms that the data is received completely for a particular request.

What do you mean by if it's an IP address? , the response pattern will be same in all the cases, isn't it?

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

Let's take the request for the device's own IP address as an example, since the reference page says that Gateway IP is only sometimes supported. The request is *#13**10## and the response is:

*#13**10*192*168*1*40##*#*1##
|||  ||  |           | |_ ACK
|||  ||  |           |_ End of message
|||  ||  |_ VALUE: 192.168.1.40
|||  ||_ DIMENSION: IP Address
|||  |_ WHERE: empty 
|||_ WHO: Device Communication
||_ this is a Dimension message
|_ Start of message

So for any request, we should parse the response into a series of messages (usually there will be only 1 plus an ACK), validate that it corresponds to the request we sent (WHO, WHERE, and DIMENSION match), and then convert the VALUE as appropriate. In this case, it's clear that the value is a list of the octets of an IP address. We could probably just replace the * with . in that case. Time and Date are also interesting, so we'll have to extract each value and convert it into a readable string. When we find an ACK or NACK message, that means we're done parsing; NACK means the command was bad or not supported.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

Let's take the request for the device's own IP address as an example, since the reference page says that Gateway IP is only sometimes supported. The request is *#13**10## and the response is:

*#13**10*192*168*1*40##*#*1##
|||  ||  |           | |_ ACK
|||  ||  |           |_ End of message
|||  ||  |_ VALUE: 192.168.1.40
|||  ||_ DIMENSION: IP Address
|||  |_ WHERE: empty 
|||_ WHO: Device Communication
||_ this is a Dimension message
|_ Start of message

So for any request, we should parse the response into a series of messages (usually there will be only 1 plus an ACK), validate that it corresponds to the request we sent (WHO, WHERE, and DIMENSION match), and then convert the VALUE as appropriate. In this case, it's clear that the value is a list of the octets of an IP address. We could probably just replace the * with . in that case. Time and Date are also interesting, so we'll have to extract each value and convert it into a readable string. When we find an ACK or NACK message, that means we're done parsing; NACK means the command was bad or not supported.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

Committed as ff5d5ec.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

Committed as ff5d5ec.

scripts/openwebnet-discovery.nse
+
+ local output = {}
+
+ local sd, gateway, err = get_socket(host, port, "*#13**15##")

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Instead of directly passing OpenWebNet command strings (with "*#" and all that), write a function that returns the appropriate command like so:

local command = get_command("Device Communication", "Gateway IP Address")

This would do a lookup in a table to convert the first parameter to "13" and the second to "50". Note that what you are requesting here is actually the device type (15), not the gateway (50) as the script indicates.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Instead of directly passing OpenWebNet command strings (with "*#" and all that), write a function that returns the appropriate command like so:

local command = get_command("Device Communication", "Gateway IP Address")

This would do a lookup in a table to convert the first parameter to "13" and the second to "50". Note that what you are requesting here is actually the device type (15), not the gateway (50) as the script indicates.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

But how can i know whether the parameters in the command are referring to WHO or WHAT or WHERE? How should I differentiate that?

In this case, I think its better to use something like this,
local command = get_command(WHO, WHERE, WHAT)

Your opinion please.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

But how can i know whether the parameters in the command are referring to WHO or WHAT or WHERE? How should I differentiate that?

In this case, I think its better to use something like this,
local command = get_command(WHO, WHERE, WHAT)

Your opinion please.

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

I see that could be difficult. Then I would have separate functions for each of the message types we want to support. I think this will only be Status Request (WHO, WHERE) and Dimension Request (WHO, WHERE, DIMENSION). In fact, we can support both with a single function which simply joins all arguments (...) with * and adds the message start (*# for requests) and end (##).

@dmiller-nmap

dmiller-nmap Jun 22, 2017

I see that could be difficult. Then I would have separate functions for each of the message types we want to support. I think this will only be Status Request (WHO, WHERE) and Dimension Request (WHO, WHERE, DIMENSION). In fact, we can support both with a single function which simply joins all arguments (...) with * and adds the message start (*# for requests) and end (##).

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

I managed to code it this way, 2ed6404. Your thoughts on this implementation please?

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

I managed to code it this way, 2ed6404. Your thoughts on this implementation please?

scripts/openwebnet-discovery.nse
+
+-- Returns table after appending the delimiter
+-- The return table contains the list of devices
+local function custom_split(delimiter, resultant)

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Instead of this function, you can call socket:receive_buf(delimiter, false) and it will return the next chunk of data up to the delimiter from the socket's receive buffer. In a loop, you can watch for an ACK message to indicate the end of the list, so that you stop trying to receive.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Instead of this function, you can call socket:receive_buf(delimiter, false) and it will return the next chunk of data up to the delimiter from the socket's receive buffer. In a loop, you can watch for an ACK message to indicate the end of the list, so that you stop trying to receive.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

I tried doing this, but there are many duplicate results if I used ## as a delimiter. I think keeping this separate function will do a better job as we can get the data from the server in one shot and we can perform small operations on it to extract the results.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

I tried doing this, but there are many duplicate results if I used ## as a delimiter. I think keeping this separate function will do a better job as we can get the data from the server in one shot and we can perform small operations on it to extract the results.

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

I think that the duplicate results are caused by not taking into account the ACK messages which frame the response: there's one at the beginning as a banner, then one at the end of each response (or a NACK if the request couldn't be answered). The syntax does not otherwise allow ## anywhere else in a message.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

I think that the duplicate results are caused by not taking into account the ACK messages which frame the response: there's one at the beginning as a banner, then one at the end of each response (or a NACK if the request couldn't be answered). The syntax does not otherwise allow ## anywhere else in a message.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 23, 2017

Contributor

Its working now. Committed as 6e7276c.

@rewanth1997

rewanth1997 Jun 23, 2017

Contributor

Its working now. Committed as 6e7276c.

scripts/openwebnet-discovery.nse
+ end
+
+ -- Fetching list of each device
+ for _, v in pairs(who) do

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

I don't think we should do this for every possible WHO value. Some of them may have other functions than "get the status of all X" if you call them with a WHAT of 0. For example, Device Communication (13) responds with a NACK. Instead, keep a list of ones that do respond properly like this (1 through 25, maybe, although I don't know what CEN means) and check those this way.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

I don't think we should do this for every possible WHO value. Some of them may have other functions than "get the status of all X" if you call them with a WHAT of 0. For example, Device Communication (13) responds with a NACK. Instead, keep a list of ones that do respond properly like this (1 through 25, maybe, although I don't know what CEN means) and check those this way.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

Committed as ceb0968.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

Committed as ceb0968.

scripts/openwebnet-discovery.nse
+
+ stdnse.debug("Fetching the list of " .. v .. " devices.")
+
+ local sd, data, err = get_socket(host, port, "*##*#" .. _ .. "*0##")

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

What happened to the last socket we opened? If we keep the TCP stream synchronized (meaning we know which responses belong to which requests), we should be able to keep sending requests/commands on the same socket.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

What happened to the last socket we opened? If we keep the TCP stream synchronized (meaning we know which responses belong to which requests), we should be able to keep sending requests/commands on the same socket.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

If I'm using the last socket we opened, I'm getting the previous response only even after sending the new data. That's the reason I'm creating new socket every time. I'm not sure of the reason behind this kind of response.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

If I'm using the last socket we opened, I'm getting the previous response only even after sending the new data. That's the reason I'm creating new socket every time. I'm not sure of the reason behind this kind of response.

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

Try it again after implementing the parsing I suggested. socket:receive_buf ought to make it simple to only consume the buffer once; as long as you only send one request and then read up to the first ACK or NACK, the socket should be ready for a second request and response.

@dmiller-nmap

dmiller-nmap Jun 22, 2017

Try it again after implementing the parsing I suggested. socket:receive_buf ought to make it simple to only consume the buffer once; as long as you only send one request and then read up to the first ACK or NACK, the socket should be ready for a second request and response.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

Committed as 6e7276c.

@rewanth1997

rewanth1997 Jun 24, 2017

Contributor

Committed as 6e7276c.

scripts/openwebnet-discovery.nse
+
+action = function(host, port)
+
+ local output = {}

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Make output a stdnse.output_table(), so that the keys are always in the same order. Some software like Ndiff will treat different orderings as different results.

@dmiller-nmap

dmiller-nmap Jun 21, 2017

Make output a stdnse.output_table(), so that the keys are always in the same order. Some software like Ndiff will treat different orderings as different results.

This comment has been minimized.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as 4d0d4a9.

@rewanth1997

rewanth1997 Jun 22, 2017

Contributor

Patched as 4d0d4a9.

@dmiller-nmap

Great! These are mostly formatting changes; the protocol implementation looks fine.

scripts/openwebnet-discovery.nse
+ [4] = "Heating",
+ [5] = "Burglar Alarm",
+ [6] = "Door Entry System"
+}

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

It'd be nice to still have the full list of WHO values, even if we only query 0 through 6.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

It'd be nice to still have the full list of WHO values, even if we only query 0 through 6.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as aedb765.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as aedb765.

scripts/openwebnet-discovery.nse
+}
+
+local device_dimensions = {
+ ["Time"] = "*#13**0##",

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

It would be cleaner to have this table simply be the dimension numbers, and use a function to add the "*#13**" and "##"

@dmiller-nmap

dmiller-nmap Jun 27, 2017

It would be cleaner to have this table simply be the dimension numbers, and use a function to add the "*#13**" and "##"

scripts/openwebnet-discovery.nse
+OpenWebNet is a communications protocol developed by Bticino since 2000.
+Retrieves the Gateway and device type. Retrieves the count and addresses
+of lights, multimedia and many other services running on server/servers.
+]]

This comment has been minimized.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 337eda5.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 337eda5.

scripts/openwebnet-discovery.nse
+
+ -- If response is NACK, it means the request method is not supported
+ if data == NACK then
+ res = {}

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

Returning nil is easier to check for than empty table. It's OK if WHO categories that have 0 devices are not shown in the script output.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

Returning nil is easier to check for than empty table. It's OK if WHO categories that have 0 devices are not shown in the script output.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

I think showing 0 devices will be more helpful to the user during reconnaissance.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

I think showing 0 devices will be more helpful to the user during reconnaissance.

scripts/openwebnet-discovery.nse
+
+local function format_dimensions(res)
+
+ if res["Time"] then

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

For "Time" and "Date", it would be preferable to send the "Date and Time" (22) query, since it saves a query. Then extract the year, month, day, hour, minute, second and pass them in a table to stdnse.format_timestamp. The table format is described in the Lua manual for os.time.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

For "Time" and "Date", it would be preferable to send the "Date and Time" (22) query, since it saves a query. Then extract the year, month, day, hour, minute, second and pass them in a table to stdnse.format_timestamp. The table format is described in the Lua manual for os.time.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 796e6dc.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 796e6dc.

scripts/openwebnet-discovery.nse
+
+ stdnse.debug("Fetching the list of " .. v .. " devices.")
+
+ local res = get_response(sd, "*##*#" .. _ .. "*0##")

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

Don't send the "*##" empty/invalid command first.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

Don't send the "*##" empty/invalid command first.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 39e9980.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 39e9980.

scripts/openwebnet-discovery.nse
+ end
+
+ -- Fetching list of dimensions of a device
+ for _, v in pairs(device_dimensions) do

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

pairs does not produce a predictable ordering, but we need output to be predictable. I suggest completing the device_dimensions table to include all values, even ones we do not query. Then iterate over a separate table of dimension names that we do want.

Only use _ as a loop variable when you intend to not use it elsewhere. It's the "throwaway" variable. If you intend on using it, use a descriptive name. Here's an example of what I am talking about. _ is used for the index, which we don't care about; we only want the names in order.

for _, label in ipairs({"Device Type", "Date and Time", "Uptime", "Firmware version", ...}) do
@dmiller-nmap

dmiller-nmap Jun 27, 2017

pairs does not produce a predictable ordering, but we need output to be predictable. I suggest completing the device_dimensions table to include all values, even ones we do not query. Then iterate over a separate table of dimension names that we do want.

Only use _ as a loop variable when you intend to not use it elsewhere. It's the "throwaway" variable. If you intend on using it, use a descriptive name. Here's an example of what I am talking about. _ is used for the index, which we don't care about; we only want the names in order.

for _, label in ipairs({"Device Type", "Date and Time", "Uptime", "Firmware version", ...}) do

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 5de72e6.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 5de72e6.

scripts/openwebnet-discovery.nse
+
+description = [[
+OpenWebNet is a communications protocol developed by Bticino since 2000.
+Retrieves the Gateway and device type. Retrieves the count and addresses

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

We don't retrieve the Gateway or "addresses of lights". Generalize this description to only say we retrieve device identifying information and number of connected devices.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

We don't retrieve the Gateway or "addresses of lights". Generalize this description to only say we retrieve device identifying information and number of connected devices.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 337eda5.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as 337eda5.

scripts/openwebnet-discovery.nse
+ end
+
+ if res["MAC address"] then
+ res["MAC address"] = string.gsub(res["MAC address"], "%.", "-")

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

We want the MAC address as colon-separated hex. Here's a list of functions that may help:

  • stdnse.tohex
  • stdnse.format_mac
  • string.format("%x")
  • table.concat
  • string.gsub (with function instead of replacement string)
@dmiller-nmap

dmiller-nmap Jun 27, 2017

We want the MAC address as colon-separated hex. Here's a list of functions that may help:

  • stdnse.tohex
  • stdnse.format_mac
  • string.format("%x")
  • table.concat
  • string.gsub (with function instead of replacement string)

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as f563a23.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as f563a23.

scripts/openwebnet-discovery.nse
+-- | Device Type: F453AV
+-- | Distribution Version: 3.0.1
+-- | Firmware version: 3.0.14
+-- | Uptime: 5.3.28.38

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

Uptime should be reported in the same format that stdnse.format_difftime uses. You don't have to use that function, though, since it's already reported broken into segments like this. Your example output should look like "5d3h28m38s". Note that the piMyHome page does not show seconds, so some devices may not report that.

@dmiller-nmap

dmiller-nmap Jun 27, 2017

Uptime should be reported in the same format that stdnse.format_difftime uses. You don't have to use that function, though, since it's already reported broken into segments like this. Your example output should look like "5d3h28m38s". Note that the piMyHome page does not show seconds, so some devices may not report that.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as be53037.

@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

Committed as be53037.

@rewanth1997

This comment has been minimized.

Show comment
Hide comment
@rewanth1997

rewanth1997 Jul 1, 2017

Contributor

@dmiller-nmap, All the requested changes are made, your final views on the modified code ?

Contributor

rewanth1997 commented Jul 1, 2017

@dmiller-nmap, All the requested changes are made, your final views on the modified code ?

@dmiller-nmap

This feedback is based on running the script; it still takes almost 20 seconds to run after I made some hasty fixes regarding NACK handling, but it's a big improvement over the current version.

scripts/openwebnet-discovery.nse
+Retrieves device identifying information and number of connected devices.
+
+References:
+ https://www.myopen-legrandgroup.com/solution-gallery/openwebnet/

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Use * at the beginning of NSEdoc lines to indicate an unordered list.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Use * at the beginning of NSEdoc lines to indicate an unordered list.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 25456ad

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 25456ad

scripts/openwebnet-discovery.nse
+-- | openwebnet-discover:
+-- | IP Address: 192.168.200.35
+-- | Net Mask: 255.255.255.0
+-- | MAC Address: 0:3:50:1:d3:11

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

This should be formatted as 00:03:50:01:d3:11. Hints on how to do this below.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

This should be formatted as 00:03:50:01:d3:11. Hints on how to do this below.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 348e234

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 348e234

scripts/openwebnet-discovery.nse
+-- | Distribution Version: 3.0.1
+-- | Date: 02.07.2017
+-- | Time: 02:11:58
+-- | Scenarios: 0

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

I still prefer to not see lines with 0 count.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

I still prefer to not see lines with 0 count.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 0b5cde6

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 0b5cde6

scripts/openwebnet-discovery.nse
+-- Returns the socket and error message
+local function get_socket(host, port, request)
+
+ local sd, response, early_resp = comm.opencon(host, port, request)

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Since we expect to get a banner in response to connection, we need to set the recv_before option to opencon. This will connect, wait for a banner, and return that as early_resp. We should NOT be sending an ACK here, since it only responds with a NACK because ACK is not a client message. So the intended flow is:

  1. C > S - connect
  2. C < S - ACK
  3. C > S - request
  4. C < S - response
@dmiller-nmap

dmiller-nmap Jul 3, 2017

Since we expect to get a banner in response to connection, we need to set the recv_before option to opencon. This will connect, wait for a banner, and return that as early_resp. We should NOT be sending an ACK here, since it only responds with a NACK because ACK is not a client message. So the intended flow is:

  1. C > S - connect
  2. C < S - ACK
  3. C > S - request
  4. C < S - response

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as c573a9f

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as c573a9f

scripts/openwebnet-discovery.nse
+ local sd, response, early_resp = comm.opencon(host, port, request)
+
+ if sd == nil then
+ return nil, "Socket connection error."

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Return or print (debug) the actual error, which is in the response variable now.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Return or print (debug) the actual error, which is in the response variable now.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as b1a2a9a

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as b1a2a9a

scripts/openwebnet-discovery.nse
+ table.insert(t, stdnse.tohex(tonumber(v)))
+ end
+
+ res["MAC Address"] = table.concat(t, ":")

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Instead of splitting, converting, and joining, try using string.gsub with a function like so:

formatted = string.gsub(raw, "(%d+)%.?",
    function (number)
      return do_something(number)
    end
)

You have options for do_something: you could simply return the byte with the value of number, which would convert the whole string to a 6-byte blob suitable for passing to stdnse.format_mac. You could convert it directly to hex with string.format, and change the separator to ":" either there or in a separate pass (not consuming it in the pattern). Etc.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Instead of splitting, converting, and joining, try using string.gsub with a function like so:

formatted = string.gsub(raw, "(%d+)%.?",
    function (number)
      return do_something(number)
    end
)

You have options for do_something: you could simply return the byte with the value of number, which would convert the whole string to a 6-byte blob suitable for passing to stdnse.format_mac. You could convert it directly to hex with string.format, and change the separator to ":" either there or in a separate pass (not consuming it in the pattern). Etc.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 348e234

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 348e234

scripts/openwebnet-discovery.nse
+
+ for _, v in ipairs(stdnse.strsplit("%.%s*", res["Uptime"])) do
+ table.insert(t, v .. units[counter])
+ counter = counter + 1

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Here, the _ variable is an index you could use instead of keeping a separate counter. Remember that Lua tables are 1-indexed, so remove the [0] = from the declaration of units at the top. Otherwise, this is a smart solution.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Here, the _ variable is an index you could use instead of keeping a separate counter. Remember that Lua tables are 1-indexed, so remove the [0] = from the declaration of units at the top. Otherwise, this is a smart solution.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as fa69e5a

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as fa69e5a

+
+ stdnse.debug("Fetching " .. device)
+
+ local res = get_response(sd, head .. device_dimension[device] .. tail)

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Need to check that res is valid here. A good test for a table with values in it is: if res and next(res) then

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Need to check that res is valid here. A good test for a table with values in it is: if res and next(res) then

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 377da5d

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 377da5d

scripts/openwebnet-discovery.nse
+ local regex = string.gsub(head, "*", "%%*") .. device_dimension[device] .. "%*" .."(.+)" .. tail
+ local tempRes = string.match(res[1], regex)
+
+ output[device] = string.gsub(tempRes, "*", ".")

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Did we check whether tempRes is not nil? What if the server sends a response that doesn't match?

@dmiller-nmap

dmiller-nmap Jul 3, 2017

Did we check whether tempRes is not nil? What if the server sends a response that doesn't match?

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 377da5d

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 377da5d

scripts/openwebnet-discovery.nse
+ output = format_dimensions(output)
+
+ -- Fetching list of each device
+ for i = 0, 6 do

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

I don't think Scenarios (WHO=0) supports retrieving a status list like this. Just run from 1 to 6.

@dmiller-nmap

dmiller-nmap Jul 3, 2017

I don't think Scenarios (WHO=0) supports retrieving a status list like this. Just run from 1 to 6.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 50af9ca

@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

Committed as 50af9ca

@rewanth1997

This comment has been minimized.

Show comment
Hide comment
@rewanth1997

rewanth1997 Jul 4, 2017

Contributor

The previous script took 55-60 sec to run. I guess I made all the changes as requested by you. Now the new script takes around 30 sec to run. I will try to optimize it more, meanwhile please have a look at the new code.

Contributor

rewanth1997 commented Jul 4, 2017

The previous script took 55-60 sec to run. I guess I made all the changes as requested by you. Now the new script takes around 30 sec to run. I will try to optimize it more, meanwhile please have a look at the new code.

@dmiller-nmap

A couple minor things and some bigger things as well. Nearly done!

scripts/openwebnet-discovery.nse
@@ -155,37 +154,37 @@ end
local function format_dimensions(res)
if res["Date and Time"] then
- res["Date"] = string.match(res["Date and Time"], "((%d+)%.(%d+)%.(%d+))$")
+ local params = {
+ [0] = "hour", "min", "sec", "msec", "dayOfWeek", "year", "month", "day"

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

If you remove the [0] = part, these will be indexed starting at 1, so you don't have to do counter - 1 below. Cleaner code by using the same index for both.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

If you remove the [0] = part, these will be indexed starting at 1, so you don't have to do counter - 1 below. Cleaner code by using the same index for both.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

Committed as 75122fe.

@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

Committed as 75122fe.

scripts/openwebnet-discovery.nse
- local t = {}
- for _, v in ipairs(stdnse.strsplit("%.%s*", res["MAC Address"])) do
- table.insert(t, stdnse.tohex(tonumber(v)))
+ res["MAC Address"] = string.gsub(res["MAC Address"], "(%d+)%.", function(num)

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

As I mentioned in our chat, the last octet does not have a "." at the end, so this will leave it as decimal. You could:

  • match "(%d+)%.?" so that it matches the last octet. Then strip off the extra ":" from the formatted string.
  • match "(%d+)(%.?)", passing both captures to your replacement function. Only insert a ":" if the second is not empty.
  • Split the string on ".", convert each item, and re-join on ":". Look at the listop library for possible ways to do this.
  • Capture all the 6 octets in a single pattern like string.rep("(%d+)%.?", 6) and format them directly with "%02x:%02x:%02x:%02x:%02x:%02x".
@dmiller-nmap

dmiller-nmap Jul 7, 2017

As I mentioned in our chat, the last octet does not have a "." at the end, so this will leave it as decimal. You could:

  • match "(%d+)%.?" so that it matches the last octet. Then strip off the extra ":" from the formatted string.
  • match "(%d+)(%.?)", passing both captures to your replacement function. Only insert a ":" if the second is not empty.
  • Split the string on ".", convert each item, and re-join on ":". Look at the listop library for possible ways to do this.
  • Capture all the 6 octets in a single pattern like string.rep("(%d+)%.?", 6) and format them directly with "%02x:%02x:%02x:%02x:%02x:%02x".

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

Committed as c4fa595.

@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

Committed as c4fa595.

scripts/openwebnet-discovery.nse
- table.insert(t, v .. units[counter])
- counter = counter + 1
+ for counter, v in ipairs(stdnse.strsplit("%.%s*", res["Uptime"])) do
+ table.insert(t, v .. units[counter - 1])

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

Same argument here for leaving units with the first element at index 1 so you don't have to subtract 1 each time.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

Same argument here for leaving units with the first element at index 1 so you don't have to subtract 1 each time.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

Committed as 75122fe.

@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

Committed as 75122fe.

+ end
+
+ -- If response is NACK, it means the request method is not supported
+ if data == NACK then

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

Ran into another problem. A host closes the connection after sending the banner, but we keep trying to send on the closed socket. The symptom is status is nil and data is "EOF". The trouble for the caller is that nil can mean "not supported" or "socket was closed." An easy fix would be:

  • nil means socket problems. Return the error as a second value
  • empty table means either no results or NACK
  • populated table means we got results.

In my test case, the server did not respond to any of our queries, but always closed the socket. This means we don't have to try to reopen and reconnect.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

Ran into another problem. A host closes the connection after sending the banner, but we keep trying to send on the closed socket. The symptom is status is nil and data is "EOF". The trouble for the caller is that nil can mean "not supported" or "socket was closed." An easy fix would be:

  • nil means socket problems. Return the error as a second value
  • empty table means either no results or NACK
  • populated table means we got results.

In my test case, the server did not respond to any of our queries, but always closed the socket. This means we don't have to try to reopen and reconnect.

+ end
+
+ -- If response is NACK, it means the request method is not supported
+ if data == NACK then

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 7, 2017

And yet another tricky one: timeouts. If the timeout expires, we may end up reading a response to one probe as the answer to the next probe. There are 2 approaches we must take to avoid this:

  1. Increase the request_timeout when connecting with comm.opencon, but do not increase the timeout or connect_timeout. Requests may take longer because the gateway has to contact the slave devices. Probably use 10 seconds (my test system took 8.2 seconds, greater than the connect_timeout (1.4s) plus the request_timeout (6s) by default).
  2. To the extent possible, we should try to check the response and synchronize. Here was the output of my mixup:
    NSE: [openwebnet-discovery 78.216.11.170:20000] Fetching the list of Heating devices.
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | *#4*0##
    NSOCK INFO [11.4180s] nsock_write(): Write request for 7 bytes to IOD #1 EID 451 [78.216.11.170:20000]
    NSOCK INFO [11.4180s] nsock_trace_handler_callback(): Callback: WRITE SUCCESS for EID 451 [78.216.11.170:20000]
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | SEND
    NSOCK INFO [11.4180s] nsock_read(): Read request from IOD #1 [78.216.11.170:20000] (timeout: 7434ms) EID 458
    NSOCK INFO [18.8520s] nsock_trace_handler_callback(): Callback: READ TIMEOUT for EID 458 [78.216.11.170:20000]
    NSE: [openwebnet-discovery 78.216.11.170:20000] Fetching the list of Burglar Alarm devices.
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | *#5*0##
    NSOCK INFO [18.8530s] nsock_write(): Write request for 7 bytes to IOD #1 EID 467 [78.216.11.170:20000]
    NSOCK INFO [18.8530s] nsock_trace_handler_callback(): Callback: WRITE SUCCESS for EID 467 [78.216.11.170:20000]
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | SEND
    NSOCK INFO [18.8530s] nsock_read(): Read request from IOD #1 [78.216.11.170:20000] (timeout: 7434ms) EID 474
    NSOCK INFO [19.6130s] nsock_trace_handler_callback(): Callback: READ SUCCESS for EID 474 [78.216.11.170:20000] (6 bytes): *#*0##
    NSE: TCP 192.168.1.23:46051 < 78.216.11.170:20000 | *#*0##
    NSE: [openwebnet-discovery 78.216.11.170:20000] Fetching the list of Door Entry System devices.
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | *#6*0##
    NSOCK INFO [19.6130s] nsock_write(): Write request for 7 bytes to IOD #1 EID 483 [78.216.11.170:20000]
    NSOCK INFO [19.6130s] nsock_trace_handler_callback(): Callback: WRITE SUCCESS for EID 483 [78.216.11.170:20000]
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | SEND
    NSOCK INFO [19.6140s] nsock_read(): Read request from IOD #1 [78.216.11.170:20000] (timeout: 7434ms) EID 490
    NSOCK INFO [23.1590s] nsock_trace_handler_callback(): Callback: READ SUCCESS for EID 490 [78.216.11.170:20000] (7 bytes): *5*0*##
    NSE: TCP 192.168.1.23:46051 < 78.216.11.170:20000 | *5*0*##
    
    So Heating (4) timed out, but the server sent a NACK which we interpreted as the answer for Burglar Alarm (5). Then we sent Door Entry (6) and got a response for 5. We should detect that the response doesn't match the request. Maybe extract the WHO value from the response and use that instead of just assuming it matches (only works for non-NACK responses). That would at least avoid mixing up answers, even if it means we accidentally drop some. Better to not report something than to report it incorrectly.
@dmiller-nmap

dmiller-nmap Jul 7, 2017

And yet another tricky one: timeouts. If the timeout expires, we may end up reading a response to one probe as the answer to the next probe. There are 2 approaches we must take to avoid this:

  1. Increase the request_timeout when connecting with comm.opencon, but do not increase the timeout or connect_timeout. Requests may take longer because the gateway has to contact the slave devices. Probably use 10 seconds (my test system took 8.2 seconds, greater than the connect_timeout (1.4s) plus the request_timeout (6s) by default).
  2. To the extent possible, we should try to check the response and synchronize. Here was the output of my mixup:
    NSE: [openwebnet-discovery 78.216.11.170:20000] Fetching the list of Heating devices.
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | *#4*0##
    NSOCK INFO [11.4180s] nsock_write(): Write request for 7 bytes to IOD #1 EID 451 [78.216.11.170:20000]
    NSOCK INFO [11.4180s] nsock_trace_handler_callback(): Callback: WRITE SUCCESS for EID 451 [78.216.11.170:20000]
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | SEND
    NSOCK INFO [11.4180s] nsock_read(): Read request from IOD #1 [78.216.11.170:20000] (timeout: 7434ms) EID 458
    NSOCK INFO [18.8520s] nsock_trace_handler_callback(): Callback: READ TIMEOUT for EID 458 [78.216.11.170:20000]
    NSE: [openwebnet-discovery 78.216.11.170:20000] Fetching the list of Burglar Alarm devices.
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | *#5*0##
    NSOCK INFO [18.8530s] nsock_write(): Write request for 7 bytes to IOD #1 EID 467 [78.216.11.170:20000]
    NSOCK INFO [18.8530s] nsock_trace_handler_callback(): Callback: WRITE SUCCESS for EID 467 [78.216.11.170:20000]
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | SEND
    NSOCK INFO [18.8530s] nsock_read(): Read request from IOD #1 [78.216.11.170:20000] (timeout: 7434ms) EID 474
    NSOCK INFO [19.6130s] nsock_trace_handler_callback(): Callback: READ SUCCESS for EID 474 [78.216.11.170:20000] (6 bytes): *#*0##
    NSE: TCP 192.168.1.23:46051 < 78.216.11.170:20000 | *#*0##
    NSE: [openwebnet-discovery 78.216.11.170:20000] Fetching the list of Door Entry System devices.
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | *#6*0##
    NSOCK INFO [19.6130s] nsock_write(): Write request for 7 bytes to IOD #1 EID 483 [78.216.11.170:20000]
    NSOCK INFO [19.6130s] nsock_trace_handler_callback(): Callback: WRITE SUCCESS for EID 483 [78.216.11.170:20000]
    NSE: TCP 192.168.1.23:46051 > 78.216.11.170:20000 | SEND
    NSOCK INFO [19.6140s] nsock_read(): Read request from IOD #1 [78.216.11.170:20000] (timeout: 7434ms) EID 490
    NSOCK INFO [23.1590s] nsock_trace_handler_callback(): Callback: READ SUCCESS for EID 490 [78.216.11.170:20000] (7 bytes): *5*0*##
    NSE: TCP 192.168.1.23:46051 < 78.216.11.170:20000 | *5*0*##
    
    So Heating (4) timed out, but the server sent a NACK which we interpreted as the answer for Burglar Alarm (5). Then we sent Door Entry (6) and got a response for 5. We should detect that the response doesn't match the request. Maybe extract the WHO value from the response and use that instead of just assuming it matches (only works for non-NACK responses). That would at least avoid mixing up answers, even if it means we accidentally drop some. Better to not report something than to report it incorrectly.
@rewanth1997

This comment has been minimized.

Show comment
Hide comment
@rewanth1997

rewanth1997 Jul 8, 2017

Contributor

@dmiller-nmap Requested changes are done. Please review them.

Contributor

rewanth1997 commented Jul 8, 2017

@dmiller-nmap Requested changes are done. Please review them.

@dmiller-nmap

Also need to add the 2 changes listed in this comment: #915 (comment)

scripts/openwebnet-discovery.nse
+ stdnse.debug("Error: " .. tostring(data))
+ -- Captures the NACK after TIMEOUT occured.
+ -- Avoids false results.
+ status, data = sd:receive_buf("##", true)

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 10, 2017

This should only be attempted if the error was "TIMEOUT". The error could be "EOF" or something else, which means we can't read any further.

@dmiller-nmap

dmiller-nmap Jul 10, 2017

This should only be attempted if the error was "TIMEOUT". The error could be "EOF" or something else, which means we can't read any further.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 12, 2017

Contributor

Committed as ee6cb72.

@rewanth1997

rewanth1997 Jul 12, 2017

Contributor

Committed as ee6cb72.

scripts/openwebnet-discovery.nse
- res["MAC Address"] = string.gsub(res["MAC Address"], "(%d+)%.", function(num)
- return string.format("%02x:", num)
+ res["MAC Address"] = string.gsub(res["MAC Address"], "(%d+)(%.?)", function(num, _)
+ if _ == "." then

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 10, 2017

Don't use _ as a variable name if you reference it anywhere else. It is only for "throwaway" values that you don't intend to use.

@dmiller-nmap

dmiller-nmap Jul 10, 2017

Don't use _ as a variable name if you reference it anywhere else. It is only for "throwaway" values that you don't intend to use.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 12, 2017

Contributor

Committed as 63f9bea.

@rewanth1997

rewanth1997 Jul 12, 2017

Contributor

Committed as 63f9bea.

@rewanth1997

This comment has been minimized.

Show comment
Hide comment
@rewanth1997

rewanth1997 Jul 12, 2017

Contributor

I think its almost done. Please look into the new changes.

Contributor

rewanth1997 commented Jul 12, 2017

I think its almost done. Please look into the new changes.

scripts/openwebnet-discovery.nse
- -- Avoids false results.
- status, data = sd:receive_buf("##", true)
- break
+ if tostring(data) == "TIMEOUT" then

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 12, 2017

Why call tostring here? If status is nil, then data must be an error string, right? Same comment for line 139.

@dmiller-nmap

dmiller-nmap Jul 12, 2017

Why call tostring here? If status is nil, then data must be an error string, right? Same comment for line 139.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 13, 2017

Contributor

I thought if we get some new kind of error (I'm not sure what the new kind of error will be), converting that into tostring might help us out while printing it.

@rewanth1997

rewanth1997 Jul 13, 2017

Contributor

I thought if we get some new kind of error (I'm not sure what the new kind of error will be), converting that into tostring might help us out while printing it.

This comment has been minimized.

@rewanth1997

rewanth1997 Jul 13, 2017

Contributor

Incase we receive nil as an error due to some reason, converting data to string won't raise throw any error during printing and comparing.

@rewanth1997

rewanth1997 Jul 13, 2017

Contributor

Incase we receive nil as an error due to some reason, converting data to string won't raise throw any error during printing and comparing.

This comment has been minimized.

@dmiller-nmap

dmiller-nmap Jul 17, 2017

If we receive nil as an error, then the API we are writing to is broken and we should crash. Errors due to network input should be handled quietly; errors due to improper/incomplete programming should be reported loudly so they can be fixed.

@dmiller-nmap

dmiller-nmap Jul 17, 2017

If we receive nil as an error, then the API we are writing to is broken and we should crash. Errors due to network input should be handled quietly; errors due to improper/incomplete programming should be reported loudly so they can be fixed.

@nmap-bot nmap-bot closed this in 6b21729 Jul 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment