ilija-lazoroski commented 3 months ago

Describe the bug

Migrating plugintoolbox.build_bash_dropper I stumbled across a weird issue where multiprocessing couldn't find plugintoolbox module but this module has been installed into the SNMP plugin. In the SNMP plugin build_bash_dropper is used in partial and then passed to IHTTPAgentBinaryRegistrar.reserver_download and because the Agent doesn't have plugintoolbox it raises:

2024-03-25 12:47:55,873 [2240:ExploiterThread-01:DEBUG] snmp_exploiter.exploit_host.43: Starting the agent binary server
2024-03-25 12:47:55,875 [2240:ExploiterThread-01:ERROR] snmp_exploiter.exploit_host.53: An unexpected exception occurred while attempting to start the agent binary HTTP server:
---------------------------------------------------------------------------
Traceback (most recent call last):
  File "multiprocessing/managers.py", line 253, in serve_client
  File "multiprocessing/connection.py", line 250, in recv
ModuleNotFoundError: No module named 'plugintoolbox'
---------------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/infection_monkey_plugins_82fc38f2-a456-4c20-b713-50b87f7957a9_lORkgQtQ7CQlXwowRm0U/SNMP/snmp_exploiter.py", line 45, in exploit_host
    download_ticket = self._http_agent_binary_server_registrar.reserve_download(
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "infection_monkey/exploit/http_agent_binary_server_registrar.py", line 24, in reserve_download
    return self._server.register(operating_system, requestor_ip, agent_binary_transform)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<string>", line 2, in register
  File "multiprocessing/managers.py", line 837, in _callmethod
multiprocessing.managers.RemoteError:
---------------------------------------------------------------------------
Traceback (most recent call last):
  File "multiprocessing/managers.py", line 253, in serve_client
  File "multiprocessing/connection.py", line 250, in recv
ModuleNotFoundError: No module named 'plugintoolbox'
---------------------------------------------------------------------------
urllib3/connectionpool.py:1013: InsecureRequestWarning: Unverified HTTPS request is being made to host '10.2.2.250'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
2024-03-25 12:47:55,963 - DEBUG - file_repository_logging_decorator.py:30 - open_file() - Opening file simulation_state.json
2024-03-25 12:47:55,965 - INFO - pywsgi.py:1349 - write() - 10.2.2.250 - "GET /api/agent-signals/82fc38f2-a456-4c20-b713-50b87f7957a9 HTTP/1.1" 200 127 0.008538
2024-03-25 12:47:56,037 [2120:ExploiterThread-01:DEBUG] multiprocessing_plugin.join.118: SNMP exited with code 0
2024-03-25 12:47:56,037 [2120:ExploiterThread-01:DEBUG] multiprocessing_plugin._retrieve_return_value.140: Closing Pipe to SNMP
2024-03-25 12:47:56,037 [2120:ExploiterThread-01:DEBUG] multiprocessing_plugin._retrieve_return_value.142: Pipe to SNMP closed
2024-03-25 12:47:56,038 [2120:ExploiterThread-01:INFO] propagator._process_exploit_attempts.206: Failed to exploit or propagate to 10.2.3.20 using SNMP: An unexpected exception occurred while attempting to start the agent binary HTTP server:
---------------------------------------------------------------------------
Traceback (most recent call last):
  File "multiprocessing/managers.py", line 253, in serve_client
  File "multiprocessing/connection.py", line 250, in recv
ModuleNotFoundError: No module named 'plugintoolbox'
---------------------------------------------------------------------------

To Reproduce

Steps to reproduce the behavior:

Rebuild SNMP out of 4011-migrate-build-bash-dropper
Run SNMP Exploiter on a machine
See error

Expected behavior

This should not happen. Period 😄

Possible solutions

The simplest will be to keep build_bash_dropper in the plugin implementation. This will duplicate code both in plugintoolbox and the plugin source code until we fix it.
@mssalvatore: The best solution will probably be to pass a proxy object instead of passing the function directly. That would be unfortunate, though, because we want the plugins to remain ignorant of multiprocessing. There may be some way that the IHTTPAgentBinaryServerRegistrar can wrap the function in a proxy object before passing it across the boundary.

ilija-lazoroski commented 3 months ago

This is simplified multiissue.tar.zip. Unzip, untar and run python main.py. You should not have monkey-plugintoolbox installed anywhere.

mssalvatore commented 1 month ago

Solution

We decided on a different solution than the one described below. See https://github.com/guardicore/monkey/issues/4126#issuecomment-2117748271

I was able to make the following changes to the main.py in mutliissue.tar and get it to work:

4a5
> from multiprocessing.context import BaseContext
47a49,56
> class CallableWrapper:
>     def __init__(self, func):
>         self._func = func
> 
>     def __call__(self):
>         return self._func()
> 
> 
49c58,59
<     def __init__(self, server: StubServer):
---
>     def __init__(self, context: BaseContext, server: StubServer):
>         self._context = context
53c63,65
<         self._server.register(transform)
---
>         SyncManager.register("CallableWrapper", CallableWrapper, exposed=["__call__"])
>         manager = self._context.Manager()
>         self._server.register(manager.CallableWrapper(transform))
66c78
<     stub_registrar = StubRegistrar(stub_server)
---
>     stub_registrar = StubRegistrar(stub_server, context)

Pros of this approach

It works! At least, it does in this minimal example.
The plugin remains ignorant of the underlying dependencies of multiprocessing.

Cons of this approach

Every registration request results in a new SyncManager (process) being created

Analysis

While it's not ideal that new SyncManager processes are created per registration, it may be unavoidable. The manager process needs to be able to import the function, which means it needs to be started with the same import path as the plugin process. In order to keep plugins isolated, each plugin must therefore have its own manager process.

The new manager process appears to be automatically cleaned up when it is no longer needed.

In order to reduce the number of processes that get opened, we should modify the interface for IHTTPAgentBinaryServerRegistrar slightly. Currently, its reserve_download() method accepts an AgentBinaryTransform, which is a callable. By default, use_agent_binary() is passed. This function is essentially a NOP. Instead, the interface for reserve_download() should accept (and default to) None for the agent_binary_transform parameter. If agent_binary_transform is None, then no manager process needs to be started.

Tasks

[ ] Implement the fix as shown above in the #Solution section of this comment. Validate that it solves the original problem.
[ ] Validate that the sync manager processes get properly cleaned up when the plugin exits.
[ ] Modify the IHTTPAgentBinaryServerRegistrar and any implementations in accordance with the ##Analysis section of this comment.