Opened 13 months ago

Closed 13 months ago

Last modified 11 months ago

#22327 closed defect (fixed)

First party isolation of Page Info

Reported by: arthuredelstein Owned by: arthuredelstein
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-7.0-must tbb-linkability TorBrowserTeam201705
Cc: mcs, brade, tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

First party isolation is not properly enforce in the Page Info window in ESR52. First reported in https://trac.torproject.org/projects/tor/ticket/21762?replyto=4#comment:4

Child Tickets

Change History (17)

comment:1 Changed 13 months ago by arthuredelstein

Keywords: TorBrowserTeam201705R added; TorBrowserTeam201705 removed
Status: newneeds_review

comment:2 Changed 13 months ago by gk

Status: needs_reviewneeds_information

Hm, looking at that branch it seems we need 32287fdd336a9ed2bbd52e71595fcb7e61394dc5 as well? Or am I missing something?

comment:3 in reply to:  2 ; Changed 13 months ago by gk

Replying to gk:

Hm, looking at that branch it seems we need 32287fdd336a9ed2bbd52e71595fcb7e61394dc5 as well? Or am I missing something?

It seems you are missing the

+#include "nsSerializationHelper.h"

in that backported patch? I somehow doubt it would compile without it looking at the code.

comment:4 in reply to:  3 Changed 13 months ago by arthuredelstein

Replying to gk:

Replying to gk:

Hm, looking at that branch it seems we need 32287fdd336a9ed2bbd52e71595fcb7e61394dc5 as well? Or am I missing something?

Yes, sorry, I should have also mentioned that I backported that patch.

It seems you are missing the

+#include "nsSerializationHelper.h"

in that backported patch? I somehow doubt it would compile without it looking at the code.

You're right. The branch builds, but the #include line somehow migrated from Ehsan's patch onto my patch. I have put it back into his patch and I have rebased the two patches onto the latest tor-browser-52.1.1esr-7.0-1 branch. Here is the new result:
https://github.com/arthuredelstein/tor-browser/commits/22327+1

I rebuilt this new branch and ran the following manual test:

  1. add torbutton
  2. set extensions.torbutton.loglevel to 3
  3. open the Browser Console
  4. visit http://html5demo.com/video
  5. open Page Info
  6. observe "tor SOCKS" messages in the Browser Console

When I ran this test, videos and images in the "media preview" box were correctly isolated to the content document's first party.

comment:5 Changed 13 months ago by gk

Status: needs_informationneeds_review

comment:6 Changed 13 months ago by gk

Cc: mcs brade added

comment:7 Changed 13 months ago by mcs

Kathy and I reviewed the changes and they look okay to us. But since we are not very familiar with the new way of doing isolation, we applied the patches and tried to run the revised code. Unfortunately, at least with our OSX debug build, invoking page info (Cmd+I) always causes an assertion failure:

Assertion failure: pb == (doa.mPrivateBrowsingId > 0), at /.../tor-browser/netwerk/base/LoadContextInfo.cpp:147

To make things more challenging, my lldb seems to have forgotten how to use symbols. I am not sure if we should be concerned about the assertion failure or not, but we will try again with an optimized build.

comment:8 Changed 13 months ago by gk

Priority: MediumHigh

comment:9 Changed 13 months ago by gk

Cc: tbb-team added
Owner: changed from tbb-team to arthuredelstein
Status: needs_reviewassigned

comment:10 Changed 13 months ago by gk

Status: assignedneeds_review

comment:11 Changed 13 months ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201705R removed
Status: needs_reviewneeds_revision

Here is the stacktrace in case it helps:

Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
0x00007fffecc13b77 in mozilla::net::GetLoadContextInfo (aLoadContext=
    0x7fffbd7e3c00, aIsAnonymous=false)
    at /home/thomas/Arbeit/Tor/tor-browser/netwerk/base/LoadContextInfo.cpp:147
147	  MOZ_ASSERT(pb == (doa.mPrivateBrowsingId > 0));
(gdb bt
#0  0x00007fffecc13b77 in mozilla::net::GetLoadContextInfo (
    aLoadContext=0x7fffbd7e3c00, aIsAnonymous=false)
    at /home/thomas/Arbeit/Tor/tor-browser/netwerk/base/LoadContextInfo.cpp:147
#1  0x00007fffecc137fe in mozilla::net::LoadContextInfoFactory::FromLoadContext (
    this=0x7fffc938e500, aLoadContext=0x7fffbd7e3c00, aAnonymous=false, 
    _retval=0x7fffffffa988)
    at /home/thomas/Arbeit/Tor/tor-browser/netwerk/base/LoadContextInfo.cpp:100
