1329521 - (CVE-2017-5468) Private browsing flag assertion failures on debug builds in devtools / page info / browser console

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Jerri Rice (rehash pending)]

Ive encountered multiple crashes in the newest nightly build involving the chrome level dev tools(gloabl console) and viewing page info.

First the page info problem as it is the easiest to reproduce.  For me when viewing any page and trying to view page info, ff crashes with the following for a stack trace:

Assertion failure: pb == (doa.mPrivateBrowsingId > 0), at c:/Users/[redacted]/mozilla
-central/netwerk/base/LoadContextInfo.cpp:146
#01: mozilla::net::LoadContextInfoFactory::FromLoadContext (c:\users\[redacted]\mozil
la-central\netwerk\base\loadcontextinfo.cpp:100)
#02: _NS_InvokeByIndex (c:\Users\[redacted]\mozilla-central\xpcom\reflect\xptcall\md\
win32\xptcinvoke_asm_x86_msvc.asm:57)
#03: CallMethodHelper::Invoke (c:\users\[redacted]\mozilla-central\js\xpconnect\src\x
pcwrappednative.cpp:2058)
#04: CallMethodHelper::Call (c:\users\[redacted]\mozilla-central\js\xpconnect\src\xpc
wrappednative.cpp:1377)
#05: XPCWrappedNative::CallMethod (c:\users\[redacted]\mozilla-central\js\xpconnect\s
rc\xpcwrappednative.cpp:1344)
#06: XPC_WN_CallMethod (c:\users\[redacted]\mozilla-central\js\xpconnect\src\xpcwrapp
ednativejsops.cpp:999)
#07: js::CallJSNative (c:\users\[redacted]\mozilla-central\js\src\jscntxtinlines.h:23
9)
#08: js::InternalCallOrConstruct (c:\users\[redacted]\mozilla-central\js\src\vm\inter
preter.cpp:457)
#09: InternalCall (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:502)

#10: js::CallFromStack (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp
:508)
#11: Interpret (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:2928)
#12: js::RunScript (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:403
)
#13: js::ExecuteKernel (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp
:684)
#14: js::Execute (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:716)
#15: ExecuteScript (c:\users\[redacted]\mozilla-central\js\src\jsapi.cpp:4410)
#16: JS::CloneAndExecuteScript (c:\users\[redacted]\mozilla-central\js\src\jsapi.cpp:
4473)
#17: mozilla::dom::XULDocument::ExecuteScript (c:\users\[redacted]\mozilla-central\do
m\xul\xuldocument.cpp:3523)
#18: mozilla::dom::XULDocument::OnScriptCompileComplete (c:\users\[redacted]\mozilla-
central\dom\xul\xuldocument.cpp:3410)
#19: NotifyOffThreadScriptCompletedRunnable::Run (c:\users\[redacted]\mozilla-central
\dom\xul\nsxulelement.cpp:2813)
#20: nsThread::ProcessNextEvent (c:\users\[redacted]\mozilla-central\xpcom\threads\ns
thread.cpp:1240)
#21: NS_ProcessNextEvent (c:\users\[redacted]\mozilla-central\xpcom\glue\nsthreadutil
s.cpp:390)
#22: mozilla::ipc::MessagePump::Run (c:\users\[redacted]\mozilla-central\ipc\glue\mes
sagepump.cpp:124)
#23: MessageLoop::RunInternal (c:\users\[redacted]\mozilla-central\ipc\chromium\src\b
ase\message_loop.cc:239)
#24: MessageLoop::RunHandler (c:\users\[redacted]\mozilla-central\ipc\chromium\src\ba
se\message_loop.cc:232)
#25: MessageLoop::Run (c:\users\[redacted]\mozilla-central\ipc\chromium\src\base\mess
age_loop.cc:212)
#26: nsBaseAppShell::Run (c:\users\[redacted]\mozilla-central\widget\nsbaseappshell.c
pp:158)
#27: nsAppShell::Run (c:\users\[redacted]\mozilla-central\widget\windows\nsappshell.c
pp:262)
#28: nsAppStartup::Run (c:\users\[redacted]\mozilla-central\toolkit\components\startu
p\nsappstartup.cpp:283)
#29: XREMain::XRE_mainRun (c:\users\[redacted]\mozilla-central\toolkit\xre\nsapprunne
r.cpp:4494)
#30: XREMain::XRE_main (c:\users\[redacted]\mozilla-central\toolkit\xre\nsapprunner.c
pp:4623)
#31: XRE_main (c:\users\[redacted]\mozilla-central\toolkit\xre\nsapprunner.cpp:4714)
#32: do_main (c:\users\[redacted]\mozilla-central\browser\app\nsbrowserapp.cpp:319)
#33: NS_internal_main (c:\users\[redacted]\mozilla-central\browser\app\nsbrowserapp.c
pp:452)
#34: wmain (c:\users\[redacted]\mozilla-central\toolkit\xre\nswindowswmain.cpp:115)
#35: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.
inl:253)
#36: BaseThreadInitThunk[C:\Windows\syswow64\kernel32.dll +0x1336a]
#37: RtlInitializeExceptionChain[C:\Windows\SysWOW64\ntdll.dll +0x39902]
#38: RtlInitializeExceptionChain[C:\Windows\SysWOW64\ntdll.dll +0x398d5]

