Closed Bug 1468552 Opened 2 years ago Closed 2 years ago

BLRG-PT-18-009: Heap-Overflow in BSPatch File Handling


(Toolkit :: Application Update, defect, P1)




Tracking Status
firefox-esr60 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed


(Reporter: jvehent, Assigned: jewilde)


(Blocks 1 open bug)


(Keywords: csectype-intoverflow, sec-low, Whiteboard: [post-critsmash-triage][adv-main65-])


(1 file)

When processing a patch file, values of the file’s header were read and used to allocate memory in an insecure way.

The Firefox updater used the binary diff patch format (bspatch) . The code processing such a bspatch file was taking values from the header file and using these values to allocate memory (toolkit/mozapps/update/updater/bspatch.cpp, in MBS_ApplyPatch).

The allocation size was truncated when the different length values of the header were added and an arithmetic overflows occurred (this was only the case on a 32 bit platform, since the values were defined as uint32_t ). This led to a heap overflow when data is copied into a buffer of insufficient size due to the truncated integer length.

When dereferencing ctrlsrc->x memory was read and written out of bounds.

While exploiting this vulnerability requires providing a correctly signed MAR file, it is recommended to check all header values and check all arithmetic operations against overflows. An improved version of the implementation is available in the Chromium source code.
Assignee: nobody → jewilde
Priority: -- → P1
Should this be sec-moderate or sec-low (or maybe something less)? It seems about the same severity as the other X41 report libmar tool bugs I've been cleaning up and the issue and fix is already public knowledge
Flags: needinfo?(dveditz)
Mar file signing makes it so that to exploit this vulnerability they would first have to be able to sign a mar file with our certificate so sec-low sounds right to me.
Adds bounds checking around file reads and header values
Flags: needinfo?(dveditz)
Keywords: checkin-needed
Backed out for syntax strictness failure in bspatch.cpp:

Push with bustage:

Build log:
[task 2018-11-27T16:57:16.962Z] 16:57:16    ERROR -  /builds/worker/workspace/build/src/toolkit/mozapps/update/updater/bspatch.cpp:126:11: error: result of comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-unsigned-zero-compare]
[task 2018-11-27T16:57:16.963Z] 16:57:16     INFO -      if (c < 0 && c != count) {
[task 2018-11-27T16:57:16.963Z] 16:57:16     INFO -          ~ ^ ~
[task 2018-11-27T16:57:16.963Z] 16:57:16     INFO -  1 error generated.
Flags: needinfo?(jewilde)
Keywords: checkin-needed
Thanks Sebastian!
Sorry that slipped by me. I've updated the patch accordingly.
Flags: needinfo?(jewilde)
Keywords: checkin-needed
Group: toolkit-core-security → core-security-release
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.