Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conpot new features #1 #375

Merged
merged 4 commits into from
Jun 23, 2018
Merged

Conpot new features #1 #375

merged 4 commits into from
Jun 23, 2018

Conversation

xandfury
Copy link
Member

@xandfury xandfury commented Jun 12, 2018

Second GSoC PR 🎉 This PR is more like a status update dealing with:

  • Conpot's Internal Interface. This is based on the work done by Lukas in a separate branch. I have rewritten almost the entire thing.
  • Improved and much thought through virtual file system
  • Basic WIP FTP Server

This PR does contain some 'controversial' code. I'll mark those so that we can have a discussion ;-)

NOTE:
Based on my past experience with tests, I am experimenting with something called TDD. So I am planning to write FTP tests first - even before fully implementing it.

Also, I usually squash my commits to keep the repo clean. But maybe I should commit more often?

* Some basic tests
* Improved Virtual File System
@xandfury xandfury requested a review from creolis June 12, 2018 18:38
@coveralls
Copy link

coveralls commented Jun 12, 2018

Pull Request Test Coverage Report for Build 1042

  • 48 of 53 (90.57%) changed or added relevant lines in 15 files are covered.
  • 10 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+2.4%) to 62.003%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conpot/core/init.py 7 8 87.5%
conpot/protocols/IEC104/IEC104_server.py 1 2 50.0%
conpot/protocols/enip/enip_server.py 2 3 66.67%
conpot/protocols/http/command_responder.py 4 6 66.67%
Files with Coverage Reduction New Missed Lines %
conpot/protocols/kamstrup/meter_protocol/kamstrup_server.py 1 69.44%
conpot/protocols/modbus/modbus_server.py 1 69.72%
conpot/protocols/ipmi/ipmi_server.py 1 20.35%
conpot/protocols/http/command_responder.py 2 69.83%
conpot/protocols/IEC104/IEC104_server.py 5 69.89%
Totals Coverage Status
Change from base Build 1038: 2.4%
Covered Lines: 3399
Relevant Lines: 5482

💛 - Coveralls

if self.cls.__name__ not in 'Proxy':
core_interface.protocols[self.cls] = self.wrapped

def __getattr__(self, name):
Copy link
Member Author

@xandfury xandfury Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorcery using magic methods.
Implementing getattr overrides Python's default mechanism for member access. So basically every attribute of a class that is decorated by @conpot_protocol decorator can be accessed here. This can be very powerful in case we want to emulate a system-wide phenomenon. Like for example we want to emulate a system restart (kamstrup management protocol ;-) we can set a counter here and freeze access to all protocols.

Some other uses include timing the last attack. This can be done by tracking the handle method for every protocol. Again can be easily done from here, without even touching the protocol implementation :-)

@creolis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds really nice to me -
@glaslos would you mind to think this through for a second? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creolis, @glaslos - For the downsides of this approach, please have a look at my comment below

def __repr__(self):
return self.cls.__repr__(self.wrapped)

__doc__ = property(lambda self: self.cls.__doc__)
Copy link
Member Author

@xandfury xandfury Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the downside of this approach is - every object that we create in bin/conpot is an instance this Wrapper class and no longer the instance of the actual protocol class. As you can see, I am monkey-patching the important stuff. So too many changes here and there could lead to very unexpected results.

However, unplugging a protocol from the internal interface is as easy as removing the @conpot_protocol decorator. Everything would still run perfect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I see the benefits and I am really curious how this will affect subsequent development (in terms of making things more flexible and easier to tie together).

A last question on this matter: How do you (personally) feel about debugging? Unexpected results is one thing - but I am a little unsure if this will have negative impact on how straight forward debugging issues will be in future :) So if somebody has any strong opinions on this specific matter, I'm open for input :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My objective for internal interface has always been to let protocols interact more deeply with each other. Not just for debugging. So for example if users want an option to change databus values while Conpot is still running, they can do so - if we extend the interface ;-)

IMO as long we maintain a plug/unplug mechanism, there is little to worry about. We can always go for - 'X' protocols with interface and 'Y' protocols without it. Conpot would still run all protocols like nothing has changed.

