mushorg / conpot

ICS/SCADA honeypot
GNU General Public License v2.0
1.22k stars 413 forks source link

Fix ENIP test crashes and conpot_protocol snafu #487

Closed srenfo closed 4 years ago

srenfo commented 4 years ago

This PR fixes multiple issues that lead to the ENIP tests crashing without much notice. The root cause was a major bug in the conpot_protocol decorator. It also adds a few tests to catch regressions.

Another related issue is that Conpot (in both production code and test code) generally does not properly shut down greenlets. A general fix for that proved to be too big for this PR.

  1. The ENIP test setUp() method was crashing, but the exceptions weren't propagated from the greenlets, thus going unnoticed. There's even a comment that acknowledges that there's something wrong:

    # TODO: udp does not cleanly shutdown. We get OSError.

    The first commit exposes the crashes but does not fix anything just yet.

  2. The second commit fixes conpot_protocol allowing only a single instance reference per class:

    core_interface.protocols[self.cls] = self.wrapped
    [...]
    return getattr(core_interface.protocols[self.cls], name)
    

    This effectively turned every decorated class into a singleton, where earlier instances would share instance variables with the most recently created instance. This manifested itself in the ENIP tests because setUp creates instance for TCP and UDP, respectively.

  3. The third commit fixes ENIP greenlets surviving across test runs. The UDP ENIP servers were never shut down properly. This is the commit I'm least happy with because the check in handle_udp feels somewhat hacky. But it mirrors the pre-existing check in handle_tcp, except it breaks out of the function entirely. The tests now wait for the greenlets to be done. With these three fixes, the tests run and succeed without crashing. The server.stop()/greenlet.join() pattern in tearDown() should be repeated throughout the code, but as mentioned above a proper refactoring was too big for this PR. You don't want to copy&paste it everywhere because it's bound to be forgotten again. Some tests already have similar but not quite identical code.

  4. The fourth commit improves ENIP test runtime and robustness: Tests were either hitting a timeout on success or reporting success anyway. That's why the ENIP test suite took >=12 seconds. It now takes just over 1 second on my machine AND response errors are no longer masked.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 1183


Changes Missing Coverage Covered Lines Changed/Added Lines %
conpot/protocols/enip/enip_server.py 9 12 75.0%
<!-- Total: 14 17 82.35% -->
Files with Coverage Reduction New Missed Lines %
conpot/protocols/http/command_responder.py 6 66.46%
conpot/protocols/enip/enip_server.py 7 71.63%
conpot/protocols/ftp/ftp_handler.py 11 82.23%
conpot/protocols/ftp/ftp_base_handler.py 14 81.35%
<!-- Total: 38 -->
Totals Coverage Status
Change from base Build 1181: 0.8%
Covered Lines: 5610
Relevant Lines: 7589

💛 - Coveralls
creolis commented 4 years ago

Thank you for your contribution :)

LGTM @xandfury?

xandfury commented 4 years ago

LGTM! Thanks for your cotribution :slightly_smiling_face: @srenfo cc: @creolis