[FFmpeg-devel] [PATCH] RTSP-MS 14/15: ASF packet parsing

[Ronald S. Bultje]

Hi,

On Mon, Jul 20, 2009 at 5:25 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> this code looks like it will segfault with the right input
>
[... code follows below ...]
>
> On Mon, Jun 29, 2009 at 03:21:11PM -0400, Ronald S. Bultje wrote:
>> +static void
>> +rtp_asf_fix_header(uint8_t *buf, int len)
>> +{
>> + ? ?uint8_t *p = buf, *end = buf + len;
>> +
>> + ? ?if (len < sizeof(ff_asf_guid) * 2 + 22 ||
>> + ? ? ? ?memcmp(p, ff_asf_header, sizeof(ff_asf_guid))) {
>> + ? ? ? ?return;
>> + ? ?}

If we get here, p has at least 2xGUID+22 bytes available, and starts
with the ASF header.

>> + ? ?p += sizeof(ff_asf_guid) + 14;

So now we have 1xGUID+8 bytes available.

>> + ? ?do {
[... loop code follows below ...]
>> + ? ?} while (end - p >= sizeof(ff_asf_guid) + 8);

And this checks for the same condition, so within the loop, I always
have 1xGUID+8bytes available.

Loop code:

>> +        uint64_t len = AV_RL64(p + sizeof(ff_asf_guid));
>> +        if (memcmp(p, ff_asf_file_header, sizeof(ff_asf_guid))) {
>> +            p += len;
>> +            continue;
>> +        }

Here we read exactly 1GUID + 8 bytes, and if we fail, then we re-do
the same check on the updated p. Should be OK.

If the condition passes:

>> +        /* skip most of the file header, to min_pktsize */
>> +        p += 6 * 8 + 3 * 4 + sizeof(ff_asf_guid) * 2;

Updates p, but doesn't access it. If we did, we might be reading
1xGUID+52 outside the checked area, however...

>> +        if (p + 8 <= end && AV_RL32(p) == AV_RL32(p + 4)) {
>> +            /* and set that to zero */
>> +            AV_WL32(p, 0);
>> +        }
>> +        break;

Here we re-check p to ensure we have enough data available before
accessing it again.

Should be safe, no? I know, security-newbie here...

Ronald