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

RESOLVED FIXED in Firefox 56

Status

()

Core
Security: Process Sandboxing
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: bobowen, Assigned: aklotz)

Tracking

Trunk
mozilla56
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: sbwc2)

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
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
(Reporter)

Comment 1

7 months ago
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)
(Reporter)

Comment 2

7 months ago
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]
(Assignee)

Comment 3

7 months ago
(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)

Comment 4

7 months ago
Aaron, this is blocking level 3 sandbox so please make it a priority.
Flags: needinfo?(aklotz)

Updated

7 months ago
Blocks: 1366694
No longer depends on: 1366694
(Assignee)

Comment 5

7 months ago
Yep, on it.
Flags: needinfo?(aklotz)

Updated

6 months ago
Assignee: nobody → aklotz
Whiteboard: sbwc2
(Assignee)

Comment 6

6 months ago
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
(Assignee)

Comment 8

6 months ago
Created attachment 8887274 [details] [diff] [review]
At sandboxing level 3+, preserve the serialized interface's IStream and release when the DocAccessibleParent is destroyed

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 9

6 months ago
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+
(Assignee)

Comment 10

6 months ago
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

Comment 11

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf1afed54d07
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months 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.