Closed
Bug 480521
Opened 16 years ago
Closed 16 years ago
liboggz and libfishsound fail to validate input
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
People
(Reporter: guninski, Assigned: cajbir)
References
Details
(Keywords: fixed1.9.1, Whiteboard: [sg:high?])
Attachments
(2 files, 5 obsolete files)
(deleted),
application/ogg
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•16 years ago
|
Component: General → Video/Audio
Product: Firefox → Core
Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:high?]
Reporter | ||
Comment 1•16 years ago
|
||
|fish_sound_comments_decode| seems to suffer from the same construct.
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.
QA Contact: general → video.audio
Reporter | ||
Comment 6•16 years ago
|
||
taras, if doable in hydras, how would look the rule for: ANYPOINTER (+ or -) SIGNEDTYPE INEQUALITY ANYPOINTER ?
Reporter | ||
Comment 8•16 years ago
|
||
imo this ``feature'' is a gcc dumbness no matter what the standard writes...
Updated•16 years ago
|
Blocks: static_analyses
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
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.
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.
Assignee: nobody → jim
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.
Attachment #364619 -
Flags: review?(dbaron)
Updated•16 years ago
|
Summary: misuse of pointer arithmetic in oggz_comments_decode → liboggz and libfishsound fail to validate input
Updated•16 years ago
|
Status: NEW → ASSIGNED
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()
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.
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.
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
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.
Attachment #364619 -
Flags: superreview+
Attachment #364619 -
Flags: review?(dbaron)
Attachment #364619 -
Flags: review?(chris.double)
Assignee | ||
Updated•16 years ago
|
Attachment #364619 -
Flags: review?(chris.double) → review?(conrad)
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?
Updated•16 years ago
|
Attachment #364619 -
Flags: review?(conrad) → review+
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
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?
Assignee | ||
Comment 22•16 years ago
|
||
Updates to latest svn for liboggz and libfishsound to pull in fix from attachment 364619 [details] [diff] [review].
Attachment #364619 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:high?] → [sg:high?][needs landing]
Assignee | ||
Comment 23•16 years ago
|
||
Assignee: jim → chris.double
Attachment #369388 -
Attachment is obsolete: true
Assignee | ||
Comment 24•16 years ago
|
||
I think this should be blocking1.9.1 or wanted1.9.1
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Assignee | ||
Comment 25•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/4a7b411c31fb
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: blocking1.9.1+ → blocking1.9.1?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:high?][needs landing] → [sg:high?][baking for 1.9.1]
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
Attachment #369938 -
Attachment is obsolete: true
Assignee | ||
Comment 27•16 years ago
|
||
Back out due to libfishsound build failure on windows
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [sg:high?][baking for 1.9.1] → [sg:high?]
Flags: blocking1.9.1? → blocking1.9.1+
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
Priority: -- → P2
Assignee | ||
Updated•16 years ago
|
Attachment #370755 -
Attachment is patch: true
Attachment #370755 -
Attachment mime type: application/octet-stream → text/plain
Comment 31•16 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/d65a87f103d2
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:high?] → [sg:high?] [needs 191 landing]
I suppose this should stay private until we've shipped a beta with this fix.
Group: core-security
Updated•16 years ago
|
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•