Closed Bug 1473113 (CVE-2018-12379) Opened 6 years ago Closed 6 years ago

Invalid memcpy in mar_read.c

Categories

(Toolkit :: Application Update, defect, P2)

x86_64
Unspecified
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ fixed
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed

People

(Reporter: hofusec, Assigned: molly)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(2 files)

Attached file 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
...
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.
Keywords: sec-high
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
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.
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
Attachment #8991507 - Flags: review?(robert.strong.bugs)
Attachment #8991507 - Flags: review?(robert.strong.bugs) → review+
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.
Attachment #8991507 - Flags: sec-approval?
sec-approval+ for trunk. Can you nominate ESR60 and Beta patches for approval and checkin after this is on trunk a few days?
Attachment #8991507 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/21c86ca04f86
Group: toolkit-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: sec-bounty?
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
Flags: needinfo?(mhowell)
Attachment #8991507 - Flags: approval-mozilla-esr60?
Attachment #8991507 - Flags: approval-mozilla-beta?
Comment on attachment 8991507 [details] [diff] [review]
Patch

Fixes a sec-high. Approved for 62.0b16 and ESR 60.2.
Attachment #8991507 - Flags: approval-mozilla-esr60?
Attachment #8991507 - Flags: approval-mozilla-esr60+
Attachment #8991507 - Flags: approval-mozilla-beta?
Attachment #8991507 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2018-12379
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: