Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Initial submission of MQTT-related code to the Nmap project. #352

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
2 participants

mogigoma commented Apr 1, 2016

I am submitting a new port definition, service probe, discovery script, and protocol library for MQTT, an Internet of Things publish-subscribe protocol. MQTT is standardized and has many implementations and public test servers.

The code in this branch has one deficiency that I suspect the reviewer(s) can help fix: it does not properly connect over TLS for either the service probe nor the discovery script.

The following command lines can be used to test multiple public test servers using multiple MQTT broker implementations both with and without user authentication.

Without user authentication and without TLS (working):

  • nmap -p 1883 --script mqtt-subscribe test.mosquitto.org
  • nmap -p 1883 --script mqtt-subscribe broker.hivemq.com
  • nmap -p 1883 --script mqtt-subscribe broker.mqttdashboard.com

With user authentication and without TLS (working):

  • nmap -PN -sV --allports -p 11638 --script mqtt-subscribe --script-args=username=nmap,password=hunter2 m10.cloudmqtt.com

Without user authentication and with TLS (not working):

  • nmap -p 8883 --script mqtt-subscribe test.mosquitto.org

If you have any questions or guidance, I will do my best to respond promptly.

Fun! I only just read about MQTT yesterday when looking for more TLS-secured protocols. I'll take a look at your submission.

mogigoma commented Apr 1, 2016

Thanks! I'm hoping to add some more protocols in the coming weeks, all for a project in an Internet of Things course.

Some feedback:

  • The IANA-reserved name for port 8883 is "secure-mqtt" so use that instead of "mqtt-tls" in the portrule and in nmap.services.
  • You can use comm.tryssl instead of creating a new socket and setting timeouts yourself, and it will transparently use TLS when appropriate. To do this, you need to either expect a banner or have data to send at the time that you call tryssl, so you may have to change your abstraction a bit because the Helper class has that info, but the Comm class is the one that needs it.
  • Update the datafiles to detect SSL on port 8883: add it to the ports line of SSLSessionReq and TLSSessionReq probes; add it to the LIKELY_SSL_PORTS in shortport.lua.
  • Be sure to use the check script or commit hooks in the Code Standards page on SecWiki to check for globals. It found a typo on line 207 of the script, for instance.
  • Could we get a reference link in the script description? I don't understand from the description what is the significance of the output.
  • We'd like all time-related script args to be able to take things like "5000ms" and "3s". You can use stdnse.parse_timespec for this. Then use nmap.clock_ms instead of os.time to get a more granular clock.

Thanks so much for using @name in your NSEdoc! Object-oriented libs can make for ugly generated docs without this.

mogigoma commented Apr 1, 2016

I'll make those changes today. Thanks for the quick turnaround on reviewing!

mogigoma commented Apr 1, 2016

I have made the recommended changes, including the stylistic ones.

mogigoma commented Apr 3, 2016

Did some minor cleanup, and added a bunch of tests for the very sensitive functions that are used for generating and parsing values.

mogigoma commented Jun 1, 2016

It's been a while, wondering if there's anything that's needed from me that would help move this along?

@nmap-bot nmap-bot closed this in ee97c8f Sep 7, 2016

nmap-bot pushed a commit that referenced this pull request Sep 7, 2016

nmap-bot pushed a commit that referenced this pull request Sep 7, 2016

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