Next, with any page open launch the global console (ctrl+shift+j) with chrome debugging enabled.  Type this into the global console, then click the resulting chrome window to get a stack trace like the following:

[Parent 5556] WARNING: CacheStorage has been disabled.: file c:/Users/[redacted]/mozi
lla-central/dom/cache/CacheStorage.cpp, line 149
Assertion failure: (principal->GetPrivateBrowsingId() > 0) == IsPrivateBrowsing(
), at c:/Users/[redacted]/mozilla-central/dom/base/nsGlobalWindow.cpp:10680
#01: mozilla::dom::WindowBinding::get_sessionStorage (c:\users\[redacted]\mozilla-cen
tral\obj-i686-pc-mingw32\dom\bindings\windowbinding.cpp:15422)
#02: mozilla::dom::WindowBinding::genericGetter (c:\users\[redacted]\mozilla-central\
obj-i686-pc-mingw32\dom\bindings\windowbinding.cpp:15580)
#03: js::CallJSNative (c:\users\[redacted]\mozilla-central\js\src\jscntxtinlines.h:23
9)
#04: js::InternalCallOrConstruct (c:\users\[redacted]\mozilla-central\js\src\vm\inter
preter.cpp:457)
#05: InternalCall (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:502)

#06: js::Call (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:521)
#07: js::DebuggerObject::call (c:\users\[redacted]\mozilla-central\js\src\vm\debugger
.cpp:10239)
#08: js::DebuggerObject::callMethod (c:\users\[redacted]\mozilla-central\js\src\vm\de
bugger.cpp:9323)
#09: js::CallJSNative (c:\users\[redacted]\mozilla-central\js\src\jscntxtinlines.h:23
9)
#10: js::InternalCallOrConstruct (c:\users\[redacted]\mozilla-central\js\src\vm\inter
preter.cpp:457)
#11: InternalCall (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:502)

#12: js::Call (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:521)
#13: js::Wrapper::call (c:\users\[redacted]\mozilla-central\js\src\proxy\wrapper.cpp:
165)
#14: js::CrossCompartmentWrapper::call (c:\users\[redacted]\mozilla-central\js\src\pr
oxy\crosscompartmentwrapper.cpp:333)
#15: js::Proxy::call (c:\users\[redacted]\mozilla-central\js\src\proxy\proxy.cpp:421)

#16: js::proxy_Call (c:\users\[redacted]\mozilla-central\js\src\proxy\proxy.cpp:662)
#17: js::CallJSNative (c:\users\[redacted]\mozilla-central\js\src\jscntxtinlines.h:23
9)
#18: js::InternalCallOrConstruct (c:\users\[redacted]\mozilla-central\js\src\vm\inter
preter.cpp:445)
#19: InternalCall (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:502)

#20: js::CallFromStack (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp
:508)
#21: js::jit::DoCallFallback (c:\users\[redacted]\mozilla-central\js\src\jit\baseline
ic.cpp:4396)
#22: ??? (???:???)
#23: ??? (???:???)
#24: ??? (???:???)
#25: EnterBaseline (c:\users\[redacted]\mozilla-central\js\src\jit\baselinejit.cpp:15
5)
#26: js::jit::EnterBaselineMethod (c:\users\[redacted]\mozilla-central\js\src\jit\bas
elinejit.cpp:195)
#27: js::RunScript (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:393
)
#28: js::InternalCallOrConstruct (c:\users\[redacted]\mozilla-central\js\src\vm\inter
preter.cpp:475)
#29: InternalCall (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp:502)

#30: js::CallFromStack (c:\users\[redacted]\mozilla-central\js\src\vm\interpreter.cpp
:508)
#31: js::jit::DoCallFallback (c:\users\[redacted]\mozilla-central\js\src\jit\baseline
ic.cpp:4396)
#32: ??? (???:???)
#33: ??? (???:???)
#34: ??? (???:???)
[GPU 9044] WARNING: Shutting down GPU process early due to a crash!: file c:/Use
rs/[redacted]/mozilla-central/gfx/ipc/GPUParent.cpp, line 370

Making these private until someone can look at them because I haven't even begun to dig into debugging properly(have some slight issues with visual studio ugh)

