1261776 - (CVE-2016-2812) Service Worker - Buffer overflow in get()

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Looben Yang]

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

Comment 1

[Looben Yang]

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.

Comment 2

[Looben Yang]

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());

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Ouch.  Pointy hat to me.  Thanks Looben!

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Use SafeElementAt() in service worker job queue. r=ehsan

Comment 5

[Looben Yang]

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Here's a try push.  I renamed the patch so it doesn't point here.

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

Comment 7

[Looben Yang]

(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/[e-mail]-b51a00d9abb682905685454359e5fa41d6010d8f/try-linux64-asan/

48.0a1 (2016-04-04)

Comment 8

[(no longer active)]

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I'm close to replacing all of this code anyway.  I'm inclined to wait for bug 1256428 to land.

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 13

[Andrew McCreight [:mccr8]]

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[(no longer active)]

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

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Use SafeElementAt() in service worker job queue. r=ehsan

This implements the NS_ASSERTION and comment solution.

Comment 17

[Looben Yang]

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 19

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

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.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Service workers are disabled in 45ESR.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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)

Comment 22

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

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.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Beta:

  https://hg.mozilla.org/releases/mozilla-beta/rev/637539f2aa3c

Aurora:

  https://hg.mozilla.org/releases/mozilla-aurora/rev/88c116e76afd

Inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/453f314bd7ae

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/453f314bd7ae

Comment 25

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

Marking verified for Fx48 based on comment 7.