Closed
Bug 468275
Opened 16 years ago
Closed 12 years ago
Audit alloc failure in theora xiph codec
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: timeless, Assigned: derf)
References
()
Details
(Keywords: crash, Whiteboard: [sg:audit])
Attachments
(1 file, 1 obsolete file)
2.90 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libtheora/lib/dec/decode.c?rev=79c023857355&mark=1331-1333,1975-1976, http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libtheora/lib/dec/decinfo.c?rev=79c023857355&mark=114-116,123-124,133,125-126,134,135-136,200-201, http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libtheora/lib/dec/state.c?rev=79c023857355&mark=428,702,965,1059,430,442,434,446,435-436,943,959-960,437 http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libtheora/lib/dec/info.c?rev=79c023857355&mark=65-66,78-79 http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libtheora/lib/dec/dequant.c?rev=79c023857355&mark=59,63,114-115,116-117,126 this is misuse of realloc: http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libtheora/lib/dec/info.c?rev=79c023857355&mark=59-60,61-62,
Updated•16 years ago
|
Whiteboard: [sg:investigate]
Updated•14 years ago
|
Whiteboard: [sg:investigate] → [sg:audit]
Comment 1•14 years ago
|
||
Tim, do you know if these have been addressed in libtheora?
Assignee | ||
Comment 2•14 years ago
|
||
I can't go through and re-check all of the links above to double-check, because the files no longer exist (and wouldn't be the same version if they did). I did a quick re-audit, and it appears that while most of the malloc/calloc/realloc usage got NULL checks added, one hunk was dropped. The following is upstream in the 1.2 development branch, but was not in the 1.1 release: Index: decinfo.c =================================================================== --- decinfo.c (revision 17275) +++ decinfo.c (working copy) @@ -128,6 +128,10 @@ _tc->comments*sizeof(_tc->comment_lengths[0])); _tc->user_comments=(char **)_ogg_malloc( _tc->comments*sizeof(_tc->user_comments[0])); + if(_tc->comment_lengths==NULL||_tc->user_comments==NULL){ + _tc->comments=0; + return TH_EFAULT; + } for(i=0;i<_tc->comments;i++){ len=oc_unpack_length(_opb); if(len<0||len>oc_pack_bytes_left(_opb)){
Assignee | ||
Comment 3•12 years ago
|
||
Josh asked me if we could close this bug. It had been long enough that I had forgotten what exactly had been reviewed, so I reviewed things again, and found the following problem @ media/libtheora/lib/state.c:586: ref_frame_data=oc_aligned_malloc(ref_frame_data_sz,16); frag_buf_offs=_state->frag_buf_offs= _ogg_malloc(_state->nfrags*sizeof(*frag_buf_offs)); if(ref_frame_data==NULL||frag_buf_offs==NULL){ _ogg_free(frag_buf_offs); _ogg_free(ref_frame_data); return TH_EFAULT; } I.e., ref_frame_data was being allocated with oc_aligned_malloc(), and freed with _ogg_free() instead of oc_aligned_free() on error. This doesn't work (the aligned block is guaranteed to start after a block of memory returned by _ogg_malloc(), because it stores the alignment offset there). This would only cause a problem if there was just enough memory to satisfy the reference frame allocation (just over 4.5 or 9 bytes per pixel) but not enough for the fragment buffer offets (1/16 or 1/8th byte per pixel). The size of the allocations is attacker-controlled, but nothing is stored in the buffers, so I don't see how this can be exploited, but it could cause a crash with libc's that don't validate pointers before trying to free them (e.g., glibc). This was introduced in upstream commit r17563 and local commit 57091:af89c96d (Nov. 8th 2010), so it should affect everything from Firefox 4 onwards. I've fixed this upstream in r18219. Patch for m-c forthcoming.
Assignee | ||
Comment 4•12 years ago
|
||
Updated•12 years ago
|
Attachment #602536 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 5•12 years ago
|
||
This version drops a couple of extraneous hunks (which I included in the upstream commit, but in order to keep this fix to a minimum, meant to exclude here). I'd dropped them from the update.sh patch, but not dropped the actual changes to the source files. Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=22bec58b158c Carrying forward r=kinetik
Attachment #602536 -
Attachment is obsolete: true
Attachment #602798 -
Flags: review+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/129ab3b6dff8
Target Milestone: --- → mozilla13
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/129ab3b6dff8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•12 years ago
|
||
I don't think there's any reason to keep this closed. This might be a potential DoS, but because we place limits on the maximum size of a video, the only way it can be triggered is by putting enough <video> tags on a page to actually run the system out of RAM, which would be a problem all by itself.
You need to log in
before you can comment on or make changes to this bug.
Description
•