Closed Bug 1184500 (CVE-2015-4482) Opened 5 years ago Closed 5 years ago

Out of bounds write in mar_read.c

Categories

(Toolkit :: Application Update, defect)

x86_64
Unspecified
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + verified
firefox41 + verified
firefox42 + verified
firefox-esr31 --- wontfix
firefox-esr38 40+ verified
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: hofusec, Assigned: spohl)

Details

(Keywords: sec-high, Whiteboard: [adv-main40+][adv-esr38.2+])

Attachments

(3 files)

Attached file testcase.zip
While opening a mar file it's possible to trigger an out of bounds write with a very long item file name (near 4 GB, so it's work only on 64bit).
This will happen before any signature or size check so it's can be triggered during a normal update process.
To reproduce extract the update.mar and try an update with an 64bit updater or use the mar tool like 'mar -t ./update.mar'
Attached file asanlog.txt
Stephen, can you take this bug?
Flags: needinfo?(spohl.mozilla.bugs)
Sure!
Assignee: nobody → spohl.mozilla.bugs
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch PatchSplinter Review
The length of the name is stored as a signed int, but is obtained by subtracting one pointer from another. If the result was greater than the max positive value of the signed int, we would pass along a negative name length to |mar_insert_item|, which would call |malloc| with an incorrect size.
Attachment #8634920 - Flags: review?(robert.strong.bugs)
Comment on attachment 8634920 [details] [diff] [review]
Patch

Thanks! Could you go through the libmar code to look for other cases that might be exploitable?
Attachment #8634920 - Flags: review?(robert.strong.bugs) → review+
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #5)
> Thanks! Could you go through the libmar code to look for other cases that
> might be exploitable?

Yes, will do. I'll open separate bugs in case I encounter anything else.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8634920 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Unfortunately, the patch is very straightforward. With some familiarity with our MAR format, it isn't very hard to construct an exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The commit message is kept pretty vague, but the straightforward code of the fix itself gives it away quite easily.

Which older supported branches are affected by this flaw?
This has been in the tree since we moved to Mercurial in 2007.

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?
This patch should apply to all branches. If any changes are necessary, it will be straightforward.

How likely is this patch to cause regressions; how much testing does it need?
Risk of regression is virtually 0. Manual testing & verification with the attached test case is straightforward on any OS supported by libmar.
Attachment #8634920 - Flags: sec-approval?
(In reply to Holger Fuhrmannek from comment #0)
> To reproduce extract the update.mar and try an update with an 64bit updater
> or use the mar tool like 'mar -t ./update.mar'

nit: this should read './mar -t update.mar'
Attachment #8634920 - Flags: sec-approval? → sec-approval+
Comment on attachment 8634920 [details] [diff] [review]
Patch

I just confirmed that this patch applies to all the branches for which I'm requesting approval here.

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Security bug would remain in our updater and any other products that rely on libmar.
[Describe test coverage new/current, TreeHerder]: Local testing shows that we correctly abort in the situation reported in this bug.
[Risks and why]: Virtually none.
[String/UUID change made/needed]: none.
Attachment #8634920 - Flags: approval-mozilla-esr38?
Attachment #8634920 - Flags: approval-mozilla-esr31?
Attachment #8634920 - Flags: approval-mozilla-beta?
Attachment #8634920 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2db081ae3185
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8634920 [details] [diff] [review]
Patch

Let's get this small fix on active branches. Note that ESR31 will be EOL on Aug 11 so will only receive chemspill fixes at this point. We do not need to backport to ESR31.

Beta+ Aurora+ ESR38+
ESR31-
Attachment #8634920 - Flags: approval-mozilla-esr38?
Attachment #8634920 - Flags: approval-mozilla-esr38+
Attachment #8634920 - Flags: approval-mozilla-esr31?
Attachment #8634920 - Flags: approval-mozilla-esr31-
Attachment #8634920 - Flags: approval-mozilla-beta?
Attachment #8634920 - Flags: approval-mozilla-beta+
Attachment #8634920 - Flags: approval-mozilla-aurora?
Attachment #8634920 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main40+][adv-esr38.2+]
QA Contact: florin.mezei
Alias: CVE-2015-4482
I've tried to verify this today on Windows 7 x64, using the latest available win64 Firefox 38 ESR (http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr38-win64/latest/). 
To verify I used steps from here: https://wiki.mozilla.org/Software_Update:Manually_Installing_a_MAR_file#Steps_for_Windows.

When I run: <path to outside directory>\updater.exe <path to outside directory> <path to installation directory> <path to installation directory>, the update starts to apply but then hugs all system memory and the OS hangs. Is this expected to happen even after the fix? Is this test scenario correct to verify this issue?
Flags: needinfo?(spohl.mozilla.bugs)
Yes, that's a valid scenario to verify the fix. I just confirmed this locally. You're correct that running this test will use 100% of one CPU and about 4GB of memory. After about half a minute, the updater should exit without a crash. You should see an update.log file next to <path to outside directory>\updater.exe with contents similar to:

> PATCH DIRECTORY C:\Users\spohl\Desktop\test
> INSTALLATION DIRECTORY C:\Program Files\Nightly
> WORKING DIRECTORY C:\Program Files\Nightly
> failed: 6
> calling QuitProgressUI

Another way to verify is to run mar.exe[1] directly in a cmd prompt:
> mar.exe -t update.mar

This will use 100% of one CPU, require about 4GB to run and take about half a minute. Once the mar.exe exits, you can type the following in the cmd prompt to get its return code:
> echo %errorlevel%

The value should read -1.

Prior to the fix in this bug, you should see a crash dialog appear after about half a minute of running either updater.exe or mar.exe. I confirmed this with[2].

Please let me know if you don't get the same results.

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr38-win64/latest/mar.exe
[2] http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr38-win64/1437070886/
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(florin.mezei)
Thanks for your help Stephen! I gave this another try today on the same machine (after freeing up some additional memory) and it worked as you've specified:
- Firefox 39 (x64) => updater crashed after a little while
- Firefox 40 Beta 9, latest Firefox 41 Aurora, latest Firefox 42 Nightly, latest ESR 38 tinderbox build [1] (all x64) => updater closed without crashing and the log file contents look exactly as you've specified

I used the same test scenario as in comment 20 (with updater.exe from each build). Marking this as verified.

[1] - http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr38-win64/latest/
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.