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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Julien Vehent]

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.

Comment 1

[June Wilde (she/her) [:jewilde]]

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

Comment 2

[Robert Strong (they/them - no direct email)]

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.

Comment 3

[June Wilde (she/her) [:jewilde]]

Attachment: Bug 1468552 - Update bspatch.cpp to match Chromium version; r?rstrong

Adds bounds checking around file reads and header values

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 5

[June Wilde (she/her) [:jewilde]]

Thanks Sebastian!
Sorry that slipped by me. I've updated the patch accordingly.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/6798cc7aaed0765f259470eb5f7cac16d4310545

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6798cc7aaed0