Last Comment Bug 468275 - Audit alloc failure in theora xiph codec
: Audit alloc failure in theora xiph codec
: crash
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
-- critical (vote)
: mozilla13
Assigned To: Timothy B. Terriberry (:derf)
: Maire Reavy [:mreavy] Please needinfo me
Depends on:
  Show dependency treegraph
Reported: 2008-12-06 15:31 PST by timeless
Modified: 2012-10-18 12:24 PDT (History)
14 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix mis-use of _ogg_free on allocation error path (4.04 KB, patch)
2012-03-02 16:44 PST, Timothy B. Terriberry (:derf)
kinetik: review+
Details | Diff | Splinter Review
Fix mis-use of _ogg_free on allocation error path v2 (2.90 KB, patch)
2012-03-04 20:31 PST, Timothy B. Terriberry (:derf)
tterribe: review+
Details | Diff | Splinter Review

Comment 1 User image cajbir (:cajbir) 2010-07-21 21:26:33 PDT
Tim, do you know if these have been addressed in libtheora?
Comment 2 User image Timothy B. Terriberry (:derf) 2010-07-21 21:46:45 PDT
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->user_comments=(char **)_ogg_malloc(
+  if(_tc->comment_lengths==NULL||_tc->user_comments==NULL){
+    _tc->comments=0;
+    return TH_EFAULT;
+  }
Comment 3 User image Timothy B. Terriberry (:derf) 2012-03-02 15:50:28 PST
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:

    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.
Comment 4 User image Timothy B. Terriberry (:derf) 2012-03-02 16:44:58 PST
Created attachment 602536 [details] [diff] [review]
Fix mis-use of _ogg_free on allocation error path
Comment 5 User image Timothy B. Terriberry (:derf) 2012-03-04 20:31:49 PST
Created attachment 602798 [details] [diff] [review]
Fix mis-use of _ogg_free on allocation error path v2

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 patch, but not dropped the actual changes to the source files.

Greenish on try:

Carrying forward r=kinetik
Comment 6 User image Timothy B. Terriberry (:derf) 2012-03-04 20:50:49 PST
Comment 7 User image Timothy B. Terriberry (:derf) 2012-03-05 16:41:40 PST
Comment 8 User image Timothy B. Terriberry (:derf) 2012-03-06 11:59:19 PST
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.

Note You need to log in before you can comment on or make changes to this bug.