S7MultiVar.Read() fails with ErrIsoInvalidPDU

[densogiaichned]

Bug Description

When using MultiVars with total size "close" (i.e. equal) to PDU size negotiated (here 240), the function falsly reports ErrIsoInvalidPDU.
In function private int RecvIsoPacket() the received size is compared against _PduSizeRequested + IsoHSize:

Sharp7/src/Sharp7.cs

Lines 2100 to 2102 in 5d1f06b

	if ((Size > _PduSizeRequested + IsoHSize) || (Size < MinPduSize))
	_LastError = S7Consts.errIsoInvalidPDU;
	else

But the returned size in this case is larger then PDU size negotiated, thus returns error falsly.

A Wireshark capture confirms, that basically everything is fine:

S7 Communication
    Header: (Ack_Data)
        Protocol Id: 0x32
        ROSCTR: Ack_Data (3)
        Redundancy Identification (Reserved): 0x0000
        Protocol Data Unit Reference: 1280
        Parameter length: 2
        Data length: 243
        Error class: No error (0x00)
        Error code: 0x00
    Parameter: (Read Var)
    Data
        Item [1]: (Success)
            Return code: Success (0xff)
            Transport size: BYTE/WORD/DWORD (0x04)
            Length: 239
            Data […]: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

When using Snap7 library, this error doesn't happen, the checks are done differently:

int TIsoTcpSocket::CheckPDU(void *pPDU, u_char PduTypeExpected)
{
	PIsoHeaderInfo Info;
	int Size;
    ClrIsoError();
	if (pPDU!=0)
	{
		Info = PIsoHeaderInfo(pPDU);
		Size = PDUSize(pPDU);
		// Performs check
		if (( Size<7 ) || ( Size>IsoPayload_Size ) ||  // Checks RFC 1006 header length
			( Info->HLength<sizeof( TCOTP_DT )-1 ) ||  // Checks ISO 8073 header length
			( Info->PDUType!=PduTypeExpected))         // Checks PDU Type
		  return SetIsoError(errIsoInvalidPDU);
		else
		  return noError;
	}
	else
		return SetIsoError(errIsoNullPointer);
}

https://github.com/davenardella/snap7/blob/main/src/core/s7_isotcp.cpp#L55-L61

Wireshark trace

Sharp7-MultivarRead.zip

Mitigation

We disabled check against PDU size negotiated, but checked against PDU.Length instead:

    // Check 0 bytes Data Packet (only TPKT+COTP = 7 bytes)
    if (Size == IsoHSize)
        RecvPacket(PDU, 4, 3); // Skip remaining 3 bytes and Done is still false
    else
    {
        if ((Size > PDU.Length) || (Size < MinPduSize))
            _LastError = S7Consts.errIsoInvalidPDU;
        else
            Done = true; // a valid Length !=7 && >16 && <247
    }