Fix Modbus TCP protocol ID validation in FramerSocket

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:

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)
 

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

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)