Closed Bug 1283674 Opened 4 years ago Closed 4 years ago

null ptr deref / access violation / crash [@ mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+0x13]

Categories

(Core :: DOM: Workers, defect, critical)

x86_64
Windows 8.1
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- disabled
firefox50 --- fixed

People

(Reporter: geeknik, Assigned: bkelly)

Details

Attachments

(5 files)

Attached file Stack Text from WinDBG
While fuzzing the latest Firefox Nightly build (Built from https://hg.mozilla.org/mozilla-central/rev/d700dc054751333e0735f975fce3d3adf153c62a) with cross_fuzz (http://lcamtuf.coredump.cx/cross_fuzz/), I encountered a null ptr dereference / access violation / crash @ mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+0x13. Flagging as security until someone confirms otherwise.

FAULTING_IP: 
xul!mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+13 [c:\builds\moz2_slave\m-cen-w64-ntly-000000000000000\build\src\dom\workers\serviceworkermanager.cpp @ 2332]
00007ffb`a0c7322f 488b5210        mov     rdx,qword ptr [rdx+10h]

EXCEPTION_RECORD:  ffffffffffffffff -- (.exr 0xffffffffffffffff)
.exr 0xffffffffffffffff
ExceptionAddress: 00007ffba0c7322f (xul!mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+0x0000000000000013)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 0000000000000000
   Parameter[1]: 0000000000000010
Attempt to read from address 0000000000000010

FAULTING_THREAD:  0000000000003d48

PROCESS_NAME:  firefox.exe

ERROR_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s.

EXCEPTION_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s.

EXCEPTION_PARAMETER1:  0000000000000000

EXCEPTION_PARAMETER2:  0000000000000010

READ_ADDRESS:  0000000000000010 

FOLLOWUP_IP: 
xul!mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+13 [c:\builds\moz2_slave\m-cen-w64-ntly-000000000000000\build\src\dom\workers\serviceworkermanager.cpp @ 2332]
00007ffb`a0c7322f 488b5210        mov     rdx,qword ptr [rdx+10h]

NTGLOBALFLAG:  70

APPLICATION_VERIFIER_FLAGS:  0

DEFAULT_BUCKET_ID:  INVALID_POINTER_READ

PRIMARY_PROBLEM_CLASS:  INVALID_POINTER_READ

BUGCHECK_STR:  APPLICATION_FAULT_INVALID_POINTER_READ_NULL_CLASS_PTR_DEREFERENCE

LAST_CONTROL_TRANSFER:  from 00007ffba0c73031 to 00007ffba0c7322f

FAULTING_SOURCE_CODE:  
No source found for 'c:\builds\moz2_slave\m-cen-w64-ntly-000000000000000\build\src\dom\workers\serviceworkermanager.cpp'


SYMBOL_STACK_INDEX:  0

SYMBOL_NAME:  xul!mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+13

FOLLOWUP_NAME:  MachineOwner

MODULE_NAME: xul

IMAGE_NAME:  xul.dll

DEBUG_FLR_IMAGE_TIMESTAMP:  57751283

STACK_COMMAND:  ~75s ; kb

FAILURE_BUCKET_ID:  INVALID_POINTER_READ_c0000005_xul.dll!mozilla::dom::workers::ServiceWorkerManager::GetDocumentController

BUCKET_ID:  X64_APPLICATION_FAULT_INVALID_POINTER_READ_NULL_CLASS_PTR_DEREFERENCE_xul!mozilla::dom::workers::ServiceWorkerManager::GetDocumentController+13

WATSON_STAGEONE_URL:  http://watson.microsoft.com/StageOne/firefox_exe/50_0_0_6025/57750c35/xul_dll/50_0_0_6025/57751283/c0000005/019c322f.htm?Retriage=1

Followup: MachineOwner
I'll take a look tomorrow.

Are null pointer derefs normally considered sec issues?  I thought that was not exploitable.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(In reply to Ben Kelly [:bkelly] from comment #2)
> Are null pointer derefs normally considered sec issues?

They aren't. It is just a good idea to double check that it really does look like one, though without a test case I guess it will be hard to figure out more than the initial report. But we can just unhide it most likely after you look it over a bit.
cross_fuzz (linked above) is the test case.

file:///d:/cross_fuzz_v3/cross_fuzz_randomized_20110105_seed.html#1000000

Ran for 3-5 hours before this crash occurred. All relevant debugging output is attached.
I think this is case where script is attempting to access navigator.serviceWorker.controller for a window that has been disconnected.  We should be checking to see if the ServiceWorkerContainer::GetOwner() method returns nullptr.
Group: core-security
This patch makes us drop our cached .controller object when the window is disconnected.  This is important in order for .controller to work consistently regardless of whether it was used before the window became disconnected.  Also, I think its just wrong to hang on to a DOM object that is associated with the window that is going away.
Attachment #8767303 - Flags: review?(amarchesini)
Attachment #8767303 - Attachment description: P1 Make ServiceWorkerContainer::GetController() work gracefully when its window is disconnected. r=baku → P0 Drop cached controlling ServiceWorker ref when ServiceWorkerContainer window is disconnect. r=baku
This lets us avoid crashing when .controller is accessed on a disconnected ServiceWorkerContainer.  I'm not sure this is fully spec'd anywhere, but I coordinated with Jake on IRC:

3:15 PM <wanderview> JakeA: what would you expect this to do?  let swc = frame.contentWindow.navigator.serviceWorker; frame.remove(); console.log(swc.controller);
3:15 PM <wanderview> JakeA: throw or just return undefined for controller?
3:17 PM <JakeA> wanderview: let's go for undefined
Attachment #8767305 - Flags: review?(amarchesini)
This adds a wpt test case.  The test fails without either P0 or P1 applied.  With P0 applied we hit the crash from comment 0.  With P0 and P1 applied we pass the test.
Attachment #8767306 - Flags: review?(amarchesini)
Attachment #8767303 - Flags: review?(amarchesini) → review+
Attachment #8767305 - Flags: review?(amarchesini) → review+
Attachment #8767306 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b538dc963f3b
P0 Drop cached controlling ServiceWorker ref when ServiceWorkerContainer window is disconnect. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2fd213837b8
P1 Make ServiceWorkerContainer::GetController() work gracefully when its window is disconnected. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28b6e2e44d1
P2 Add wpt test verifying serviceWorker.controller behavior on disconnected window. r=baku
NI myself to check if this needs uplift.  Kind of a corner case, but it's a crasher.
Flags: needinfo?(bkelly)
[Tracking Requested - why for this release]:

This appears to be a long standing bug.  Requesting tracking since its a crash, albeit a rare one.
Comment on attachment 8767305 [details] [diff] [review]
P1 Make ServiceWorkerContainer::GetController() work gracefully when its window is disconnected. r=baku

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: Sites can reliably trigger a browser crash.  The particular code is somewhat rare under normal usage, though.
[Describe test coverage new/current, TreeHerder]: New test included with patch.
[Risks and why]: Minimal.  This only affects service workers, includes a test, and basically just adds some nullptr checking.
[String/UUID change made/needed]: None.
Attachment #8767305 - Flags: approval-mozilla-beta?
Attachment #8767305 - Flags: approval-mozilla-aurora?
Comment on attachment 8767303 [details] [diff] [review]
P0 Drop cached controlling ServiceWorker ref when ServiceWorkerContainer window is disconnect. r=baku

See comment 14.
Attachment #8767303 - Flags: approval-mozilla-beta?
Attachment #8767303 - Flags: approval-mozilla-aurora?
Comment on attachment 8767306 [details] [diff] [review]
P2 Add wpt test verifying serviceWorker.controller behavior on disconnected window. r=baku

See comment 14.
Attachment #8767306 - Flags: approval-mozilla-beta?
Attachment #8767306 - Flags: approval-mozilla-aurora?
Comment on attachment 8767305 [details] [diff] [review]
P1 Make ServiceWorkerContainer::GetController() work gracefully when its window is disconnected. r=baku

Fix for potential crash in service workers, includes new test, let's uplift this for the beta 6 build.
Attachment #8767305 - Flags: approval-mozilla-beta?
Attachment #8767305 - Flags: approval-mozilla-beta+
Attachment #8767305 - Flags: approval-mozilla-aurora?
Attachment #8767305 - Flags: approval-mozilla-aurora+
Attachment #8767303 - Flags: approval-mozilla-beta?
Attachment #8767303 - Flags: approval-mozilla-beta+
Attachment #8767303 - Flags: approval-mozilla-aurora?
Attachment #8767303 - Flags: approval-mozilla-aurora+
Comment on attachment 8767306 [details] [diff] [review]
P2 Add wpt test verifying serviceWorker.controller behavior on disconnected window. r=baku

Adds tests.
Attachment #8767306 - Flags: approval-mozilla-beta?
Attachment #8767306 - Flags: approval-mozilla-beta+
Attachment #8767306 - Flags: approval-mozilla-aurora?
Attachment #8767306 - Flags: approval-mozilla-aurora+
Track this as this is uplifted and is a crash.
does not apply cleanly to aurora, can you take a look ? Thanks!
Flags: needinfo?(bkelly)
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r e28b6e2e44d1
grafting 352565:e28b6e2e44d1 "Bug 1283674 P2 Add wpt test verifying serviceWorker.controller behavior on disconnected window. r=baku"
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue') is the error
Yea, the wpt MANIFEST.json is super annoying for these uplifts.  It conflicts on beta and aurora in different ways.  I'll land the patches shortly.
You need to log in before you can comment on or make changes to this bug.