#2  0x00007fffecb72740 in NS_InvokeByIndex (that=0x7fffc938e500, methodIndex=7, 
    paramCount=3, params=0x7fffffffa958)
    at /home/thomas/Arbeit/Tor/tor-browser/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:182
#3  0x00007fffed9b6fd9 in CallMethodHelper::Invoke (this=0x7fffffffa910)
    at /home/thomas/Arbeit/Tor/tor-browser/js/xpconnect/src/XPCWrappedNative.cpp:2058
#4  0x00007fffed9b4c0e in CallMethodHelper::Call (this=0x7fffffffa910)
    at /home/thomas/Arbeit/Tor/tor-browser/js/xpconnect/src/XPCWrappedNative.cpp:1377
#5  0x00007fffed999d11 in XPCWrappedNative::CallMethod (ccx=..., 
    mode=XPCWrappedNative::CALL_METHOD)
    at /home/thomas/Arbeit/Tor/tor-browser/js/xpconnect/src/XPCWrappedNative.cpp:1344
#6  0x00007fffed9a2884 in XPC_WN_CallMethod (cx=0x7fffdff65000, argc=2, 
    vp=0x7fffd9d2d098)
    at /home/thomas/Arbeit/Tor/tor-browser/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:999
#7  0x00007ffff2404ac6 in js::CallJSNative (cx=0x7fffdff65000, 
    native=0x7fffed9a257b <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/thomas/Arbeit/Tor/tor-browser/js/src/jscntxtinlines.h:239
#8  0x00007ffff23d0a3e in js::InternalCallOrConstruct (cx=0x7fffdff65000, 
    args=..., construct=js::NO_CONSTRUCT)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:459
#9  0x00007ffff23d0d82 in InternalCall (cx=0x7fffdff65000, args=...)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:504
#10 0x00007ffff23d0dac in js::CallFromStack (cx=0x7fffdff65000, args=...)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:510
#11 0x00007ffff23de3f9 in Interpret (cx=0x7fffdff65000, state=...)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:2922
#12 0x00007ffff23d068c in js::RunScript (cx=0x7fffdff65000, state=...)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:405
#13 0x00007ffff23d1b66 in js::ExecuteKernel (cx=0x7fffdff65000, script=..., 
    envChainArg=..., newTargetValue=..., evalInFrame=..., result=0x7fffffffc350)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:686
#14 0x00007ffff23d1e56 in js::Execute (cx=0x7fffdff65000, script=..., 
    envChainArg=..., rval=0x7fffffffc350)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/vm/Interpreter.cpp:719
#15 0x00007ffff213d788 in ExecuteScript (cx=0x7fffdff65000, scope=..., script=..., rval=0x7fffffffc350)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/jsapi.cpp:4350
#16 0x00007ffff213dd65 in JS::CloneAndExecuteScript (cx=0x7fffdff65000, 
    scriptArg=..., rval=...)
    at /home/thomas/Arbeit/Tor/tor-browser/js/src/jsapi.cpp:4413
#17 0x00007fffefdf03cc in mozilla::dom::XULDocument::ExecuteScript (
    this=0x7fffc413b800, aScript=0x7fffc8cfe290)
    at /home/thomas/Arbeit/Tor/tor-browser/dom/xul/XULDocument.cpp:3523
#18 0x00007fffefdefcbb in mozilla::dom::XULDocument::OnScriptCompileComplete (
    this=0x7fffc413b800, aScript=0x7fffc3b2d120, aStatus=nsresult::NS_OK)
    at /home/thomas/Arbeit/Tor/tor-browser/dom/xul/XULDocument.cpp:3410
#19 0x00007fffefe05212 in NotifyOffThreadScriptCompletedRunnable::Run (
    this=0x7fffbd999f40)
    at /home/thomas/Arbeit/Tor/tor-browser/dom/xul/nsXULElement.cpp:2797
#20 0x00007fffecb4432b in nsThread::ProcessNextEvent (this=0x7ffff6823600, 
    aMayWait=false, aResult=0x7fffffffc79f)
    at /home/thomas/Arbeit/Tor/tor-browser/xpcom/threads/nsThread.cpp:1216
#21 0x00007fffecbb1089 in NS_ProcessNextEvent (aThread=0x7ffff6823600, 
    aMayWait=false)
    at /home/thomas/Arbeit/Tor/tor-browser/xpcom/glue/nsThreadUtils.cpp:361
#22 0x00007fffed241843 in mozilla::ipc::MessagePump::Run (this=0x7ffff6958580, 
    aDelegate=0x7ffff6845c00)
    at /home/thomas/Arbeit/Tor/tor-browser/ipc/glue/MessagePump.cpp:96
#23 0x00007fffed1b1bfb in MessageLoop::RunInternal (this=0x7ffff6845c00)
    at /home/thomas/Arbeit/Tor/tor-browser/ipc/chromium/src/base/message_loop.cc:232
#24 0x00007fffed1b1b8e in MessageLoop::RunHandler (this=0x7ffff6845c00)
    at /home/thomas/Arbeit/Tor/tor-browser/ipc/chromium/src/base/message_loop.cc:225
#25 0x00007fffed1b1b67 in MessageLoop::Run (this=0x7ffff6845c00)
    at /home/thomas/Arbeit/Tor/tor-browser/ipc/chromium/src/base/message_loop.cc:205
#26 0x00007fffeff6494c in nsBaseAppShell::Run (this=0x7fffd989e5a0)
    at /home/thomas/Arbeit/Tor/tor-browser/widget/nsBaseAppShell.cpp:156
#27 0x00007ffff0c44175 in nsAppStartup::Run (this=0x7fffd986b450)
    at /home/thomas/Arbeit/Tor/tor-browser/toolkit/components/startup/nsAppStartup.cpp:283
#28 0x00007ffff0d1c50d in XREMain::XRE_mainRun (this=0x7fffffffcc60)
    at /home/thomas/Arbeit/Tor/tor-browser/toolkit/xre/nsAppRunner.cpp:5028
#29 0x00007ffff0d1cabd in XREMain::XRE_main (this=0x7fffffffcc60, argc=3, 
    argv=0x7fffffffe0a8, aAppData=0x7fffffffde80)
    at /home/thomas/Arbeit/Tor/tor-browser/toolkit/xre/nsAppRunner.cpp:5161
#30 0x00007ffff0d1cd36 in XRE_main (argc=3, argv=0x7fffffffe0a8, 
    aAppData=0x7fffffffde80, aFlags=0)
    at /home/thomas/Arbeit/Tor/tor-browser/toolkit/xre/nsAppRunner.cpp:5252
#31 0x000055555555c0a0 in do_main (argc=3, argv=0x7fffffffe0a8, 
    envp=0x7fffffffe0c8, xreDirectory=0x7ffff685ac00)
    at /home/thomas/Arbeit/Tor/tor-browser/browser/app/nsBrowserApp.cpp:282
#32 0x000055555555c386 in main (argc=3, argv=0x7fffffffe0a8, envp=0x7fffffffe0c8)
    at /home/thomas/Arbeit/Tor/tor-browser/browser/app/nsBrowserApp.cpp:415

FWIW it happens with an optimized build as well.

comment:12 in reply to:  11 ; Changed 13 months ago by arthuredelstein

Replying to gk:

Here is the stacktrace in case it helps:

Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
0x00007fffecc13b77 in mozilla::net::GetLoadContextInfo (aLoadContext=
    0x7fffbd7e3c00, aIsAnonymous=false)
[...]

FWIW it happens with an optimized build as well.

It turns out this assertion failure happens in a debug build when you open "Page Info" even without the patches in comment:4. I'm currently trying to track down the cause of the assertion by binary search. But my feeling is that the fix for this ticket shouldn't be delayed for what appears to be an unrelated issue.

comment:13 Changed 13 months ago by arthuredelstein

Status: needs_revisionneeds_review

I opened a ticket for the assert: #22462

comment:14 in reply to:  12 Changed 13 months ago by gk

Replying to arthuredelstein:

Replying to gk:

Here is the stacktrace in case it helps:

Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
0x00007fffecc13b77 in mozilla::net::GetLoadContextInfo (aLoadContext=
    0x7fffbd7e3c00, aIsAnonymous=false)
[...]

FWIW it happens with an optimized build as well.

It turns out this assertion failure happens in a debug build when you open "Page Info" even without the patches in comment:4. I'm currently trying to track down the cause of the assertion by binary search. But my feeling is that the fix for this ticket shouldn't be delayed for what appears to be an unrelated issue.

Good catch, I agree.

comment:15 Changed 13 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, I looked over it. Both the backport and the patch on top of it look good to me but I have to echo what mcs said that I am not very familiar with the new way of isolation either. That said, I compiled and tested the code and it works fine for me. Let it get into our next nightly build.

Applied to tor-browser-52.1.0esr-7.0-2 (commits ef0f9d150d4210bed5b517763cf8872e7f729649 and af4e5af7cfa3cfd73ff53fd6ec1be4e97e43f630) and tor-browser-51.1.1esr-7.0-1 (commit 24862c15f6f719cd05a42d784ed7a7b7bb05c567 and 2d4d11e1e6bad5f384c409fa0391e97a9c202439).

Thanks!

comment:16 Changed 12 months ago by arthuredelstein

Keywords: tbb-first-party added

comment:17 Changed 11 months ago by gk

Keywords: tbb-linkability added; tbb-first-party removed
Note: See TracTickets for help on using tickets.