[Live-devel] bug in RTCPInstance::processIncomingReport (RTCP.cpp)

[Frederik De Ruyck]

Hi Ross,

unfortunately I could not find any packet that allowed me to reproduce 
the issue (feeding hard coded data to processIncomingReport with info 
I'd collected from memory right before the crash).

What looks like a quick fix to me is the following:

there is a code line in RTCP.cpp::processIncomingReport which checks if 
the remaining "packetSize" is smaller than the next chunk (defined by 
"length") =>  if (length > packetSize) break;

If so, the loop exits and so the call to processIncomingReport.

It seems to me that this check was to prevent faulty processing or 
input, though I believe in my case this is insufficient.

If "packetSize" is able to drop below zero (effectively making it a very 
big number as it is unsigned) then "length" always has a corrupted value 
because it is derived from "pkt" which is invalid since "packetSize" is < 0.

Because "packetSize" is then over four billion (it dropped below zero), 
length is very likely to be smaller, allowing the code to continue 
execution and to go rampage reading from random memory.

The check would have been ok if packetSize would be of a signed type, 
allowing it to be smaller than zero.

My suggestion is to replace "#define ADVANCE(n) pkt += (n); packetSize 
-= (n);" with "#define ADVANCE(n) if (! okPkt(pkt, totPacketSize, n)) 
break; pkt += (n); packetSize -= (n);"

and to add "okPkt":

bool RTCPInstance::okPkt(const unsigned char* const pkt, const unsigned 
totPacketSize, const int n) const
{
     const u_int8_t* const nextPkt = pkt + n;

     if ( nextPkt < fInBuf || nextPkt >= (fInBuf + maxRTCPPacketSize) )
     {
         printf("index ptr out of array bounds at incoming RTCP packet");
         return false;
     }
     else if ( nextPkt >= (fInBuf + totPacketSize) )
     {
         printf("index ptr out of packet bounds at incoming RTCP packet");
         return false;
     }
     return true;
}

Combining a memory range check with pointer arithmetic is a lot safer 
than relying on expected input.

I'll send through more info about what triggers this if I find any.

Regards,

Frederik De Ruyck

Op 14/11/2016 om 09:54 schreef Frederik De Ruyck:
> Hi,
>
> Last week I've experienced a crash at:
>
> void RTCPInstance::processIncomingReport(unsigned packetSize
>       , struct sockaddr_in const& fromAddressAndPort
>       , int tcpSocketNum
>       , unsigned char tcpStreamChannelId)
>
> at line:
>
> rtcpHdr = ntohl(*(u_int32_t*)pkt);
>
> I've upgraded to the last version of Live555 (06-11-2016) to confirm 
> that the issue is still present.
>
> I've had this crash three times with version 07-08-2016 and now today 
> a fourth time with latest 06-11-2016.
>
> I've debugged the code and the crash is of type segfault because the 
> memory dereferenced at address pkt is likely outside the application's 
> memory space.
>
> This is caused because packetSize is decremented beyond 0, the value 
> of "packetSize" at the time of the crash is 4294068404, it is of 
> unsigned type so it overflows to a huge number when it drops below 0.
>
> It is the macro ADVANCE(length) that causes the pointer "pkt" to refer 
> to an address that is beyond the scope of "fInBuf_ptr".
>
> I will now try to copy the incoming packet contents out of my debugger 
> memory editor and see if I can create a test case with that.
>
> Regards,
>
> Frederik De Ruyck
>
>
>
> This email has been scanned by BullGuard antivirus protection.
> For more info visit www.bullguard.com
>
>
>
> _______________________________________________
> live-devel mailing list
> live-devel at lists.live555.com
> http://lists.live555.com/mailman/listinfo/live-devel
>
>



This email has been scanned by BullGuard antivirus protection.
For more info visit www.bullguard.com