Errors in OverbyteIcsHttpProt

[khm123 0]

There are couple of errors in OverbyteIcsHttpProt.pas that can result in part of headers ending up in BodyData

 

// procedure THttpCli.SocketDataAvailable(Sender: TObject; ErrCode: Word);


    while FReceiveLen > 0 do begin
        I := 0;
        while (I <= FReceiveLen) and (Ord(FReceiveBuffer[I]) <> 10) do	// out of bounds
            Inc(I)
        if I > FReceiveLen then											// would never break
            break;

 

Proposed changes:

    while FReceiveLen > 0 do begin
        I := 0;                                                   // FP 09/09/06
        while (I < FReceiveLen) and (Ord(FReceiveBuffer[I]) <> 10) do
            Inc(I);                                               // FP 09/09/06
        if I >= FReceiveLen then
            break;

 

Edited Monday at 09:01 PM by khm123

[Angus Robertson 582]

Thanks, I'll have a look at how the buffer is declared. 

 

But this code has not changed in almost 20 years, have you actually seen this fail?

 

Angus

 

[khm123 0]

Yes I did, but it is so rare that it did go unnoticed for 20years 🙂

In short:

-headers have to be split in multiple packets

-break between packets have to be exacly on CRLF sequence (CR in first packet, LF in second)

-last header line data in first packet should be equal length to previous header line data

 

Observed conditions:

Headers were very long - 30+ lines. Some header lines were very long.

On first call of  SocketDataAvailable, FReceiveBuffer did not receive whole header.

After

Len := FCtrlSocket.Receive(@FReceiveBuffer[FReceiveLen], I);

Len is 1369, FReceiveBuffer[1368] = #13, FReceiveBuffer[1369] = #0

 

so everything there is as expected, 20+ full header lines and last line in buffer ending with CR (LF will come in next packet/call to SocketDataAvailable + rest of the headers)

After processing all header lines that were terminated with CRLF we observe here:

while FReceiveLen > 0 do begin
        I := 0;                                                   // FP 09/09/06
        while (I <= FReceiveLen) and (Ord(FReceiveBuffer[I]) <> 10) do // FP 09/09/06
            Inc(I);                                               // FP 09/09/06
        if I > FReceiveLen then
            break;

FReceiveLen is 32 and first 33 bytes of FReveiveBuffer are:

X-Content-Type-Options: nosniff#$D#$A

You would expect that FReceiveBuffer[32] would be #0 but it isn't probably due to incorrect parameters of

MoveTBytes(FReceiveBuffer, I + 1, 0, FReceiveLen);  // FP 09/09/06

and fact that last full header line was:

X-XSS-Protection: 1; mode=block#$D#$A

exacty 31 bytes(without CRLF) as our last unprocessed (and incomplete) header line.

It will increase I to 32, it will not break (but it should cause line is not terminated with LF - but state of buffer and errors in code says otherwise )

Rest of the code in while loop

while FReceiveLen > 0 do begin

will consider line complete and will add it to headers, FReceiveLen would go from -1 to 0 due to

FReceiveLen := FReceiveLen - I - 1;                               // FP 09/09/06
        if FReceiveLen > 0 then begin
//          Move(FReceiveBuffer[I], FReceiveBuffer[0], FReceiveLen + 1);
            MoveTBytes(FReceiveBuffer, I + 1, 0, FReceiveLen);  // FP 09/09/06
            // Debugging purpose only
            //FillChar(FReceiveBuffer[FReceiveLen], I + 1, '*');
        end
        else if FReceiveLen < 0 then                           // AG 03/19/07
            FReceiveLen := 0;                                  // AG 03/19/07

and we will go to another packet/SocketDataAvailable call

which starts with #10 and have rest of headers after that

FLastresponse would be empty and rest of headers will end up in body.

Edited 19 hours ago by khm123

[Angus Robertson 582]

Thanks, for the explanation, I see that buffer is a dynamic TBytes, unusual for 20 years ago when Delphi didn't really support TBytes.  I only started making wide use of TBytes a few years ago with a lot of new library functions. 

 

I'll fix the code, and check other receive loops for similar problems. 

 

I'm hoping to release ICS V9.4 this month, with various minor fixes.

 

Angus

 

[khm123 0]

Thank you for continued support and upgrade of ICS 👍