|-- common
|-- telnet
|-- http
|-- snmp
`-- ftp etc.
"""
def __init__(self):
self.data_fs = osfs.OSFS(root_path='')
self.data_fs = osfs.OSFS(root_path='') # Specify the place where you would place the uploads
Copy link
Member Author

@xandfury xandfury Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyfilesystem2 is pretty cool. You can specify something like ftp://will:[email protected]/private to store the uploads directly to a FTP server.
You can even do this - zip://projects.zip or tar://projects.tar to store directly to a zip/tar file. I haven't tested these URLs though.

Question: What should we keep the deafult storage URL here? Since we are creating an instance of this VirtualFS class in conpot.core.__init__.py, I was not sure about the default file, directory - could use some advice here ;-)
@creolis
cc: @glaslos

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean where the default directory for the VFS will reside?
$cwd/vfs seems appropriate, ... or do I miss an obvious tripwire?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to push this default directory to conpot repo. Would a conpot_vfs.tar file here be okay with you?

logger = logging.getLogger(__name__)


class ProxyDecoder(abc.ABC):
Copy link
Member Author

@xandfury xandfury Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ABCs for pluggable proxy decoders

- Use AbstractFS.home_fs object to access the chroot jail FS
- Use AbstractFS.root_fs object to access the global '/' FS

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some really good stuff I thought of when I was beginning to experiment with FTP. Use AbstractFS.root_fs to access '/' of the vfs and use AbstractFS.home_fs to access a protocol specific chroot jail FS. AbstractFS.home_fs is basically /' + 'protocol_name' directory. But if a protocol tries to access ../ from home_fs, an implicit Illegal back reference exception is raised.
AbstractFS also wraps around os.* command. So you can do AbstractFS.mkdir(path) etc.

Made a mistake here: Use AbstractFS.home_fs object attribute to access the chroot jail FS.

raise NotImplementedError

def chmod(self, path, mode):
Copy link
Member Author

@xandfury xandfury Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simulating chmod in vfs : Shoud we allow this? If not what should we return?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should definitely allow this. Some protocols (like FTP) will make use of it and it might be very strange if you're able to upload, but not able to change permissions (unless you're all of the sudden not the owner of the file anymore)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wouldn't it expose us to privledge escalation exploits? Since we're accepting arbitary uploads from potential hackers ;-) I was thinking about maybe faking it somehow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I was still thinking of the final VFS approach as a "really" virtual filesystem where all content it stored in virtual files and permissions / other metadata is stored held within memory (or stored permanently in a descriptor file) in order to simulate file access. Of course we should not render ourselves prone to exploitation - but we have to provide the current set of typical FTP commands - and chmod is usually allowed.

Copy link
Member Author

@xandfury xandfury Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, pyfilesystem would allow us to create FS in memory. It is called memoryFS in their terms.
Actually, storing FS in memory could be better than storing FS on disk (faster access etc.). However FS can be bulky (given the number of protocols), do you think is is okay have that on RAM?

There would be an additional overhead of flushing the filesystem when conpot crashes.
The current implementation creates the protocol_fs in /tmp. So we don't have to worry about anything.

I'll have a look into how this can be faked. I'll let you know about any findings :-)

* Guardian AST tests
* Modbus function 43
* Several 501 and 404 tests for HTTP server
* Proxy decoder tests
@@ -12,6 +10,13 @@ omit =
conpot/core/loggers/stix_transform.py
conpot/core/loggers/taxii_log.py
conpot/utils/mac_addr.py
# New features.. Just to avoid coverage drop - before we actually write some tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're starting to take drops in coverage personally, do you? ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe. No. Not really 😉

@creolis creolis merged commit 59a6b1a into mushorg:py3 Jun 23, 2018
@creolis
Copy link
Member

creolis commented Jun 23, 2018

I merged, even though there is one open question, but this is more a thing to debate on and maybe add some notes to the docs.

In general, since there is the decorator now, I think we should add a general description and "DOs and DONTs" for future developers to the docs, hinting them about possible tripwires.

All in all I'm more then happy with the PR <3

@xandfury
Copy link
Member Author

Thanks for the merge :-)

@xandfury xandfury deleted the new-features branch July 13, 2018 08:00
srenfo pushed a commit to srenfo/conpot that referenced this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants