Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 1329521 (CVE-2017-5468)

Private browsing flag assertion failures on debug builds in devtools / page info / browser console

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Page Info Window
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: Jerri Rice (rehash pending), Assigned: baku)

Tracking

(Blocks: 1 bug, {sec-low})

53 Branch
Firefox 53
sec-low
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox53 fixed)

Details

(Whiteboard: [OA][adv-main53+][post-critsmash-triage])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

6 months ago
(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)
Blocks: 1323262
Component: Untriaged → Page Info Window
Flags: needinfo?(jerri.rice.001)
Summary: Multiple crashes in the newest nightly build on windows 01-08-17 → Private browsing flag assertion failures on debug builds in devtools / page info / browser console

Comment 2

6 months ago
(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)
Flags: needinfo?(amarchesini)
(Reporter)

Comment 3

6 months ago
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).
Flags: needinfo?(jerri.rice.001)
(Assignee)

Comment 4

6 months ago
Yes, I can reproduce it.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
(Assignee)

Comment 5

6 months ago
Created attachment 8826202 [details] [diff] [review]
pb1.patch

When the docShell is type chrome, the check is wrong.
btw, My patch for removing pb from loadContext is not finished yet.
Attachment #8826202 - Flags: review?(bugs)
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.
Flags: needinfo?(amarchesini)
Keywords: sec-low
(Reporter)

Comment 7

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

Comment 8

6 months ago
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.
Flags: needinfo?(amarchesini)

Comment 9

6 months ago
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.
Attachment #8826202 - Flags: review?(bugs) → review-
(Assignee)

Comment 10

6 months ago
Created attachment 8826525 [details] [diff] [review]
pb2.patch
Attachment #8826202 - Attachment is obsolete: true
Attachment #8826525 - Flags: review?(bugs)

Comment 11

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

The assertion in nsGlobalWindow will be handled separately? 
It is still something to look at.
Attachment #8826525 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

6 months ago
Created attachment 8826539 [details] [diff] [review]
pb3.patch
Attachment #8826539 - Flags: review?(bugs)

Comment 13

6 months ago
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());
Attachment #8826539 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/88ca55621f27
https://hg.mozilla.org/mozilla-central/rev/f7e3cc798e5d
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Group: firefox-core-security → core-security-release
I understand why we have to check for typeChrome, but why do we switch from diagnostic asserts to ifdef debug?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 16

6 months ago
Also MOZ_ASSERT_IF(!mIsChrome, ...) is gone because of one the latest ehsan's patch restoring PrivateBrowsing in sessionStorage.
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(amarchesini)
Whiteboard: [OA]
(Assignee)

Comment 18

5 months ago
We discussed this issue during a meeting. I cancel my NI.
(Assignee)

Updated

5 months ago
Flags: needinfo?(amarchesini)
Whiteboard: [OA] → [OA][adv-main53+]
I assume this wasn't 53 only and 52 was affected? (Flags aren't set)
Flags: needinfo?(amarchesini)
(Assignee)

Updated

3 months ago
status-firefox52: --- → unaffected
Flags: needinfo?(amarchesini)
Flags: qe-verify-
Whiteboard: [OA][adv-main53+] → [OA][adv-main53+][post-critsmash-triage]
Group: core-security-release
Alias: CVE-2017-5468
You need to log in before you can comment on or make changes to this bug.