From 89bae56f73b076841ff0c725577534a9c7cb18e8 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 3 Feb 2026 14:24:58 +0000 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20Improve?= =?UTF-8?q?=20TCPCommunicator=20security?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Make 'host' a mandatory argument in TCPCommunicator to prevent accidental exposure. - Implement a 10MB buffer limit to prevent memory exhaustion DoS. - Fix a TypeError in TCPCommunicator logging when handling address tuples. - Update examples/tcp_server.py to bind to localhost by default. - Add unit tests for TCPCommunicator security features. Co-authored-by: ChristopheHD <16214389+ChristopheHD@users.noreply.github.com> --- .jules/sentinel.md | 4 ++ enocean/communicators/tcpcommunicator.py | 8 ++- .../tests/test_tcpcommunicator.py | 54 +++++++++++++++++++ examples/tcp_server.py | 2 +- 4 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 .jules/sentinel.md create mode 100644 enocean/communicators/tests/test_tcpcommunicator.py diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..a382dd0 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2026-02-03 - [TCP Exposure and DoS Mitigation] +**Vulnerability:** TCPCommunicator listened on all interfaces by default and had no buffer limit, leading to potential unauthorized access and DoS via memory exhaustion. +**Learning:** Defaulting to all interfaces (`''`) in network listeners is a security risk. Making the host mandatory ensures users consciously choose the binding interface. Additionally, found a Python pitfall where logging a tuple using `"%s" % (addr)` fails with `TypeError` because the tuple is unpacked; it must be `"%s" % (addr,)`. +**Prevention:** Always require binding address for network listeners and implement resource limits (buffer sizes) for all external inputs. Use defensive string formatting for logging objects that might be tuples. diff --git a/enocean/communicators/tcpcommunicator.py b/enocean/communicators/tcpcommunicator.py index 239012d..6defa28 100644 --- a/enocean/communicators/tcpcommunicator.py +++ b/enocean/communicators/tcpcommunicator.py @@ -10,10 +10,11 @@ class TCPCommunicator(Communicator): ''' Socket communicator class for EnOcean radio ''' logger = logging.getLogger('enocean.communicators.TCPCommunicator') - def __init__(self, host='', port=9637): + def __init__(self, host, port=9637): super(TCPCommunicator, self).__init__() self.host = host self.port = port + self.max_buffer_size = 10 * 1024 * 1024 def run(self): self.logger.info('TCPCommunicator started') @@ -27,7 +28,7 @@ def run(self): (client, addr) = sock.accept() except socket.timeout: continue - self.logger.debug('Client "%s" connected' % (addr)) + self.logger.debug('Client "%s" connected' % (addr,)) client.settimeout(0.5) while True and not self._stop_flag.is_set(): try: @@ -36,6 +37,9 @@ def run(self): break if not data: break + if len(self._buffer) + len(data) > self.max_buffer_size: + self.logger.warning('Buffer size limit exceeded, dropping connection') + break self._buffer.extend(bytearray(data)) self.parse() client.close() diff --git a/enocean/communicators/tests/test_tcpcommunicator.py b/enocean/communicators/tests/test_tcpcommunicator.py new file mode 100644 index 0000000..386fad4 --- /dev/null +++ b/enocean/communicators/tests/test_tcpcommunicator.py @@ -0,0 +1,54 @@ +# -*- encoding: utf-8 -*- +from __future__ import print_function, unicode_literals, division, absolute_import +import pytest +import socket +from unittest.mock import MagicMock, patch +from enocean.communicators.tcpcommunicator import TCPCommunicator + + +def test_tcp_communicator_init(): + ''' Test TCPCommunicator initialization ''' + with pytest.raises(TypeError): + TCPCommunicator() + com = TCPCommunicator('127.0.0.1') + assert com.host == '127.0.0.1' + assert com.port == 9637 + assert com.max_buffer_size == 10 * 1024 * 1024 + + +@patch('socket.socket') +def test_tcp_communicator_buffer_limit(mock_socket_cls): + ''' Test TCPCommunicator buffer limit ''' + mock_sock = MagicMock() + mock_socket_cls.return_value = mock_sock + + mock_client = MagicMock() + # accept once, then timeout to exit the outer loop (if _stop_flag is set) + mock_sock.accept.side_effect = [(mock_client, ('127.0.0.1', 12346)), socket.timeout] + + # Simulate receiving data: first chunk is fine, second chunk exceeds limit + # We use 0x55 to avoid the buffer being cleared by parse() if it thinks the data is garbage + mock_client.recv.side_effect = [b'\x55' + b'A' * 59, b'A' * 60] + + com = TCPCommunicator('127.0.0.1') + com.max_buffer_size = 100 + + # We want to stop AFTER the first client is handled. + # We can't easily do it without threading or side effects. + # Let's make the second accept call set the stop flag. + def side_effect_accept(): + if mock_sock.accept.call_count == 2: + com._stop_flag.set() + raise socket.timeout + return (mock_client, ('127.0.0.1', 12346)) + + mock_sock.accept.side_effect = side_effect_accept + + com.run() + + # recv should be called twice: once for the first chunk, once for the chunk that exceeds the limit + assert mock_client.recv.call_count == 2 + # The first chunk should be in the buffer, the second one should be dropped + assert len(com._buffer) == 60 + # Connection should be closed after breaking the loop + mock_client.close.assert_called_once() diff --git a/examples/tcp_server.py b/examples/tcp_server.py index d7fa94d..b362e68 100755 --- a/examples/tcp_server.py +++ b/examples/tcp_server.py @@ -12,7 +12,7 @@ import Queue as queue init_logging() -communicator = TCPCommunicator() +communicator = TCPCommunicator('127.0.0.1') communicator.start() while communicator.is_alive(): try: From 1497034c0df5165bb9fa7666aa5a74a0d6520835 Mon Sep 17 00:00:00 2001 From: Christophe Hardouin Duparc Date: Tue, 3 Feb 2026 15:26:10 +0100 Subject: [PATCH 2/2] Delete .jules/sentinel.md --- .jules/sentinel.md | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 .jules/sentinel.md diff --git a/.jules/sentinel.md b/.jules/sentinel.md deleted file mode 100644 index a382dd0..0000000 --- a/.jules/sentinel.md +++ /dev/null @@ -1,4 +0,0 @@ -## 2026-02-03 - [TCP Exposure and DoS Mitigation] -**Vulnerability:** TCPCommunicator listened on all interfaces by default and had no buffer limit, leading to potential unauthorized access and DoS via memory exhaustion. -**Learning:** Defaulting to all interfaces (`''`) in network listeners is a security risk. Making the host mandatory ensures users consciously choose the binding interface. Additionally, found a Python pitfall where logging a tuple using `"%s" % (addr)` fails with `TypeError` because the tuple is unpacked; it must be `"%s" % (addr,)`. -**Prevention:** Always require binding address for network listeners and implement resource limits (buffer sizes) for all external inputs. Use defensive string formatting for logging objects that might be tuples.