Closed Bug 1379643 Opened 7 years ago Closed 7 years ago

Leak of 248 bytes during mochitest-a11y tests when current user not in restricting SIDs for Windows content sandbox.

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bobowen, Assigned: bugzilla)

References

Details

(Whiteboard: sbwc2)

Attachments

(1 file)

At sandboxing level 3+, preserve the serialized interface's IStream and release when the DocAccessibleParent is destroyed
11.00 KB, patch
jimm
: review+
Details | Diff | Splinter Review 
Bug 1366694 comment 7 tracked the trigger for this leak down to the removal of the user's own SID from the restricting SIDs in the access token for the content process sandbox policy.

I've managed to get the tests to run locally, although I have to skip a couple, but I haven't reproduced the leak.

I don't want this to delay getting this turned on for nightly, but I've set this to block the roll-out to release, at least for the moment.

Here's the log including the stack for the allocation of the leak:
== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 4980
     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       18      248|139167211        3|
  14 |Accessible                            |       96       96|   26936        1|
 583 |OuterDocAccessible                    |      144      144|      81        1|
1636 |nsTArray_base                         |        8        8|20250218        1|
nsTraceRefcnt::DumpStatistics: 1788 entries
Serial Numbers of Leaked Objects:
37 @000001E75771AB30 (1 references)
allocation stack:
#00: nsAccessibilityService::CreateAccessibleByFrameType(nsIFrame *,nsIContent *,mozilla::a11y::Accessible *) [accessible/base/nsAccessibilityService.cpp:1670]
#01: nsAccessibilityService::CreateAccessible(nsINode *,mozilla::a11y::Accessible *,bool *) [accessible/base/nsAccessibilityService.cpp:1095]
#02: mozilla::a11y::TreeWalker::AccessibleFor(nsIContent *,unsigned int,bool *) [accessible/base/TreeWalker.cpp:325]
#03: mozilla::a11y::TreeWalker::Scope(nsIContent *) [accessible/base/TreeWalker.cpp:81]
#04: InsertIterator::Next() [accessible/generic/DocAccessible.cpp:1844]
#05: mozilla::a11y::DocAccessible::ProcessContentInserted(mozilla::a11y::Accessible *,nsTArray<nsCOMPtr<nsIContent> > const *) [accessible/generic/DocAccessible.cpp:1871]
#06: mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) [accessible/base/NotificationController.cpp:728]
#07: nsRefreshDriver::Tick(__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:1856]
#08: mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver *,__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:329]
#09: mozilla::RefreshDriverTimer::TickRefreshDrivers(__int64,mozilla::TimeStamp,nsTArray<RefPtr<nsRefreshDriver> > &) [layout/base/nsRefreshDriver.cpp:300]
#10: mozilla::RefreshDriverTimer::Tick(__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:320]
#11: mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:762]
#12: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:677]
#13: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() [layout/base/nsRefreshDriver.cpp:521]
#14: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1438]
#15: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:489]
#16: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:97]
#17: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:314]
#18: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:294]
#19: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:158]
#20: nsAppShell::Run() [widget/windows/nsAppShell.cpp:273]
#21: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:287]
#22: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4589]
#23: XREMain::XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [toolkit/xre/nsAppRunner.cpp:4772]
#24: XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [toolkit/xre/nsAppRunner.cpp:4867]
#25: do_main [browser/app/nsBrowserApp.cpp:238]
#26: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:312]
#27: wmain [toolkit/xre/nsWindowsWMain.cpp:118]
#28: __scrt_common_main_seh [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253]
#29: KERNEL32.DLL + 0x12774
#30: ntdll.dll + 0x70d61
TEST-INFO | leakcheck | default process: leaked 1 Accessible
TEST-INFO | leakcheck | default process: leaked 1 OuterDocAccessible
TEST-INFO | leakcheck | default process: leaked 1 nsTArray_base
OK, with the browser mochitests failure (that I got backed-out for) I've managed to reproduce locally running accessible/tests/browser/bounds/browser_test_zoom.js

By logging out all the ref counts with sandbox level 3 and 2 it seems that with level 3 we are not getting releases in the parent with the stack below.

My guess is that this is from a windows message sent from the child that is now failing to be sent.

aklotz, eeejay - does this help either of you track this down?

