From 09e14adaf5675f7035d58910dbcdc703ae1512d5 Mon Sep 17 00:00:00 2001 From: starwarsivan Date: Wed, 17 Dec 2025 13:31:33 +0100 Subject: [PATCH 1/5] fix(framer): validate Modbus TCP protocol ID Add protocol ID check in FramerSocket to reject non-zero values, and include a test to verify that invalid messages produce empty results. --- pymodbus/framer/socket.py | 4 ++++ test/framer/test_framer.py | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/pymodbus/framer/socket.py b/pymodbus/framer/socket.py index 89230b361..21d578835 100644 --- a/pymodbus/framer/socket.py +++ b/pymodbus/framer/socket.py @@ -25,6 +25,10 @@ def decode(self, data: bytes) -> tuple[int, int, int, bytes]: Log.debug("Very short frame (NO MBAP): {} wait for more data", data, ":hex") return 0, 0, 0, self.EMPTY tid = int.from_bytes(data[0:2], 'big') + pid = int.from_bytes(data[2:4], 'big') + if pid != 0: + Log.error("Invalid Modbus TCP protocol id: {}", pid) + return 0, 0, 0, self.EMPTY msg_len = int.from_bytes(data[4:6], 'big') + 6 dev_id = int(data[6]) if data_len < msg_len: diff --git a/test/framer/test_framer.py b/test/framer/test_framer.py index 10a96f6b5..93103ccef 100644 --- a/test/framer/test_framer.py +++ b/test/framer/test_framer.py @@ -470,3 +470,23 @@ def test_framer_encode(self, test_framer, msg): actual = test_framer.buildFrame(message) assert msg == actual + + def test_invalid_protocol_id_for_framer_socket(self): + """Test that ModbusSocketFramer rejects an invalid Protocol ID.""" + from pymodbus.framer.socket import FramerSocket + framer = FramerSocket(DecodePDU(False)) + + # Construct a Modbus TCP header with invalid Protocol ID (nonzero) + # Transaction ID = 1 + # Protocol ID = 1 (invalid, expected 0) + # Length = 3 + # Unit = 1 + # Function code = 3 + data = b"\x00\x01\x00\x01\x00\x03\x01\x03\x00" + + msg_len, dev_id, tid, pdu = framer.decode(data) + + assert msg_len == 0 + assert dev_id == 0 + assert tid == 0 + assert pdu == framer.EMPTY \ No newline at end of file From 3741a52e2ee6f5eec1432a2beecd515115e52874 Mon Sep 17 00:00:00 2001 From: starwarsivan Date: Wed, 17 Dec 2025 13:42:01 +0100 Subject: [PATCH 2/5] test(framer): correct protocol ID values in FramerSocket tests Updated two test vectors that previously used fixed, nonzero protocol IDs. Now the vectors use Protocol ID = 0, in compliance with the Modbus TCP specification --- test/framer/test_framer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/framer/test_framer.py b/test/framer/test_framer.py index 93103ccef..a150e0b9f 100644 --- a/test/framer/test_framer.py +++ b/test/framer/test_framer.py @@ -414,8 +414,8 @@ async def test_handleFrame1(self, test_framer): @pytest.mark.parametrize(("is_server"), [True]) @pytest.mark.parametrize(("entry", "msg"), [ - (FramerType.SOCKET, b"\x00\x01\x12\x34\x00\x06\xff\x02\x01\x02\x00\x08"), - (FramerType.TLS, b"\x00\x01\x12\x34\x00\x06\xff\x02\x01\x02\x00\x08"), + (FramerType.SOCKET, b"\x00\x01\x00\x00\x00\x06\xff\x02\x01\x02\x00\x08"), + (FramerType.TLS, b"\x00\x01\x00\x00\x00\x06\xff\x02\x01\x02\x00\x08"), (FramerType.RTU, b"\x00\x01\x00\x00\x00\x01\xfc\x1b"), (FramerType.ASCII, b":F7031389000A60\r\n"), ]) From 8e2c28b4c19fdddd90fc76133e358c166c5334f2 Mon Sep 17 00:00:00 2001 From: starwarsivan Date: Wed, 17 Dec 2025 19:15:43 +0100 Subject: [PATCH 3/5] Updated additional test where raw messages where using protocol id set to 0x1234 and did not follow standard --- test/framer/test_extras.py | 12 ++++++------ test/transaction/test_transaction.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/framer/test_extras.py b/test/framer/test_extras.py index e9ee01d62..5cd4a82c6 100755 --- a/test/framer/test_extras.py +++ b/test/framer/test_extras.py @@ -43,7 +43,7 @@ def setup_method(self): def test_tcp_framer_transaction_half2(self): """Test a half completed tcp frame transaction.""" - msg1 = b"\x00\x01\x12\x34\x00\x06\xff" + msg1 = b"\x00\x01\x00\x00\x00\x06\xff" msg2 = b"\x02\x01\x02\x00\x08" used_len, pdu = self._tcp.handleFrame(msg1, 0, 0) assert not pdu @@ -55,7 +55,7 @@ def test_tcp_framer_transaction_half2(self): def test_tcp_framer_transaction_half3(self): """Test a half completed tcp frame transaction.""" - msg1 = b"\x00\x01\x12\x34\x00\x06\xff\x02\x01\x02\x00" + msg1 = b"\x00\x01\x00\x00\x00\x06\xff\x02\x01\x02\x00" msg2 = b"\x08" used_len, pdu = self._tcp.handleFrame(msg1, 0, 0) assert not pdu @@ -68,7 +68,7 @@ def test_tcp_framer_transaction_half3(self): def test_tcp_framer_transaction_short(self): """Test that we can get back on track after an invalid message.""" msg1 = b'' - msg2 = b"\x00\x01\x12\x34\x00\x06\xff\x02\x01\x02\x00\x08" + msg2 = b"\x00\x01\x00\x00\x00\x06\xff\x02\x01\x02\x00\x08" used_len, pdu = self._tcp.handleFrame(msg1, 0, 0) assert not pdu assert not used_len @@ -79,21 +79,21 @@ def test_tcp_framer_transaction_short(self): def test_tcp_framer_transaction_wrong_id(self): """Test a half completed tcp frame transaction.""" - msg = b"\x00\x01\x12\x34\x00\x06\xff\x02\x01\x02\x00\x08" + msg = b"\x00\x01\x00\x00\x00\x06\xff\x02\x01\x02\x00\x08" used_len, pdu = self._tcp.handleFrame(msg, 1, 0) assert not pdu assert used_len == len(msg) def test_tcp_framer_transaction_wrong_tid(self): """Test a half completed tcp frame transaction.""" - msg = b"\x00\x01\x12\x34\x00\x06\xff\x02\x01\x02\x00\x08" + msg = b"\x00\x01\x00\x00\x00\x06\xff\x02\x01\x02\x00\x08" used_len, pdu = self._tcp.handleFrame(msg, 0, 10) assert not pdu assert used_len == len(msg) def test_tcp_framer_transaction_wrong_fc(self): """Test a half completed tcp frame transaction.""" - msg = b"\x00\x01\x12\x34\x00\x06\xff\x70\x01\x02\x00\x08" + msg = b"\x00\x01\x00\x00\x00\x06\xff\x70\x01\x02\x00\x08" with pytest.raises(ModbusIOException): self._tcp.handleFrame(msg, 0, 0) diff --git a/test/transaction/test_transaction.py b/test/transaction/test_transaction.py index 873ab5f44..42dec097f 100755 --- a/test/transaction/test_transaction.py +++ b/test/transaction/test_transaction.py @@ -285,7 +285,7 @@ async def test_transaction_receiver(self, use_clc): transact.retries = 0 transact.connection_made(mock.AsyncMock()) - data = b"\x00\x00\x12\x34\x00\x06\x00\x01\x01\x02\x00\x04" + data = b"\x00\x00\x00\x00\x00\x06\x00\x01\x01\x02\x00\x04" transact.data_received(data) response = await transact.response_future assert isinstance(response, ReadCoilsResponse) From ef7bc15c78658047106dc397b871f69f16d690fe Mon Sep 17 00:00:00 2001 From: starwarsivan Date: Wed, 17 Dec 2025 19:25:49 +0100 Subject: [PATCH 4/5] Fixed protocol id test to pass pylint --- pymodbus/framer/socket.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pymodbus/framer/socket.py b/pymodbus/framer/socket.py index 21d578835..8476c6876 100644 --- a/pymodbus/framer/socket.py +++ b/pymodbus/framer/socket.py @@ -25,9 +25,8 @@ def decode(self, data: bytes) -> tuple[int, int, int, bytes]: Log.debug("Very short frame (NO MBAP): {} wait for more data", data, ":hex") return 0, 0, 0, self.EMPTY tid = int.from_bytes(data[0:2], 'big') - pid = int.from_bytes(data[2:4], 'big') - if pid != 0: - Log.error("Invalid Modbus TCP protocol id: {}", pid) + if (pid := int.from_bytes(data[2:4], 'big')): + Log.error("Invalid Modbus protocol id: {}", pid) return 0, 0, 0, self.EMPTY msg_len = int.from_bytes(data[4:6], 'big') + 6 dev_id = int(data[6]) From a9da199dea49ac1bb9b985a7ee21f1786c3a0db7 Mon Sep 17 00:00:00 2001 From: starwarsivan Date: Wed, 17 Dec 2025 19:40:24 +0100 Subject: [PATCH 5/5] Removed import - not needed --- test/framer/test_framer.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/framer/test_framer.py b/test/framer/test_framer.py index a150e0b9f..77c4e82dd 100644 --- a/test/framer/test_framer.py +++ b/test/framer/test_framer.py @@ -473,7 +473,6 @@ def test_framer_encode(self, test_framer, msg): def test_invalid_protocol_id_for_framer_socket(self): """Test that ModbusSocketFramer rejects an invalid Protocol ID.""" - from pymodbus.framer.socket import FramerSocket framer = FramerSocket(DecodePDU(False)) # Construct a Modbus TCP header with invalid Protocol ID (nonzero) @@ -489,4 +488,4 @@ def test_invalid_protocol_id_for_framer_socket(self): assert msg_len == 0 assert dev_id == 0 assert tid == 0 - assert pdu == framer.EMPTY \ No newline at end of file + assert pdu == framer.EMPTY