diff --git a/pymodbus/framer/socket.py b/pymodbus/framer/socket.py index 89230b361..8476c6876 100644 --- a/pymodbus/framer/socket.py +++ b/pymodbus/framer/socket.py @@ -25,6 +25,9 @@ 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') + 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]) if data_len < msg_len: 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/framer/test_framer.py b/test/framer/test_framer.py index 10a96f6b5..77c4e82dd 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"), ]) @@ -470,3 +470,22 @@ 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.""" + 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 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)