I'll try to bring some follow up work with this soon.

Comment 1

[:Gijs (he/him)]

(In reply to Jerri Rice (rehash pending) from comment #0)
> Next, with any page open launch the global console (ctrl+shift+j) with
> chrome debugging enabled.  Type this

Type what?

> into the global console, then click the
> resulting chrome window to get a stack trace like the following:

> <snip stack>



:baku, are you aware of the page info thing and/or can you repro? (I don't have a debug build handy right now)

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs from comment #1)
> (In reply to Jerri Rice (rehash pending) from comment #0)
> First the page info problem as it is the easiest to reproduce.
> For me when viewing any page and trying to view page info, ff
> crashes with the following for a stack trace:
> :baku, are you aware of the page info thing and/or can you repro? (I don't
> have a debug build handy right now)

Comment 3

[Jerri Rice (rehash pending)]

Well, I should have used quotes, type "this" and press enter.  The global console will show the global chrome window, which if you click on it triggers the same assertion with a different stack trace(some worrisome JIT code with the global console).

Comment 4

[Andrea Marchesini [:baku]]

Yes, I can reproduce it.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: pb1.patch

When the docShell is type chrome, the check is wrong.
btw, My patch for removing pb from loadContext is not finished yet.

Comment 6

[Daniel Veditz [:dveditz]]

Is this basically part of the stuff Huseby worried about in bug 1323262? Also would Ehsan's patch in bug 1319908 have helped? (seems kind of similar).

The crashes may show memory issues, but it's hard to build an exploit around someone manually running a debugger.

Comment 7

[Jerri Rice (rehash pending)]

There's not even a need for the sec low here I just didn't have time to dig into debugging it.

If more eye's on it helps make it public.  I didn't expect this to be exploitable, just helping the devs with the nightly stuff when things don't pan out.  Err on the side of caution though ya know.

Comment 8

[Andrea Marchesini [:baku]]

This is exactly the same issue. I'm working on get rid of privateBrowsing boolean in nsILoadContext and clean up how privateBrowsing is handled by docShell.

The issue here is that we have several assertions that compare privateBrowsing boolean and what we have in docShell originAttributes. This works fine for content docShell, but for chrome docShell, privateBrowsing boolean is set to true, but docShell originAttributes.mPrivateBrowsingId is 0. This because originAttributes is taken from the principal, and from chrome docShells we use SystemPrincipal.

We need to clean up the 'ownership' model of this privateBrowsing information in docShell. We agree, during the work-week in hawaii, on how this issue should be handled. I have a patch, but it's not finished yet. Probably it will be ready in next week.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8826202 [details] [diff] [review]
pb1.patch

I would very much not like to remove any of these assertions (at least not before pb flag handling is removed from nsILoadContext), but make them more exact.
So, couldn't you QI aLoadContext to nsIDocShell in
GetLoadContextInfo(nsILoadContext *aLoadContext, bool aIsAnonymous)
and then assert that either it is chrome docshell or 
pb == (oa.mPrivateBrowsingId > 0)

Why do we need to remove the assertion from 
 GetLoadContextInfo(nsIChannel * aChannel)? The assertion crash is in the other GetLoadContextInfo 

So this is not addressing the other stack trace, right. The one about
nsGlobalWindow.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: pb2.patch

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8826525 [details] [diff] [review]
pb2.patch

The assertion in nsGlobalWindow will be handled separately? 
It is still something to look at.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: pb3.patch

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8826539 [details] [diff] [review]
pb3.patch

Because of how NS_NewScriptGlobalObject is called, you should be able to just
use mIsChrome here and no need to access docshell.
So something like
MOZ_ASSERT_IF(!mIsChrome, (principal->GetPrivateBrowsingId() > 0) == IsPrivateBrowsing());

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88ca55621f27
https://hg.mozilla.org/mozilla-central/rev/f7e3cc798e5d

Comment 15

[Tanvi Vyas[:tanvi]]

I understand why we have to check for typeChrome, but why do we switch from diagnostic asserts to ifdef debug?

Comment 16

[Andrea Marchesini [:baku]]

Also MOZ_ASSERT_IF(!mIsChrome, ...) is gone because of one the latest ehsan's patch restoring PrivateBrowsing in sessionStorage.

Comment 17

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini [:baku] from comment #16)
> Also MOZ_ASSERT_IF(!mIsChrome, ...) is gone because of one the latest
> ehsan's patch restoring PrivateBrowsing in sessionStorage.

I don't follow.

Comment 18

[Andrea Marchesini [:baku]]

We discussed this issue during a meeting. I cancel my NI.

Comment 19

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

I assume this wasn't 53 only and 52 was affected? (Flags aren't set)