515376 - missing index validation in dirac.c:dirac_parse_info()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[georgi - hopefully not receiving bugspam]

http://mxr.mozilla.org/mozilla-central/source/media/liboggz/src/liboggz/dirac.c#153

153   info->video_format = video_format = dirac_uint( &bs ); /* index */
154 
155   info->width = dirac_fsize_tbl[video_format].width;
156   info->height = dirac_fsize_tbl[video_format].height;


|dirac_uint| seems to return int32 without any bound checking so using it as index potentially may read memory or cause a crash.

can't hit the code yet.

Comment 1

[Conrad Parker]

Thanks for the report. I've added a check in upstream liboggz:

commit 164e35e743e7681fbed34c66a015a779f73176f2
Author: Conrad Parker <[e-mail]>
Date:   Thu Sep 10 22:28:32 2009 +0900

    Mozilla #515376: Check index in dirac_parse_info()

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #1)
> Thanks for the report. I've added a check in upstream liboggz:

I will pull this into bug 512327...

Comment 3

[georgi - hopefully not receiving bugspam]

> commit 164e35e743e7681fbed34c66a015a779f73176f2

the patch on github seems sane to me.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Fixed on m-c by bug 511038.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #4)
> Fixed on m-c by bug 511038.

I mean fixed by bug 512327.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't enable Dirac so this doesn't matter to us.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #6)
> We don't enable Dirac so this doesn't matter to us.

We've disabled the Dirac decoding code, not the Dirac identification code. We actually can hit the code in question, as it's in liboggz's stream identification. Try loading this file: http://diracvideo.org/download/test-streams/ogg/sage-640x360.ogg and you'll hit the code.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ok

Comment 9

[georgi - hopefully not receiving bugspam]

the code is reachable on 3.5, it is at least a crash and potentially can be exploitable - basically it is doing stuff with ptr[atoi()].

the short patch seems quite safe and dveditz can bless branch checkins.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is fixed by bug 512327 on trunk and on 1.9.2 branch.

Comment 11

[georgi - hopefully not receiving bugspam]

> This is fixed by bug 512327 on trunk and on 1.9.2 branch.

it doesn't seem fixed on 1.9.1:
https://hg.mozilla.org/releases/mozilla-1.9.1/

is this branch supported ?

Comment 12

[Daniel Veditz [:dveditz]]

We work backwards -- trunk first (no users). If that looks good land on the current 1.9.2 dev branch (some alpha users). If that looks good we can land it on the stable 1.9.1 branch (millions of users).

As it happens cpearce is landing 512327 on the 1.9.1 branch tonight, which is scary fast for the size of the patch.

Comment 13

[georgi - hopefully not receiving bugspam]

>As it happens cpearce is landing 512327 on the 1.9.1 branch tonight, which is
>scary fast for the size of the patch.

i meant backporting only this bug, the patch is very short:

http://github.com/kfish/liboggz/commit/164e35e743e7681fbed34c66a015a779f73176f2

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Fixed in 1.9.1.4 by bug 512327.

(In reply to comment #13)
> i meant backporting only this bug, the patch is very short:
> 
> http://github.com/kfish/liboggz/commit/164e35e743e7681fbed34c66a015a779f73176f2

We're trying to reduce the number of patches we maintain on top of external libraries. In many ways they're unavoidable, but fewer patches on top of external libraries makes updating them easier. 

Thanks for the bug report!

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Is there any way for QA to verify this fix for 1.9.1?

Comment 16

[Matthew Gregan [:kinetik]]

Load the file in the bug URL to exercise the code.  To make sure this particular bug was fixed you'd need to construct a file with a bogus video format field--I'm not sure how easy it'd be to do that.

Comment 17

[georgi - hopefully not receiving bugspam]

> I'm not sure how easy it'd be to do that.

off the top of my head there are too possible approach:
1. use hex editor
2. reencode with redefined constant.

i am trying with a hex editor.

Comment 18

[georgi - hopefully not receiving bugspam]

i am having troubles with the hex editor approach.

if i |memcpy| into the buffer i get the needed value.

screwing the value with hex editor doesn't hit the breakpoint at all.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Ogg checksum fix

(In reply to comment #18)
> i am having troubles with the hex editor approach.
> 
> if i |memcpy| into the buffer i get the needed value.
> 
> screwing the value with hex editor doesn't hit the breakpoint at all.

When you change the contents of an Ogg file, it's page checksums become invalid, and the pages will be rejected by the decoder. Try using this attached C++ program after you've edited the file, it will recompute the checksums and, provided your file is otherwise valid, enable your edited file to get past the checksum check.