georgi - hopefully not receiving bugspam Reporter
	Description • 16 years ago

in the loop of oggz_comments_decode:


len=readint(c, 0);
if (c+len>end) return -1;
c+=len

the |if| condition misbehaves for negative and/or overflowing |length| on both 32 and 64 bit linux.

this C code illustrates the problem:
char *a,*b;
volatile int l;
a=malloc(20);
b=a+40;
l= -2;
if (a+l > b) 
	printf(">\n");
else
	printf("<=\n");


opening the testcase ogg4.ogg causes:
(gdb) break oggz_comments.c:611
(gdb) p/x len
$1 = 0x9000001a
gdb) next
581        for (i=0;i<nb_fields;i++) {
(gdb) p/x c
$2 = 0x7f5e9ea44050
(gdb) x/4x c
0x7f5e9ea44050: Cannot access memory at address 0x7f5e9ea44050
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x00007f5f15ec7559 in oggz_comments_decode (oggz=0x7f5f0ebe0000, 
    serialno=2401, comments=0x7f5f0ea44007 "#", length=73)
    at /opt/pub/firefox-central/src/media/liboggz/src/liboggz/oggz_comments.c:584
584           len=readint(c, 0);

the testcase crashesh on purpose. small values of |len| like -1 will keep attacker in mapped space.

	georgi - hopefully not receiving bugspam Reporter
	Comment 1 • 16 years ago

|fish_sound_comments_decode| seems to suffer from the same construct.

	Reed Loden [:reed]
	Comment 2 • 16 years ago

Can you please make sure you check the "Reset QA Contact to default" box when you move bugs, since you apparently have JS off for bugzilla.

	georgi - hopefully not receiving bugspam Reporter
	Comment 3 • 16 years ago

ok, will try to remember.

	cajbir (:cajbir) Assignee
	Comment 4 • 16 years ago

Added liboggz/libfishsound maintainer to the CC list.

	georgi - hopefully not receiving bugspam Reporter
	Comment 5 • 16 years ago

hm, xine/mplayer eat about 2G VM and don't crash

	georgi - hopefully not receiving bugspam Reporter
	Comment 6 • 16 years ago

taras, if doable in hydras, how would look the rule for:

ANYPOINTER (+ or -) SIGNEDTYPE INEQUALITY ANYPOINTER
?

	georgi - hopefully not receiving bugspam Reporter
	Comment 7 • 16 years ago

by INEQUALITY mean one of < > <= >=

	georgi - hopefully not receiving bugspam Reporter
	Comment 8 • 16 years ago

imo this ``feature'' is a gcc dumbness no matter what the standard writes...

	Brendan Eich [:brendan]
	Comment 9 • 16 years ago

(In reply to comment #8)
> imo this ``feature'' is a gcc dumbness no matter what the standard writes...

What does the standard say?

/be

	georgi - hopefully not receiving bugspam Reporter
	Comment 10 • 16 years ago

>What does the standard say?

frankly speaking i don't know. i don't care much what it says if i can reproduce it in a testcase.

	Jim Blandy :jimb
	Comment 11 • 16 years ago

If there's some code here that doesn't conform to ISO C, or if GCC is failing to implement the standard, I don't see where.  The code simply isn't validating untrustworthy input.

This code uses int and long instead of size_t.  It uses 'int' to hold the values of 32-bit unsigned fields.  I'm left with the impression we'd find more if we looked more.

	Jim Blandy :jimb
	Comment 12 • 16 years ago

David, I'm fingering you for this for lack of a better idea who should review this.

This stops the immediate crash when visiting the attached ogg file, but then Firefox enters an infinite loop when exiting.  So there's more to this.

	georgi - hopefully not receiving bugspam Reporter
	Comment 13 • 16 years ago

ok, looks like i was wrong on gcc and i would like to withdraw my statement.

[1] so on 32 bit sizeof(void*)==sizeof(int) and in fact it doesn't matter the exact reason for the int overflow. it will overflow in the case s/int/size_t/ too.

what i wrongly found counterintuitive was 64 bit where 
sizeof(void*) == 2*sizeof(int) == 8
and
((int) -1) >= anypointer

>This code uses int and long instead of size_t
>It uses 'int' to hold the values of 32-bit unsigned fields.  I'm left with the >impression we'd find more if we looked more.

size_t is not efficient solution though it will probably kill some cases. see [1].
sure there are more constructs like this.
just wanted to point out that the subset of them where |len| >= 2^31 will cause problems with high enough probability imho.
this includes all cases similar to len=atoi(),atol()

	georgi - hopefully not receiving bugspam Reporter
	Comment 14 • 16 years ago

very similar discussion to this:

http://lwn.net/Articles/278137/
GCC and pointer overflows

http://www.kb.cert.org/vuls/id/162289
C compilers may silently discard some wraparound checks

afaict both reiterate the fact that pointer arithmetic overflows.

	Jim Blandy :jimb
	Comment 15 • 16 years ago

Pointer arithmetic certainly does overflow, and security boundary code should be much more careful than liboggz and libfishsound are.  It's easy to assume that (c+len>end) and (len>end-c) were equivalent, but as this bug shows, they're not.

The cert.org link was eye-opening; thanks very much.  But it doesn't seem hard to avoid undefined constructs with some care.  PTR+INT is undefined if the result is not within the same object as PTR; the latter expression above avoids doing pointer arithmetic with an untrusted integer.

Dumb static analysis idea: give integral types an "untrusted" flag: pointer to untrusted char; untrusted int; and so on.  Arithmetic on untrusted values would yield untrusted values.  Pointer arithmetic with untrusted integers would raise a warning.  Untrusted values become trusted after a test has established lower and upper bounds for them.  Drawbacks: the condition for becoming trusted isn't tied closely enough to the size and extent of the object being indexed to make the analysis valid; it's icky to have a type change its significance after an 'if'; a function to (say) read a 32-bit big-endian integer from an array of bytes would want to be both 'untrusted int32_t f(untrusted uint8_t *)' and 'int32_t f(int8_t *)'; and on and on.

	georgi - hopefully not receiving bugspam Reporter
	Comment 16 • 16 years ago

>give integral types an "untrusted" flag

afaict the current buzzword for this is close to "annotation aided SA"

> read a 32-bit big-endian integer from an array of bytes would want to be both

just finding all the functions/macros that read numbers and then do allocation stuff without strict validation is an interesting task:

int l=readint(stream);
int *i=new int[l]; /* l >= (-4 / 4)+1 */

int l=readint(stream);
char *i=malloc(l+1); /*l == -1 */

[3]
int l=readint(stream);
int height=readint(stream);
unsigned int total = width * height;
char *i=malloc( total ); /*l,height >= 2^16 */

the paranoid part is in [3] one can replace |readint| with |strlen| and it will work with a C string of *just* 2^16+1 == 64K+1 bytes

	David Baron :dbaron:
	Comment 17 • 16 years ago

Comment on attachment 364619 [details] [diff] [review]
Bug 480521: validate lengths in Ogg comment data.

This looks reasonable to me, but I don't know anything about this code; if somebody around here owns this, it's doublec.  Sorry I didn't notice the misdirected review request while I was traveling.

	cajbir (:cajbir) Assignee
	Comment 18 • 16 years ago

Comment on attachment 364619 [details] [diff] [review]
Bug 480521: validate lengths in Ogg comment data.

I've changed the reviewer to Conrad, the liboggz maintainer. Conrad can you take a look?

	Conrad Parker
	Comment 19 • 16 years ago

Comment on attachment 364619 [details] [diff] [review]
Bug 480521: validate lengths in Ogg comment data.

This patch looks good. Avoids overflow, and len is always positive from readint.

Applied in upstream svn.annodex.net liboggz r3882 and libfishsound r3883

	Jim Blandy :jimb
	Comment 20 • 16 years ago

I take it this bug should stay ASSIGNED until we re-import from upstream?  Does it need something on the whiteboard, to catch the attention of the person who does the import?

	cajbir (:cajbir) Assignee
	Comment 21 • 16 years ago

I'll be doing the import shortly. I'll attach it here.

	cajbir (:cajbir) Assignee
	Comment 22 • 16 years ago

Updates to latest svn for liboggz and libfishsound to pull in fix from attachment 364619 [details] [diff] [review].

	cajbir (:cajbir) Assignee
	Comment 24 • 16 years ago

I think this should be blocking1.9.1 or wanted1.9.1

	cajbir (:cajbir) Assignee
	Comment 25 • 16 years ago

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/4a7b411c31fb

	cajbir (:cajbir) Assignee
	Comment 26 • 16 years ago

I had to push a followup patch to fix breakage on win32 compilers. This is a rolled up patch that includes that.

Minor change was pushed:
http://hg.mozilla.org/mozilla-central/rev/6cc04ca21fec

	cajbir (:cajbir) Assignee
	Comment 27 • 16 years ago

Back out due to libfishsound build failure on windows

	chris hofmann
	Comment 28 • 16 years ago

> I think this should be blocking1.9.1 or wanted1.9.1

it ought to get P1 if it needs a beta cycle or P2 if it can be slipped in before the first RC

	Matthew Gregan [:kinetik]
	Comment 31 • 16 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/d65a87f103d2

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/37acc64e5b6a

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 16 years ago

I suppose this should stay private until we've shipped a beta with this fix.