848417 - (CVE-2013-0799) Mozilla Maintenance Service buffer overflow allowing privilege escalation

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Frédéric Hoguin]

Attachment: Proof of Concept source code and executable

The Mozilla Maintenance Service is vulnerable to a stack-based buffer overflow, allowing an unprivileged user to escalate privileges to NT_AUTHORITY\SYSTEM.

The bug lies in workmonitor.cpp, in the ProcessSoftwareUpdateCommand function, at lines 307-308, where a fixed size array is passed to GetInstallationDir:

WCHAR installDir[MAX_PATH] = {L'\0'};
if (!GetInstallationDir(argc, argv, installDir)) {

GetInstallationDir, in turn, performs an unchecked copy of argv[2] into the destination buffer (here, installDir):

static BOOL
GetInstallationDir(int argcTmp, LPWSTR *argvTmp, WCHAR aResultDir[MAX_PATH])
{
  if (argcTmp < 2) {
    return FALSE;
  }
  wcscpy(aResultDir, argvTmp[2]);


Maintenanceservice.exe wrongly assumes that its arguments have a valid size. However, due to the way the maintenance service is configured, any unprivileged user can start it, with arbitrary arguments: this is why Firefox can use it to upgrade without triggering an UAC prompt.

In the ProcessSoftwareUpdateCommand stack layout, the installDir buffer is located 588 bytes before the security cookie used to protect against stack based buffer overflows. Therefore, an argv[2] greater than 588 bytes will trigger a process termination due to this protection, and an event will be added to the Windows' event log. The attached PoC triggers the bug with a 592 bytes long argv[2].

The event can later be viewed with Windows' event viewer, under the Windows logs > Application category:

Problem signature:
P1: maintenanceservice.exe => exe filename
P2: 19.0.0.4794            => exe version
P3: 511eba22               => exe timestamp
[...]
P7: 0000313b               => faulting offset from ImageBase, this is the instruction right after __security_check_cookie in ProcessSoftwareUpdateCommand
P8: c0000409               => Exception type: Stack buffer overflow

Although it may look like the bug is not exploitable due to the stack overflow protection, it is indeed possible to exploit this bug:
-The service can be launched as many times as needed: the attacker gets as many tries as needed.
-The security cookie (stack overflow protection) is predictable (see below).
-ASLR and DEP are useless (see below).

Stack overflow protection:
The stack overflow protection works by generating a 32 bits integer which is called the security cookie. It is supposed to be random, however it is not. It is obtained by XORing the result of GetSystemTimeAsFileTime, GetTickCount, QueryPerformanceCounter, the process ID, and the thread ID:
-An attacker can call the 3 functions just before launching the service, and the results will be nearly identical, as they are all time-based. Even if there is a difference, it'll affect only the lowest bits of the security cookie.
-The process ID is predictable, as is the thread ID. Furthermore, they are low values, never superior to 1,000,000, and as such will affect only the lowest bits of the security cookie.

This makes the real entropy of the security cookie 16 bits, low enough to make an attack practical.

ASLR and DEP:
DEP is defeated with a ROP payload.
ASLR is useless because the base addresses of some DLLs like ntdll and kernel32 are randomized at boot time, and don't change until next reboot. An attacker running can just call GetModuleHandle to get their base addresses, and adapt the payload.

This was tested on Windows 7 64 bits, and it should work on all versions of Windows.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Wip

Uses wcsncpy instead of wcscpy. One thing I don't remember off-hand is whether stack allocated buffers are zero'd out automatically on Windows, or if we need to do a memset to guarantee that installDir[MAX_PATH + 1] is null.

Comment 2

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #1)
> One thing I don't remember off-hand is
> whether stack allocated buffers are zero'd out automatically on Windows, or
> if we need to do a memset to guarantee that installDir[MAX_PATH + 1] is null.

Sorry, I meant to say: "that installDir[MAX_PATH] is null" (since installDir has a size of MAX_PATH + 1).

If buffers on the stack aren't zero'd out automatically, it might be preferable to simply set installDir[MAX_PATH] to L'\0' instead of doing a memset. wcsncpy takes care of the characters between the end of the string and MAX_PATH - 1 by setting them to null.

Comment 3

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

Comment on attachment 721917 [details] [diff] [review]
Wip

Let's get bbondy's input on this

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 721917 [details] [diff] [review]
Wip

Review of attachment 721917 [details] [diff] [review]:
-----------------------------------------------------------------

Stack allocated arrays (and heap allocated arrays) are not zero'd out by default.
If you initialize an array though with less elements than are in the array, then the rest of the elements will be initialized with 0's. 
So you could use memset, ZeroMemory, ..., but just doing = { L'\0' } is fine.

Comment 5

[Brian R. Bondy [:bbondy]]

This is

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

In this patch we also initialize the updateStatusFilePath buffer to L'\0' in IsStatusApplying. I think this is ready for a review.

It might be good to mention that this is a case of local privilege escalation. I don't believe that this can be exploited remotely, unless an attacker has gained the ability to launch services on the remote system through other means.

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 722057 [details] [diff] [review]
Patch

Review of attachment 722057 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking this bug. Looks great.
Please run through try before landing.

Comment 8

[Stephen A Pohl [:spohl]]

The patch built fine, the test slaves seem to be awfully backed up however:
https://tbpl.mozilla.org/?tree=Try&rev=9c1c61be98e0

I ran xpcshell tests yesterday on the wip patch and the tests were green:
https://tbpl.mozilla.org/?tree=Try&rev=c6301284b6af

I'd appreciate it if someone could take it from here and land the patch if the feedback from try is sufficient. I don't have push privileges to inbound or trunk.

Comment 9

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

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8db16b10212

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

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

Since this bug affects old versions going back quite a way and not just trunk, it should have received a security rating before checkin after getting sec-approval+ on the patch or should have been set sec-approval? before any code was checked in. 

We will have to backport this to older versions and may need to do it on all affected before the next release so we don't expose ourselves.

See https://wiki.mozilla.org/Security/Bug_Approval_Process for process.

Comment 13

[Alex Keybl [:akeybl]]

From our read of the bug, this exploit requires an attacker to have access to the computer as an unprivileged user. We typically de-prioritize exploits requiring that the user already pwned a computer, unless the fix is low risk. Can we get approval nominations on this bug with a risk assessment asap?

Comment 14

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

Comment on attachment 722057 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 307181
User impact if declined: possible priv-escalation
Testing completed (on m-c, etc.): on m-c... there are already tests to make sure the maintenance service works.
Risk to taking this patch (and alternatives if risky): small risk since this has baked on m-c and there are tests that exercise the maintenance service
String or UUID changes made by this patch: None

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Low risk, lets cover all our bases then and land to branches asap.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c57ac120964
https://hg.mozilla.org/releases/mozilla-beta/rev/5b9c7be62231
https://hg.mozilla.org/releases/mozilla-esr17/rev/937117c2028d

Comment 17

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

Based on conversation with Stephen Pohl, QA has decided that a manual verification of this bug fix is not required.