Closed Bug 1468552 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Application Update, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: jvehent, Assigned: jewilde)

References

(Blocks 1 open bug)

Details

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

Attachments

(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
Status: NEW → ASSIGNED
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
https://github.com/chromium/chromium/commit/df3791021e31e0b99c5a64fea1161d0cf8aad5df
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:

https://hg.mozilla.org/integration/autoland/rev/e45d7cae915a56013eaa2f6a8f09788fa0b032c7

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&revision=2b4f08a645a0385021b64b4f995c18cb2250a5c6

Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=214159619&repo=autoland
[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
https://hg.mozilla.org/mozilla-central/rev/6798cc7aaed0
Group: toolkit-core-security → core-security-release
Status: ASSIGNED → RESOLVED
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.