<OuterDocAccessible> 000001993F17F280 3 Release 4 [thread 0000019929A29800]
    #01: SetRestrictedErrorInfo[C:\WINDOWS\System32\combase.dll +0x1dede]
    #02: DllGetClassObject[C:\WINDOWS\System32\combase.dll +0x9e669]
    #03: DllGetClassObject[C:\WINDOWS\System32\combase.dll +0xa30e6]
    #04: CoSetProxyBlanket[C:\WINDOWS\System32\combase.dll +0x2b6e2]
    #05: NdrInterfacePointerMemorySize[C:\WINDOWS\System32\RPCRT4.dll +0x788d3]
    #06: NdrClientCall3[C:\WINDOWS\System32\RPCRT4.dll +0xdc93e]
    #07: NdrStubCall3[C:\WINDOWS\System32\RPCRT4.dll +0x191a4]
    #08: CStdStubBuffer_Invoke[C:\WINDOWS\System32\combase.dll +0x4d99]
    #09: CoCreateInstance[C:\WINDOWS\System32\combase.dll +0xa795b]
    #10: CoCreateInstance[C:\WINDOWS\System32\combase.dll +0xa6286]
    #11: CoCreateInstance[C:\WINDOWS\System32\combase.dll +0xac75e]
    #12: CoCreateInstance[C:\WINDOWS\System32\combase.dll +0xa83ff]
    #13: DllGetClassObject[C:\WINDOWS\System32\combase.dll +0xa48aa]
    #14: Ordinal87[C:\WINDOWS\System32\combase.dll +0x43369]
    #15: Ordinal87[C:\WINDOWS\System32\combase.dll +0x42fe8]
    #16: CallWindowProcW[C:\WINDOWS\System32\USER32.dll +0xbc50]
    #17: DispatchMessageW[C:\WINDOWS\System32\USER32.dll +0xb5cf]
    #18: nsAppShell::ProcessNextNativeEvent (c:\dev\mozilla-central\widget\windows\nsappshell.cpp:394)
    #19: nsBaseAppShell::DoProcessNextNativeEvent (c:\dev\mozilla-central\widget\nsbaseappshell.cpp:139)
    #20: nsBaseAppShell::OnProcessNextEvent (c:\dev\mozilla-central\widget\nsbaseappshell.cpp:272)
    #21: nsThread::ProcessNextEvent (c:\dev\mozilla-central\xpcom\threads\nsthread.cpp:1365)
    #22: NS_ProcessNextEvent (c:\dev\mozilla-central\xpcom\threads\nsthreadutils.cpp:489)
    #23: mozilla::ipc::MessagePump::Run (c:\dev\mozilla-central\ipc\glue\messagepump.cpp:97)
    #24: MessageLoop::RunHandler (c:\dev\mozilla-central\ipc\chromium\src\base\message_loop.cc:314)
    #25: MessageLoop::Run (c:\dev\mozilla-central\ipc\chromium\src\base\message_loop.cc:294)
    #26: nsBaseAppShell::Run (c:\dev\mozilla-central\widget\nsbaseappshell.cpp:158)
    #27: nsAppShell::Run (c:\dev\mozilla-central\widget\windows\nsappshell.cpp:273)
    #28: nsAppStartup::Run (c:\dev\mozilla-central\toolkit\components\startup\nsappstartup.cpp:287)
Flags: needinfo?(eitan)
Flags: needinfo?(aklotz)
Some of the releases have slightly different stacks (below)

It appears that it is processing a 0x0400 message and the three releases occur during the one message dispatch.

<OuterDocAccessible> 000001993F17F280 3 Release 3 [thread 0000019929A29800]
    #01: CStdStubBuffer2_Disconnect[C:\WINDOWS\System32\combase.dll +0x4f22]
    #02: DllGetClassObject[C:\WINDOWS\System32\combase.dll +0x9e6cd]
    #03: DllGetClassObject[C:\WINDOWS\System32\combase.dll +0xa30e6]
    #04: CoSetProxyBlanket[C:\WINDOWS\System32\combase.dll +0x2b6e2]

<OuterDocAccessible> 000001993F17F280 3 Release 2 [thread 0000019929A29800]
    #01: CoIsHandlerConnected[C:\WINDOWS\System32\combase.dll +0xe2640]
    #02: SetRestrictedErrorInfo[C:\WINDOWS\System32\combase.dll +0x1df1e]
    #03: DllGetClassObject[C:\WINDOWS\System32\combase.dll +0xa31fd]
    #04: CoSetProxyBlanket[C:\WINDOWS\System32\combase.dll +0x2b6e2]
(In reply to Bob Owen (:bobowen) from comment #1)
> My guess is that this is from a windows message sent from the child that is
> now failing to be sent.

Ah, now everything falls into place.

Up until now, COM objects hosted in the parent process were marshaled directly from the main thread STA. This implies that any communication from content to those objects must be done via messaging.

Now that we no longer have that, I am going to need to switch this over to use the MTA like we do in content.
Flags: needinfo?(eitan)
Flags: needinfo?(aklotz)
Aaron, this is blocking level 3 sandbox so please make it a priority.
Flags: needinfo?(aklotz)
Blocks: 1366694
No longer depends on: 1366694
Yep, on it.
Flags: needinfo?(aklotz)
Assignee: nobody → aklotz
Whiteboard: sbwc2
My initial solution has some problems so I am exploring another route. I hope to have a patch soon but these issues are... multi-faceted. :-)
Status: NEW → ASSIGNED
At sandboxing level 3, content cannot send a message to the parent process's hidden COM window. This is a problem because content is therefore unable to release its reference to a document's parent IAccessible.

Since that parent IAccessible lives for the duration of the IPDL actor, the simplest workaround is to save a reference to the serialized interface's IStream and call CoReleaseMarshalData on it when the actor is destroyed. This effectively makes the parent release the accessible on content's behalf.
Attachment #8887274 - Flags: review?(jmathies)
Comment on attachment 8887274 [details] [diff] [review]
At sandboxing level 3+, preserve the serialized interface's IStream and release when the DocAccessibleParent is destroyed

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

::: ipc/mscom/COMPtrHolder.h
@@ +150,5 @@
> +         * behalf.
> +         */
> +        RefPtr<IStream> stream(proxyStream.GetStream());
> +
> +        // Rewind the stream

Why do this? a comment here might help.
Attachment #8887274 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1afed54d07d18524b35395d427cc840caf5019
Bug 1379643: When running under sandbox level >= 3, parent should retain IStream of marshaled interface to be destroyed later; r=jimm
https://hg.mozilla.org/mozilla-central/rev/cf1afed54d07
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: