Re: [NSE] Schneider NSE(modicon-info.nse)

[Daniel Miller <bonsaiviking () gmail com>]

Stephen,

Thanks for the contribution! It's always exciting to see these ICS/SCADA
devices laid bare with a little ingenuity. I have a few suggestions for
improvements that I'd like to see before we integrate this:

1. Use the shortport.port_or_service portrule with the "modbus" service
name. Version detection has a few matches for modbus, so we should be
flexible for strange cases (port forwarding, maybe?) where Modbus runs on a
port other than 502.

2. Provide a bit more detail on the values in packets you are sending. Much
like how I dissected the BACnet probes in bacnet-info recently, it's better
to use other format operators to bin.pack than "H" so that we can document
exactly what is being sent.

3. It would be nice if there were some way to make sure the device is still
responding properly during all those send-receives in init_comms. We should
at least check the return status for socket:receive(), but it would be nice
to be able to check the data, too.

4. To build a string of repeated characters, use string.rep() instead of
looping over a concatenation operator. This is not only cleaner, but more
efficient. Related, if you build a string (project_info) by looping over
concatenation, try using the strbuf library instead.

5. Chained calls to bin.unpack() where the previous pos return value is
passed directly to the next call can be rewritten into one call. So this:

  pos, project_rev_1 = bin.unpack("C", response, pos )
  pos, project_rev_2 =  bin.unpack("C", response, pos)
  pos, project_rev_3 =  bin.unpack("C", response, pos)

Is equivalent to this:

  pos, project_rev_1, project_rev_2, project_rev_3 = bin.unpack("C3",
response, pos)

6. Try the length-prefixed string format operators to bin.unpack. So this:

    pos, size = bin.unpack("C", response, pos + 1)
    pos, output["Network Module"] = bin.unpack("A" .. size, response, pos)


Becomes this:

  pos, output["Network Module"] = bin.unpack("p", response, pos + 1)

7. You can test if a string is == "" instead of checking if the length is
0, it's a bit cleaner.

8. Use the shell script or the git hooks on our Code Standards wiki page to
help find problems with global variables:
https://secwiki.org/w/Nmap/Code_Standards#Tools_to_help

Sorry for the laundry list, but I thought some of these tips might be
helpful for other script authors, too. I'll look at the other scripts
you've submitted recently and let you know if there's anything else we'd
need before including those, so watch for those messages.

Dan

On Tue, Sep 30, 2014 at 3:15 PM, Stephen Hilt <hilt () digitalbond com> wrote:

> Hello,
>
> This weekend I gave a talk at DerbyCon and released a new NSE for Schneider
> Electric Modicon Devices.
>
> The script can be found
> https://github.com/digitalbond/Redpoint/blob/master/modicon-info.nse
>
> For More information, head on over to our blog and read the post we made
> about the script.
>
> http://www.digitalbond.com/blog/2014/09/29/redpoint-schneidermodicon-plc-enumeration/
> _______________________________________________
> Sent through the dev mailing list
> http://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/