From be207651294fab7f59188859eeaf3f62173b7be0 Mon Sep 17 00:00:00 2001 From: Maximilian Hils Date: Mon, 29 Mar 2021 17:28:54 +0200 Subject: [PATCH 1/3] detect recursive self-connects and stop them --- mitmproxy/addons/proxyserver.py | 7 +++- mitmproxy/proxy/server.py | 41 +++++++++++------------ test/mitmproxy/addons/test_proxyserver.py | 25 ++++++++++++++ 3 files changed, 51 insertions(+), 22 deletions(-) diff --git a/mitmproxy/addons/proxyserver.py b/mitmproxy/addons/proxyserver.py index 549d05739..3f5f1a445 100644 --- a/mitmproxy/addons/proxyserver.py +++ b/mitmproxy/addons/proxyserver.py @@ -5,7 +5,7 @@ from typing import Dict, Optional, Tuple from mitmproxy import command, controller, ctx, flow, http, log, master, options, platform, tcp, websocket from mitmproxy.flow import Error, Flow from mitmproxy.proxy import commands, events -from mitmproxy.proxy import server +from mitmproxy.proxy import server, server_hooks from mitmproxy.proxy.layers.tcp import TcpMessageInjected from mitmproxy.proxy.layers.websocket import WebSocketMessageInjected from mitmproxy.utils import asyncio_utils, human, strutils @@ -180,3 +180,8 @@ class Proxyserver: self.inject_event(event) except ValueError as e: ctx.log.warn(str(e)) + + def server_connected(self, ctx: server_hooks.ServerConnectionHookData): + # check if the outbound part of this connection appeared as a new client. + if ctx.server.sockname in self._connections: + ctx.server.error = "Stopped mitmproxy from recursively connecting to itself." diff --git a/mitmproxy/proxy/server.py b/mitmproxy/proxy/server.py index a57071274..4e3796129 100644 --- a/mitmproxy/proxy/server.py +++ b/mitmproxy/proxy/server.py @@ -177,32 +177,31 @@ class ConnectionHandler(metaclass=abc.ABCMeta): else: addr = human.format_address(command.connection.address) self.log(f"server connect {addr}") - connected_hook = asyncio_utils.create_task( - self.handle_hook(server_hooks.ServerConnectedHook(hook_data)), - name=f"handle_hook(server_connected) {addr}", - client=self.client.peername, - ) - if not connected_hook: - return # this should not be needed, see asyncio_utils.create_task + await self.handle_hook(server_hooks.ServerConnectedHook(hook_data)) - self.server_event(events.OpenConnectionCompleted(command, None)) + if errmsg := command.connection.error: + self.log(f"server connection to {addr} killed: {errmsg}") + self.server_event(events.OpenConnectionCompleted(command, f"Connection killed: {errmsg}")) + del self.transports[command.connection] + writer.close() + else: + self.server_event(events.OpenConnectionCompleted(command, None)) - # during connection opening, this function is the designated handler that can be cancelled. - # once we have a connection, we do want the teardown here to happen in any case, so we - # reassign the handler to .handle_connection and then clean up here once that is done. - new_handler = asyncio_utils.create_task( - self.handle_connection(command.connection), - name=f"server connection handler for {addr}", - client=self.client.peername, - ) - if not new_handler: - return # this should not be needed, see asyncio_utils.create_task - self.transports[command.connection].handler = new_handler - await asyncio.wait([new_handler]) + # during connection opening, this function is the designated handler that can be cancelled. + # once we have a connection, we do want the teardown here to happen in any case, so we + # reassign the handler to .handle_connection and then clean up here once that is done. + new_handler = asyncio_utils.create_task( + self.handle_connection(command.connection), + name=f"server connection handler for {addr}", + client=self.client.peername, + ) + if not new_handler: + return # this should not be needed, see asyncio_utils.create_task + self.transports[command.connection].handler = new_handler + await asyncio.wait([new_handler]) self.log(f"server disconnect {addr}") command.connection.timestamp_end = time.time() - await connected_hook # wait here for this so that closed always comes after connected. await self.handle_hook(server_hooks.ServerDisconnectedHook(hook_data)) async def handle_connection(self, connection: Connection) -> None: diff --git a/test/mitmproxy/addons/test_proxyserver.py b/test/mitmproxy/addons/test_proxyserver.py index 0e85054de..ada0f787e 100644 --- a/test/mitmproxy/addons/test_proxyserver.py +++ b/test/mitmproxy/addons/test_proxyserver.py @@ -160,3 +160,28 @@ async def test_warn_no_nextlayer(): await tctx.master.await_log("Proxy server listening at", level="info") assert tctx.master.has_log("Warning: Running proxyserver without nextlayer addon!", level="warn") await ps.shutdown_server() + + +@pytest.mark.asyncio +async def test_self_connect(): + ps = Proxyserver() + with taddons.context(ps) as tctx: + state = HelperAddon() + state.layers = [ + lambda ctx: layers.modes.ReverseProxy(ctx), + lambda ctx: layers.HttpLayer(ctx, HTTPMode.transparent), + lambda ctx: layers.modes.ReverseProxy(ctx), + ] + tctx.master.addons.add(state) + + tctx.configure(ps, listen_host="127.0.0.1", listen_port=0) + ps.running() + await tctx.master.await_log("Proxy server listening", level="info") + assert ps.server + proxy_addr = ps.server.sockets[0].getsockname()[:2] + + tctx.options.mode = f"reverse:{':'.join(str(x) for x in proxy_addr)}" + reader, writer = await asyncio.open_connection(*proxy_addr) + writer.write(b"GET / HTTP/1.1\r\n\r\n") + assert b"502 Bad Gateway" in await reader.readuntil(b"\r\n\r\n") + assert b"Stopped mitmproxy from recursively connecting" in await reader.readuntil(b"") From d0e54bc32c2e21c3841a47f5c5acbc67754518b1 Mon Sep 17 00:00:00 2001 From: Maximilian Hils Date: Tue, 30 Mar 2021 08:55:49 +0200 Subject: [PATCH 2/3] partially revert "detect recursive self-connects and stop them" This partially reverts commit be207651294fab7f59188859eeaf3f62173b7be0. The intended solution is prone to race conditions where server_connected triggers before the new connection is registered. We revert the change here, but keep the tests. --- mitmproxy/addons/proxyserver.py | 7 +----- mitmproxy/proxy/server.py | 41 +++++++++++++++++---------------- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/mitmproxy/addons/proxyserver.py b/mitmproxy/addons/proxyserver.py index 3f5f1a445..549d05739 100644 --- a/mitmproxy/addons/proxyserver.py +++ b/mitmproxy/addons/proxyserver.py @@ -5,7 +5,7 @@ from typing import Dict, Optional, Tuple from mitmproxy import command, controller, ctx, flow, http, log, master, options, platform, tcp, websocket from mitmproxy.flow import Error, Flow from mitmproxy.proxy import commands, events -from mitmproxy.proxy import server, server_hooks +from mitmproxy.proxy import server from mitmproxy.proxy.layers.tcp import TcpMessageInjected from mitmproxy.proxy.layers.websocket import WebSocketMessageInjected from mitmproxy.utils import asyncio_utils, human, strutils @@ -180,8 +180,3 @@ class Proxyserver: self.inject_event(event) except ValueError as e: ctx.log.warn(str(e)) - - def server_connected(self, ctx: server_hooks.ServerConnectionHookData): - # check if the outbound part of this connection appeared as a new client. - if ctx.server.sockname in self._connections: - ctx.server.error = "Stopped mitmproxy from recursively connecting to itself." diff --git a/mitmproxy/proxy/server.py b/mitmproxy/proxy/server.py index 4e3796129..a57071274 100644 --- a/mitmproxy/proxy/server.py +++ b/mitmproxy/proxy/server.py @@ -177,31 +177,32 @@ class ConnectionHandler(metaclass=abc.ABCMeta): else: addr = human.format_address(command.connection.address) self.log(f"server connect {addr}") - await self.handle_hook(server_hooks.ServerConnectedHook(hook_data)) + connected_hook = asyncio_utils.create_task( + self.handle_hook(server_hooks.ServerConnectedHook(hook_data)), + name=f"handle_hook(server_connected) {addr}", + client=self.client.peername, + ) + if not connected_hook: + return # this should not be needed, see asyncio_utils.create_task - if errmsg := command.connection.error: - self.log(f"server connection to {addr} killed: {errmsg}") - self.server_event(events.OpenConnectionCompleted(command, f"Connection killed: {errmsg}")) - del self.transports[command.connection] - writer.close() - else: - self.server_event(events.OpenConnectionCompleted(command, None)) + self.server_event(events.OpenConnectionCompleted(command, None)) - # during connection opening, this function is the designated handler that can be cancelled. - # once we have a connection, we do want the teardown here to happen in any case, so we - # reassign the handler to .handle_connection and then clean up here once that is done. - new_handler = asyncio_utils.create_task( - self.handle_connection(command.connection), - name=f"server connection handler for {addr}", - client=self.client.peername, - ) - if not new_handler: - return # this should not be needed, see asyncio_utils.create_task - self.transports[command.connection].handler = new_handler - await asyncio.wait([new_handler]) + # during connection opening, this function is the designated handler that can be cancelled. + # once we have a connection, we do want the teardown here to happen in any case, so we + # reassign the handler to .handle_connection and then clean up here once that is done. + new_handler = asyncio_utils.create_task( + self.handle_connection(command.connection), + name=f"server connection handler for {addr}", + client=self.client.peername, + ) + if not new_handler: + return # this should not be needed, see asyncio_utils.create_task + self.transports[command.connection].handler = new_handler + await asyncio.wait([new_handler]) self.log(f"server disconnect {addr}") command.connection.timestamp_end = time.time() + await connected_hook # wait here for this so that closed always comes after connected. await self.handle_hook(server_hooks.ServerDisconnectedHook(hook_data)) async def handle_connection(self, connection: Connection) -> None: From 945e4e3ef5b94244f11259a0522b3c876042dc2a Mon Sep 17 00:00:00 2001 From: Maximilian Hils Date: Tue, 30 Mar 2021 09:02:12 +0200 Subject: [PATCH 3/3] detect recursive self-connects and stop them (attempt 2) --- mitmproxy/addons/proxyserver.py | 12 +++++++- mitmproxy/proxy/server.py | 6 ++-- test/mitmproxy/addons/test_proxyserver.py | 34 ++++++++--------------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/mitmproxy/addons/proxyserver.py b/mitmproxy/addons/proxyserver.py index 549d05739..ff7e3426a 100644 --- a/mitmproxy/addons/proxyserver.py +++ b/mitmproxy/addons/proxyserver.py @@ -4,7 +4,7 @@ from typing import Dict, Optional, Tuple from mitmproxy import command, controller, ctx, flow, http, log, master, options, platform, tcp, websocket from mitmproxy.flow import Error, Flow -from mitmproxy.proxy import commands, events +from mitmproxy.proxy import commands, events, server_hooks from mitmproxy.proxy import server from mitmproxy.proxy.layers.tcp import TcpMessageInjected from mitmproxy.proxy.layers.websocket import WebSocketMessageInjected @@ -180,3 +180,13 @@ class Proxyserver: self.inject_event(event) except ValueError as e: ctx.log.warn(str(e)) + + def server_connect(self, ctx: server_hooks.ServerConnectionHookData): + assert ctx.server.address + self_connect = ( + ctx.server.address[1] == self.options.listen_port + and + ctx.server.address[0] in ("localhost", "127.0.0.1", "::1", self.options.listen_host) + ) + if self_connect: + ctx.server.error = "Stopped mitmproxy from recursively connecting to itself." diff --git a/mitmproxy/proxy/server.py b/mitmproxy/proxy/server.py index a57071274..865340dba 100644 --- a/mitmproxy/proxy/server.py +++ b/mitmproxy/proxy/server.py @@ -142,9 +142,9 @@ class ConnectionHandler(metaclass=abc.ABCMeta): server=command.connection ) await self.handle_hook(server_hooks.ServerConnectHook(hook_data)) - if command.connection.error: - self.log(f"server connection to {human.format_address(command.connection.address)} killed before connect.") - self.server_event(events.OpenConnectionCompleted(command, "Connection killed.")) + if err := command.connection.error: + self.log(f"server connection to {human.format_address(command.connection.address)} killed before connect: {err}") + self.server_event(events.OpenConnectionCompleted(command, f"Connection killed: {err}")) return async with self.max_conns[command.connection.address]: diff --git a/test/mitmproxy/addons/test_proxyserver.py b/test/mitmproxy/addons/test_proxyserver.py index ada0f787e..6fcf9cac3 100644 --- a/test/mitmproxy/addons/test_proxyserver.py +++ b/test/mitmproxy/addons/test_proxyserver.py @@ -5,9 +5,10 @@ import pytest from mitmproxy.addons.proxyserver import Proxyserver from mitmproxy.proxy.layers.http import HTTPMode -from mitmproxy.proxy import layers +from mitmproxy.proxy import layers, server_hooks from mitmproxy.connection import Address from mitmproxy.test import taddons, tflow +from mitmproxy.test.tflow import tclient_conn, tserver_conn class HelperAddon: @@ -162,26 +163,15 @@ async def test_warn_no_nextlayer(): await ps.shutdown_server() -@pytest.mark.asyncio -async def test_self_connect(): +def test_self_connect(): + server = tserver_conn() + client = tclient_conn() + server.address = ("localhost", 8080) ps = Proxyserver() with taddons.context(ps) as tctx: - state = HelperAddon() - state.layers = [ - lambda ctx: layers.modes.ReverseProxy(ctx), - lambda ctx: layers.HttpLayer(ctx, HTTPMode.transparent), - lambda ctx: layers.modes.ReverseProxy(ctx), - ] - tctx.master.addons.add(state) - - tctx.configure(ps, listen_host="127.0.0.1", listen_port=0) - ps.running() - await tctx.master.await_log("Proxy server listening", level="info") - assert ps.server - proxy_addr = ps.server.sockets[0].getsockname()[:2] - - tctx.options.mode = f"reverse:{':'.join(str(x) for x in proxy_addr)}" - reader, writer = await asyncio.open_connection(*proxy_addr) - writer.write(b"GET / HTTP/1.1\r\n\r\n") - assert b"502 Bad Gateway" in await reader.readuntil(b"\r\n\r\n") - assert b"Stopped mitmproxy from recursively connecting" in await reader.readuntil(b"") + # not calling .running() here to avoid unnecessary socket + ps.options = tctx.options + ps.server_connect( + server_hooks.ServerConnectionHookData(server, client) + ) + assert server.error == "Stopped mitmproxy from recursively connecting to itself."