1184500 - (CVE-2015-4482) Out of bounds write in mar_read.c

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

Attachment: 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'

Comment 1

[Holger Fuhrmannek]

Attachment: asanlog.txt

Comment 2

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

Stephen, can you take this bug?

Comment 3

[Stephen A Pohl [:spohl]]

Sure!

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: Patch

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.

Comment 5

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

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?

Comment 6

[Stephen A Pohl [:spohl]]

(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.

Comment 7

[Stephen A Pohl [:spohl]]

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.

Comment 8

[Stephen A Pohl [:spohl]]

(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'

Comment 9

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

Sec-approval+ for trunk and we should get branch patches.

Comment 10

[Stephen A Pohl [:spohl]]

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.

Comment 11

[Stephen A Pohl [:spohl]]

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

Comment 12

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/2db081ae3185

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2db081ae3185

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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-

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c5a8abf21bb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b654784a57e2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/9042488f5118

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1bc400014f71
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3856aebece7b
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/69dcc68fad52

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/1bc400014f71
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/3856aebece7b
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/69dcc68fad52

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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?

Comment 21

[Stephen A Pohl [:spohl]]

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/

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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/