1473113 - (CVE-2018-12379) Invalid memcpy in mar_read.c

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P2)

Description

[Holger Fuhrmannek]

Attachment: update.mar.zip

By opening a mar file which contains a very long item file name (length = max int) it's possible to trigger an invalid memcpy call. This can lead to an out-of-bounds read/write.
To reproduce extract the update.mar and try an update with an 64bit updater or use the mar tool like 'mar -t ./update.mar'

ASAN-Log:

==11754==ERROR: AddressSanitizer: negative-size-param: (size=-2147483648)
    #0 0x4bdeb4 in __asan_memcpy moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
    #1 0x500c2a in mar_insert_item  src/modules/libmar/src/mar_read.c:44:3
    #2 0x500c2a in mar_consume_index  src/modules/libmar/src/mar_read.c:109
    #3 0x500c2a in mar_read_index  src/modules/libmar/src/mar_read.c:142
    #4 0x500c2a in mar_fpopen  src/modules/libmar/src/mar_read.c:164
    #5 0x500c2a in mar_open  src/modules/libmar/src/mar_read.c:182
    #6 0x4efa94 in ArchiveReader::Open(char const*)  src/toolkit/mozapps/update/updater/archivereader.cpp:209:14
    #7 0x4f908d in UpdateThreadFunc(void*)  src/toolkit/mozapps/update/updater/updater.cpp:2480:27
...

Comment 1

[Daniel Veditz [:dveditz]]

Why aren't we validating (and in this case rejecting) the .mar signature before doing _anything_ else with a potentially evil fake update?

Haven't looked into it but let's start with "sec-high" and hope that's an over-estimate.

Comment 2

[Molly Howell (she/her) (mostly inactive)]

This is definitely a real out of bounds write. I'm not sure if it would end up being exploitable in practice, but it's definitely at least a crash.

I can't see a reason why we need to parse the index before we check the signature. I'll work on changing that.

Comment 3

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e0690b7cbd3ee03eb3882c29144ef62e332dbe

Comment 4

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdbcbe82ec4c70d020fecce49f3d1912f9988b03

Comment 5

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

Comment 6

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8991507 [details] [diff] [review]
Patch

[Security approval request comment]

How easily could an exploit be constructed based on the patch?
In my opinion the patch does not point to a specific exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I do not believe so.

Which older supported branches are affected by this flaw?
All supported branches.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports should be trivial to create and would likely be identical to the patch here; the affected code has not been under active development on any supported branch.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely because the patch is straightforward and solid automated testing coverage exists. However as with any updater patch giving it a few days on Nightly before approving any uplifts would be a worthwhile precaution.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Can you nominate ESR60 and Beta patches for approval and checkin after this is on trunk a few days?

Comment 8

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21c86ca04f868e7e8a95eac2fa9288f31ccd7fa1
Bug 1473113 - Defer initializing the MAR index until it's needed. r=rstrong

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/21c86ca04f86

Comment 10

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8991507 [details] [diff] [review]
Patch

I think it's been long enough, I haven't seen any problems or reports of problems. As I suspected it would, this patch applies cleanly to beta and ESR 60.

[ESR Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a sec-high bug.

User impact if declined:
Potential for privilege elevation via updater exploit.

Fix Landed on Version:
63

Risk to taking this patch (and alternatives if risky):
Risk is low because the patch is straightforward and has both been through the updater automated testing and also been given a trial run on nightly for several days.

String or UUID changes made by this patch: 
None

[Beta Approval Request Comment]

[Feature/Bug causing the regression]:
This bug has probably always been there; I didn't look past where the hg history starts, but it appears to have been there already by then.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
No

[List of other uplifts needed for the feature/fix]:
None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8991507 [details] [diff] [review]
Patch

Fixes a sec-high. Approved for 62.0b16 and ESR 60.2.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3659f1ac0dc
https://hg.mozilla.org/releases/mozilla-esr60/rev/76f13904a547