Bug 1261776 (CVE-2016-2812)

Service Worker - Buffer overflow in get()

RESOLVED FIXED in Firefox 46

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: loobenyang, Assigned: bkelly)

Tracking

({csectype-bounds, sec-high})

48 Branch
mozilla48
x86_64
All
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox45 wontfix, firefox46+ fixed, firefox47+ fixed, firefox48+ verified, firefox-esr38 unaffected, firefox-esr45 disabled)

Details

(Whiteboard: [post-critsmash-triage][adv-main46+] btpp-active)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
Posted file BOF_Get_Repro.zip
Steps to reproduce:

(unzip BOF_Get_Repro.zip, keep BOF_Get_Repro.js, mainpage.html, svcworker0.js and svcworker2.js in the same folder)
1. Run server side script BOF_Get_Repro.js in Node.js (node BOF_Get_Repro.js ).
2. Enter http://localhost:12345 in Firefox asan build.
3. Asan reports a buffer overflow in get().



Firefox version: 48.0a1 (2016-03-26)

=================================================================
==906==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f0de0f9ea08 at pc 0x7f0dd8a7fe8e bp 0x7fff962b7550 sp 0x7fff962b7548
READ of size 8 at 0x7f0de0f9ea08 thread T0
    #0 0x7f0dd8a7fe8d in get /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:261
    #1 0x7f0dd8a7fe8d in operator[] /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:555
    #2 0x7f0dd8a7fe8d in Done /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/workers/ServiceWorkerManager.cpp:320
    #3 0x7f0dd8a7fe8d in Done /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/workers/ServiceWorkerManager.cpp:400
    #4 0x7f0dd8a7fe8d in mozilla::dom::workers::ServiceWorkerInstallJob::ContinueAfterInstallEvent(bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/workers/ServiceWorkerManager.cpp:1221
    #5 0x7f0dd8ac07d8 in mozilla::dom::workers::ContinueLifecycleRunnable::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/workers/ServiceWorkerManager.cpp:702
    #6 0x7f0dd3113710 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:994
    #7 0x7f0dd318ceba in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #8 0x7f0dd3b37d99 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:97
    #9 0x7f0dd3a9df9c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #10 0x7f0dd3a9df9c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #11 0x7f0dd3a9df9c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #12 0x7f0dd904a0c7 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #13 0x7f0ddaee9698 in nsAppStartup::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/components/startup/nsAppStartup.cpp:281
    #14 0x7f0ddafe7b4a in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsAppRunner.cpp:4342
    #15 0x7f0ddafe8db6 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsAppRunner.cpp:4439
    #16 0x7f0ddafe9bfe in XRE_main /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsAppRunner.cpp:4545
    #17 0x48a793 in do_main /builds/slave/m-cen-l64-asan-000000000000000/build/src/browser/app/nsBrowserApp.cpp:220
    #18 0x48a793 in main /builds/slave/m-cen-l64-asan-000000000000000/build/src/browser/app/nsBrowserApp.cpp:360
    #19 0x7f0dec489ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #20 0x489bcc in _start (/home/parnell/FirefoxBuilds/firefox/firefox+0x489bcc)

0x7f0de0f9ea08 is located 0 bytes to the right of global variable 'nsTArrayHeader::sEmptyHdr' from '/builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/xpcom/build/Unified_cpp_xpcom_build1.cpp' (0x7f0de0f9ea00) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:261 get
Shadow bytes around the buggy address:
  0x0fe23c1ebcf0: 00 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd00: 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd10: 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd20: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd30: 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
=>0x0fe23c1ebd40: 00[f9]f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0fe23c1ebd50: 00 00 00 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd60: 01 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd70: 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd80: 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe23c1ebd90: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  F==906==ABORTING
Reporter

Comment 1

3 years ago
It's kind of a deeper variant of Bug 1234949. From the stack  trace and line numbers we can tell, the fix of Bug 1234949 was in it.

Looking at the patch of Bug 1234949 :
  void
  Done(ServiceWorkerJob* aJob)
  {
    MOZ_ASSERT(aJob);
    QueueData& queue = GetQueue(aJob->mJobType);
	MOZ_RELEASE_ASSERT(!queue.mJobs.IsEmpty());
    MOZ_ASSERT(queue.mJobs[0] == aJob);
    if (NS_WARN_IF(queue.mJobs[0] != aJob)) {
      return;
    }
    Pop(queue);
  }

