Closed Bug 966044 Opened 6 years ago Closed 6 years ago

Unconditional buffer overflow in EbmlComposer::GenerateHeader()

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch fix (obsolete) — Splinter Review
We declare a stack buffer of size 8, then copy a string of length 8 into it. The null character smashes the stack.
Attachment #8368246 - Flags: review?(chris.double)
Comment on attachment 8368246 [details] [diff] [review]
fix

Review of attachment 8368246 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look good but needs gecko_fixes.patch to be updated for r+.

::: media/libmkv/WebMElement.c
@@ +55,5 @@
>    return t ^ r;
>  }
>  
>  void writeVideoTrack(EbmlGlobal *glob, unsigned int trackNumber, int flagLacing,
> +                     const char *codecId, unsigned int pixelWidth, unsigned int pixelHeight,

gecko_fix.patch also needs to be updated for this file.

::: media/libmkv/WebMElement.h
@@ +24,1 @@
>                       double frameRate);

You'll need to update gecko_fix.patch for these changes. :rillian may be able to advise on how this was generated.
Attachment #8368246 - Flags: review?(chris.double) → review-
The const-ification is cleaner than gecko-fix.patch. so please put the change in a new patch file and apply it from media/libmkv/update.sh. The idea is to make the patches we carry against libraries with an upstream explicit so it's easy to upgrade the underlying code.

If you're willing to submit this upstream, let me know. Otherwise I can do it.
Attachment #8368246 - Attachment is obsolete: true
Attachment #8368273 - Flags: review?(chris.double)
I liked the other better!
Attached patch first fix...fixed (obsolete) — Splinter Review
Attachment #8368297 - Flags: review?(giles)
Comment on attachment 8368297 [details] [diff] [review]
first fix...fixed

Review of attachment 8368297 [details] [diff] [review]:
-----------------------------------------------------------------

Please put the const changes in a separate const.patch instead of in gecko_fix.patch.
Attachment #8368297 - Flags: review?(giles) → review-
Attached patch fix of fixed first fix. (obsolete) — Splinter Review
Attachment #8368297 - Attachment is obsolete: true
Attachment #8368305 - Flags: review?(giles)
Attachment #8368305 - Flags: review?(giles)
Attachment #8368305 - Attachment is obsolete: true
Attachment #8368306 - Flags: review?(giles)
Comment on attachment 8368306 [details] [diff] [review]
fix of first fix fixed, fixed to add patch containing fix.

Review of attachment 8368306 [details] [diff] [review]:
-----------------------------------------------------------------

I think that fixed it.
Attachment #8368306 - Flags: review?(giles) → review+
Duplicate of this bug: 964762
(In reply to Ralph Giles (:rillian) from comment #9)
> Comment on attachment 8368306 [details] [diff] [review]
> fix of first fix fixed, fixed to add patch containing fix.
> 
> Review of attachment 8368306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that fixed it.

I could weep.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3373499773
https://hg.mozilla.org/mozilla-central/rev/7c3373499773
Assignee: nobody → efaustbmo
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8368273 [details] [diff] [review]
fix without modifying third party libraries

Review no longer needed.
Attachment #8368273 - Attachment is obsolete: true
Attachment #8368273 - Flags: review?(chris.double)
You need to log in before you can comment on or make changes to this bug.