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

[Michael Niedermayer]

On Mon, Jul 20, 2009 at 05:42:32PM -0400, Ronald S. Bultje wrote:
> 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.

the updated p can have any value the attacker chooses if he can make
len have any value and i think he can but maybe i miss something ...


> 
> 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.

if i did not miss something p can decrease and only the right side is checked
this may not be the only issue i dont know ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090721/0e469482/attachment.pgp>