The patch did fix Bug 1234949  by preventing unmatched job poping the queue. The original test cases and fuzzers could no longer trigger the issue.

However, the fix itself is still vulnerable. What if it's matched jobs popping the queue but still pop the queue when it's empty? Then checking code "queue.mJobs[0]" itself causes out of bound access:

    if (NS_WARN_IF(queue.mJobs[0] != aJob)) {

Yes it can. I managed to trigger it with the attached test case.
Reporter

Comment 2

3 years ago
I pasted a wrong copy (my local modified copy)of code in previous comment, the patch was:

   void
   Done(ServiceWorkerJob* aJob)
   {
     MOZ_ASSERT(aJob);
     QueueData& queue = GetQueue(aJob->mJobType);
     MOZ_ASSERT(!queue.mJobs.IsEmpty());
     MOZ_ASSERT(queue.mJobs[0] == aJob);
+    if (NS_WARN_IF(queue.mJobs[0] != aJob)) {
+      return;
+    }
     Pop(queue);
   }
 };

MOZ_ASSERT in release build actually does nothing:

MOZ_ASSERT(!queue.mJobs.IsEmpty());
Group: core-security → dom-core-security
Flags: sec-bounty?
Ouch.  Pointy hat to me.  Thanks Looben!
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8738009 - Flags: review?(ehsan)
Attachment #8738009 - Flags: feedback?(loobenyang)
Reporter

Comment 5

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #4)
> Created attachment 8738009 [details] [diff] [review]
> Use SafeElementAt() in service worker job queue. r=ehsan

If you can generate a Asan build with the patch, I'll verify it on my side tonight.
Here's a try push.  I renamed the patch so it doesn't point here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51a00d9abb6
Reporter

Comment 7

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #6)
> Here's a try push.  I renamed the patch so it doesn't point here.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51a00d9abb6

Could not reproduce it any more with this build:

http://archive.mozilla.org/pub/firefox/try-builds/bkelly@mozilla.com-b51a00d9abb682905685454359e5fa41d6010d8f/try-linux64-asan/

48.0a1 (2016-04-04)
Comment on attachment 8738009 [details] [diff] [review]
Use SafeElementAt() in service worker job queue. r=ehsan

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +315,5 @@
>    {
>      MOZ_ASSERT(aJob);
>      QueueData& queue = GetQueue(aJob->mJobType);
>      MOZ_ASSERT(!queue.mJobs.IsEmpty());
>      MOZ_ASSERT(queue.mJobs[0] == aJob);

Based on this bug happening, both of these assertions are bogus, right?  As in, queue.mJobs _can_ be empty here, and therefore its first entry may not be aJob.  So at least you need to fix these assertions.  But more importantly, what are the circumstances in which our previous assumptions will break down?  IOW, what's the scenario in which the job queue will be empty by the time Done() is called?  I'm having difficulty reconciling this patch with the assumptions behind these checks...
Attachment #8738009 - Flags: review?(ehsan) → review-
I'm close to replacing all of this code anyway.  I'm inclined to wait for bug 1256428 to land.
Could we MOZ_RELEASE_ASSERT those assertions and hope it doesn't happen much in practice? Bug 1256428 doesn't look like it can be backported, but we don't want to wait for it to ride the trains to fix this issue.
Looben, can you clarify if the stack in comment 0 was from a standard ASAN build with assertions enabled?  I just want to make sure I understand.  It does seem odd that we didn't hit the assertion if your theory in comment 1 and comment 2 is correct.  Its possible we are running on a garbage this pointer or something.

Thanks.
Flags: needinfo?(loobenyang)
(In reply to Andrew McCreight [:mccr8] from comment #10)
> Could we MOZ_RELEASE_ASSERT those assertions and hope it doesn't happen much
> in practice? Bug 1256428 doesn't look like it can be backported, but we
> don't want to wait for it to ride the trains to fix this issue.

I think the issue Ehsan is raising is why didn't Looben hit the assertions in comment 0.  They should be enabled from ASAN builds.
(In reply to Ben Kelly [:bkelly] from comment #12)
> I think the issue Ehsan is raising is why didn't Looben hit the assertions
> in comment 0.  They should be enabled from ASAN builds.

Standard ASan builds are non-DEBUG.
Ehsan, are you willing to revisit your review decision based on the ASAN builds being non-debug?  If you look at the build Looben references in comment 7 you can see it has debug disabled.
Flags: needinfo?(ehsan)
(In reply to Ben Kelly [:bkelly] from comment #14)
> Ehsan, are you willing to revisit your review decision based on the ASAN
> builds being non-debug?  If you look at the build Looben references in
> comment 7 you can see it has debug disabled.

Yes, I know.

We discussed this at length on IRC.  I'm fine with replacing the check with MOZ_RELEASE_ASSERT.  Or alternatively with demoting the assertions to NS_ASSERTIONs, documenting that they can fail and adding runtime checks in places where we currently MOZ_ASSERT things about the queue.
Flags: needinfo?(ehsan)
This implements the NS_ASSERTION and comment solution.
Attachment #8738009 - Attachment is obsolete: true
Attachment #8738009 - Flags: feedback?(loobenyang)
Attachment #8738216 - Flags: review?(ehsan)
Reporter

Comment 17

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #11)
> Looben, can you clarify if the stack in comment 0 was from a standard ASAN
> build with assertions enabled?  I just want to make sure I understand.  It
> does seem odd that we didn't hit the assertion if your theory in comment 1
> and comment 2 is correct.  Its possible we are running on a garbage this
> pointer or something.
> 
> Thanks.

All my testings and stack traces are based on official optimized ASAN builds downloaded from mozilla website.
Flags: needinfo?(loobenyang)
Whiteboard: btpp-active
Attachment #8738216 - Flags: review?(ehsan) → review+
Comment on attachment 8738216 [details] [diff] [review]
Use SafeElementAt() in service worker job queue. r=ehsan

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think this race condition is easy to trigger.  I've been unable to reproduce it locally and had to rely on the timing on Looben's machine.

In addition, the out-of-bounds memory location must contain a particular pointer that we are checking against.  Its possible for this to match, but it should make it harder to exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes, I think the comment and code itself makes it pretty clear we can read an invalid index here.

Which older supported branches are affected by this flaw?

44+  We reduced the likelihood of this being a problem in bug 1234949.

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

See above.

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

The patch is small and should apply easily.

How likely is this patch to cause regressions; how much testing does it need?

Very low risk. It essentially makes an if-statement use a safe getter instead of an unsafe one.  The overall program logic is unchanged.
Attachment #8738216 - Flags: sec-approval?
Attachment #8738216 - Flags: approval-mozilla-beta?
Attachment #8738216 - Flags: approval-mozilla-aurora?
I want to sec-approve this but I wanted to see if we can get it into beta first. What do you think, Liz?

There is also the question of ESR45.
Service workers are disabled in 45ESR.
Al, I'm ok with this for beta 9 (keeping in mind that's beta 9 of 11+RC since we will have an extra week of betas now)
Flags: needinfo?(lhenry) → needinfo?(abillings)
Comment on attachment 8738216 [details] [diff] [review]
Use SafeElementAt() in service worker job queue. r=ehsan

Approvals given. Let's get this into trunk and then branches in time for this beta.
Flags: needinfo?(abillings)
Attachment #8738216 - Flags: sec-approval?
Attachment #8738216 - Flags: sec-approval+
Attachment #8738216 - Flags: approval-mozilla-beta?
Attachment #8738216 - Flags: approval-mozilla-beta+
Attachment #8738216 - Flags: approval-mozilla-aurora?
Attachment #8738216 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/453f314bd7ae
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: sec-bounty? → sec-bounty+
Group: dom-core-security → core-security-release
Flags: qe-verify+
Whiteboard: btpp-active → [post-critsmash-triage] btpp-active
Marking verified for Fx48 based on comment 7.
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] btpp-active → [post-critsmash-triage][adv-main46+] btpp-active
Alias: CVE-2016-2812
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.