674651 - nsContentPolicy should skip resource and chrome schemes

Status: RESOLVED WONTFIX

(Core :: DOM: Navigation, defect)

Description

[Benjamin Stover (:stechz)]

I have been looking for sqlite accesses during startup on mobile, and I noticed:
* sqlite was being initialized by storage service
* storage service was initialized by permission manager
* permission manager was being initialized by nsContentBlocker
* nsContentBlocker was being initialized by nsCategoryObserver
* nsCategoryObserver is being initialized because we are checking for a policy on a chrome page

We could prevent database reads before paint just by checking the scheme of the URI we are checking.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Stacktrace showing initialization of storage service

Comment 2

[Benjamin Stover (:stechz)]

Attachment: nsContentPolicy should skip resource and chrome schemes

Comment 3

[Boris Zbarsky [:bzbarsky]]

That opens a security hole, no?  In particular, it allows things that should have all loads blocked to load chrome: and resource: URIs.

(Ignoring for the moment that generally any time you have an explicit scheme list chances are the code is wrong.)

In this particular case, what was the requesting principal?

Comment 4

[Benjamin Stover (:stechz)]

(In reply to comment #3)
> That opens a security hole, no?  In particular, it allows things that should
> have all loads blocked to load chrome: and resource: URIs.

I'm confused, but I don't really understand this code. If the thing should be blocked, how could it load anything else in the first place?

> In this particular case, what was the requesting principal?

What I really want is for the DB to not be initialized for implicitly trusted content (maybe skip the check if the docshell we are loading is a chrome docshell).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> If the thing should be blocked, how could it load anything else in the first
> place?

"The thing" is the load originator.  You're checking the load target.  In particular, you're allowing loads of "chrome" and "resource" targets to go through no matter who the load originator is.

> What I really want is for the DB to not be initialized for implicitly trusted
> content 

Yes, but you want the load _originator_ to be trusted, not the load _target_.

And we already have a check for that.  See http://hg.mozilla.org/mozilla-central/annotate/982a5835fba1/content/base/public/nsContentPolicyUtils.h#l189

Hence my question about what the principal is in this case.

Comment 6

[Benjamin Stover (:stechz)]

> "The thing" is the load originator.  You're checking the load target.  In
> particular, you're allowing loads of "chrome" and "resource" targets to go
> through no matter who the load originator is.

Ah. I see.

> Yes, but you want the load _originator_ to be trusted, not the load _target_.

> And we already have a check for that.  See
> http://hg.mozilla.org/mozilla-central/annotate/982a5835fba1/content/base/
> public/nsContentPolicyUtils.h#l189
> 
> Hence my question about what the principal is in this case.

The principal is null.

Comment 7

[Benjamin Stover (:stechz)]

If it is null, can we also short-circuit here?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Null as in the null pointer, or null as in an nsNullPrincipal?

Can you attach a callstack to the relevant ShouldLoad call, please?

Comment 9

[Benjamin Stover (:stechz)]

It was a NULL pointer. It's the very first window loaded when Fennec starts up.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> It was a NULL pointer. 

OK.  What's that callstack, then?  I'd really like to fix the caller to pass in a useful principal if possible....

In particular, is this perhaps the ua stylesheet load?

Comment 11

[Benjamin Stover (:stechz)]

#0  sqlite3_initialize () at /home/ben/projects/mc/db/sqlite3/src/sqlite3.c:105131
#1  0xb6d06a26 in mozilla::storage::Service::initialize (this=0xaffe9f70) at /home/ben/projects/mc/storage/src/mozStorageService.cpp:328
#2  0xb6d06350 in mozilla::storage::Service::getSingleton () at /home/ben/projects/mc/storage/src/mozStorageService.cpp:244
#3  0xb6d042a2 in ServiceConstructor (aOuter=0x0, aIID=..., aResult=0xbfffd924)
    at /home/ben/projects/mc/storage/build/mozStorageModule.cpp:53
#4  0xb70af9b6 in mozilla::GenericFactory::CreateInstance (this=0xaffca230, aOuter=0x0, aIID=..., aResult=0xbfffd924)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/GenericFactory.cpp:48
#5  0xb7107e75 in nsComponentManagerImpl::CreateInstanceByContractID (this=0xb43bf220, 
    aContractID=0xb7bb6218 "@mozilla.org/storage/service;1", aDelegate=0x0, aIID=..., aResult=0xbfffd924)
    at /home/ben/projects/mc/xpcom/components/nsComponentManager.cpp:1296
#6  0xb7108c3e in nsComponentManagerImpl::GetServiceByContractID (this=0xb43bf220, 
    aContractID=0xb7bb6218 "@mozilla.org/storage/service;1", aIID=..., result=0xbfffd9ec)
    at /home/ben/projects/mc/xpcom/components/nsComponentManager.cpp:1698
#7  0xb70a388c in CallGetService (aContractID=0xb7bb6218 "@mozilla.org/storage/service;1", aIID=..., aResult=0xbfffd9ec)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsComponentManagerUtils.cpp:94
#8  0xb70a3d7a in nsGetServiceByContractID::operator() (this=0xbfffda04, aIID=..., aInstancePtr=0xbfffd9ec)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsComponentManagerUtils.cpp:278
#9  0xb6ba7030 in nsCOMPtr<mozIStorageService>::assign_from_gs_contractid (this=0xbfffdb88, gs=..., aIID=...)
    at ../../../dist/include/nsCOMPtr.h:1252
#10 0xb6ba67b1 in nsCOMPtr (this=0xbfffdb88, gs=...) at ../../../dist/include/nsCOMPtr.h:627
#11 0xb6cc05a3 in nsPermissionManager::InitDB (this=0xaffc8b80, aRemoveFile=0)
    at /home/ben/projects/mc/extensions/cookie/nsPermissionManager.cpp:274
#12 0xb6cc02b6 in nsPermissionManager::Init (this=0xaffc8b80) at /home/ben/projects/mc/extensions/cookie/nsPermissionManager.cpp:248
#13 0xb6cc0072 in nsPermissionManager::GetSingleton () at /home/ben/projects/mc/extensions/cookie/nsPermissionManager.cpp:207
#14 0xb6cbffb1 in nsPermissionManager::GetXPCOMSingleton () at /home/ben/projects/mc/extensions/cookie/nsPermissionManager.cpp:186
#15 0xb6cbeacf in nsIPermissionManagerConstructor (aOuter=0x0, aIID=..., aResult=0xbfffdd94)
    at /home/ben/projects/mc/extensions/cookie/nsCookieModule.cpp:49
#16 0xb70af9b6 in mozilla::GenericFactory::CreateInstance (this=0xaffca1b0, aOuter=0x0, aIID=..., aResult=0xbfffdd94)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/GenericFactory.cpp:48
#17 0xb7107e75 in nsComponentManagerImpl::CreateInstanceByContractID (this=0xb43bf220, 
    aContractID=0xb7bb79bc "@mozilla.org/permissionmanager;1", aDelegate=0x0, aIID=..., aResult=0xbfffdd94)
    at /home/ben/projects/mc/xpcom/components/nsComponentManager.cpp:1296
#18 0xb7108c3e in nsComponentManagerImpl::GetServiceByContractID (this=0xb43bf220, 
    aContractID=0xb7bb79bc "@mozilla.org/permissionmanager;1", aIID=..., result=0xbfffde5c)
    at /home/ben/projects/mc/xpcom/components/nsComponentManager.cpp:1698
#19 0xb70a388c in CallGetService (aContractID=0xb7bb79bc "@mozilla.org/permissionmanager;1", aIID=..., aResult=0xbfffde5c)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsComponentManagerUtils.cpp:94
#20 0xb70a3dc0 in nsGetServiceByContractIDWithError::operator() (this=0xbfffdec8, aIID=..., aInstancePtr=0xbfffde5c)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsComponentManagerUtils.cpp:288
#21 0xb6bbd762 in nsCOMPtr<nsIPermissionManager>::assign_from_gs_contractid_with_error (this=0xaffe9ef8, gs=..., aIID=...)
    at ../../../../dist/include/nsCOMPtr.h:1262
#22 0xb6bbd3fc in nsCOMPtr<nsIPermissionManager>::operator= (this=0xaffe9ef8, rhs=...) at ../../../../dist/include/nsCOMPtr.h:721
#23 0xb6cc87ed in nsContentBlocker::Init (this=0xaffe9ee0) at /home/ben/projects/mc/extensions/permissions/nsContentBlocker.cpp:85
#24 0xb6cc8265 in nsContentBlockerConstructor (aOuter=0x0, aIID=..., aResult=0xbfffe004)
    at /home/ben/projects/mc/extensions/permissions/nsModuleFactory.cpp:43
#25 0xb70af9b6 in mozilla::GenericFactory::CreateInstance (this=0xaffca210, aOuter=0x0, aIID=..., aResult=0xbfffe004)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/GenericFactory.cpp:48
#26 0xb7107e75 in nsComponentManagerImpl::CreateInstanceByContractID (this=0xb43bf220, 
    aContractID=0xaffcb348 "@mozilla.org/permissions/contentblocker;1", aDelegate=0x0, aIID=..., aResult=0xbfffe004)
    at /home/ben/projects/mc/xpcom/components/nsComponentManager.cpp:1296
#27 0xb7108c3e in nsComponentManagerImpl::GetServiceByContractID (this=0xb43bf220, 
    aContractID=0xaffcb348 "@mozilla.org/permissions/contentblocker;1", aIID=..., result=0xbfffe0cc)
    at /home/ben/projects/mc/xpcom/components/nsComponentManager.cpp:1698
#28 0xb70a388c in CallGetService (aContractID=0xaffcb348 "@mozilla.org/permissions/contentblocker;1", aIID=..., aResult=0xbfffe0cc)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsComponentManagerUtils.cpp:94
#29 0xb70a3d7a in nsGetServiceByContractID::operator() (this=0xbfffe0e4, aIID=..., aInstancePtr=0xbfffe0cc)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsComponentManagerUtils.cpp:278
#30 0xb5cea6ca in nsCOMPtr<nsIContentPolicy>::assign_from_gs_contractid (this=0xbfffe11c, gs=..., aIID=...)
---Type <return> to continue, or q <return> to quit---
    at ../../../dist/include/nsCOMPtr.h:1252
#31 0xb5ce9c47 in nsCOMPtr (this=0xbfffe11c, gs=...) at ../../../dist/include/nsCOMPtr.h:627
#32 0xb60e3501 in nsCategoryCache<nsIContentPolicy>::EntryAdded (this=0xaffe9dfc, aValue=...)
    at ../../../dist/include/nsCategoryCache.h:130
#33 0xb70a1bfd in nsCategoryObserver (this=0xb09fffc0, aCategory=0xaffc97c8 "content-policy", aListener=0xaffe9dfc)
    at /home/ben/projects/mc/obj-mobile/xpcom/build/nsCategoryCache.cpp:100
#34 0xb60e3436 in nsCategoryCache<nsIContentPolicy>::GetEntries (this=0xaffe9dfc) at ../../../dist/include/nsCategoryCache.h:101
#35 0xb60e2d51 in nsContentPolicy::CheckPolicy (this=0xaffe9df0, 
    policyMethod=&virtual nsIContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, short*), contentType=6, contentLocation=0xaff06780, requestingLocation=0x0, requestingContext=0x0, mimeType=..., extra=0x0, 
    decision=0xbfffe6ba) at /home/ben/projects/mc/content/base/src/nsContentPolicy.cpp:151
#36 0xb60e2f63 in nsContentPolicy::ShouldLoad (this=0xaffe9df0, contentType=6, contentLocation=0xaff06780, requestingLocation=0x0, 
    requestingContext=0x0, mimeType=..., extra=0x0, decision=0xbfffe6ba)
    at /home/ben/projects/mc/content/base/src/nsContentPolicy.cpp:218
#37 0xb5ce508a in NS_CheckContentLoadPolicy (contentType=6, contentLocation=0xaff06780, originPrincipal=0x0, context=0x0, mimeType=..., 
    extra=0x0, decision=0xbfffe6ba, policyService=0x0, aSecMan=0x0) at ../../../dist/include/nsContentPolicyUtils.h:223
#38 0xb6abd502 in nsDocShell::InternalLoad (this=0xb430f200, aURI=0xaff06780, aReferrer=0x0, aOwner=0x0, aFlags=1, 
    aWindowTarget=0xb43201c0, aTypeHint=0x0, aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=1, aDocShell=0x0, 
    aRequest=0x0) at /home/ben/projects/mc/docshell/base/nsDocShell.cpp:8149
#39 0xb6aa3a35 in nsDocShell::LoadURI (this=0xb430f200, aURI=0xaff06780, aLoadInfo=0xb09fff80, aLoadFlags=0, aFirstParty=1)
    at /home/ben/projects/mc/docshell/base/nsDocShell.cpp:1465
#40 0xb6aac281 in nsDocShell::LoadURI (this=0xb430f200, aURI=0xbfffeb40, aLoadFlags=0, aReferringURI=0x0, aPostStream=0x0, 
    aHeaderStream=0x0) at /home/ben/projects/mc/docshell/base/nsDocShell.cpp:3802
#41 0xb6b7c0f7 in nsWebShellWindow::Initialize (this=0xb0999950, aParent=0x0, aOpener=0x0, aShell=0xb29fc880, aUrl=0xaff06540, 
    aInitialWidth=100, aInitialHeight=100, aIsHiddenWindow=1, widgetInitData=...)
    at /home/ben/projects/mc/xpfe/appshell/src/nsWebShellWindow.cpp:259
#42 0xb6b78825 in nsAppShellService::JustCreateTopWindow (this=0xaffea5a0, aParent=0x0, aUrl=0xaff06540, aChromeMask=4094, 
    aInitialWidth=100, aInitialHeight=100, aIsHiddenWindow=1, aAppShell=0xb29fc880, aResult=0xbfffeddc)
    at /home/ben/projects/mc/xpfe/appshell/src/nsAppShellService.cpp:431
#43 0xb6b78176 in nsAppShellService::CreateHiddenWindow (this=0xaffea5a0, aAppShell=0xb29fc880)
    at /home/ben/projects/mc/xpfe/appshell/src/nsAppShellService.cpp:180
#44 0xb6b92572 in nsAppStartup::CreateHiddenWindow (this=0xb0976a00)
    at /home/ben/projects/mc/toolkit/components/startup/nsAppStartup.cpp:193
#45 0xb5ad675f in XRE_main (argc=1, argv=0xbffff484, aAppData=0xb4310380) at /home/ben/projects/mc/toolkit/xre/nsAppRunner.cpp:3497
#46 0x080498f6 in main (argc=1, argv=0xbffff484) at /home/ben/projects/mc/mobile/app/nsBrowserApp.cpp:155

In frame #38, aURI is resource://gre-resources/hiddenWindow.html.

Comment 12

[Benjamin Stover (:stechz)]

Attachment: nsContentPolicy should skip resource and chrome schemes

Comment 13

[Benjamin Stover (:stechz)]

Attachment: nsContentPolicy should skip resource and chrome schemes

Wrong patch.

Comment 14

[Benjamin Stover (:stechz)]

LOL, I just noticed that I first attached a stacktrace before you requested it. Oh well.

Ignore the debug code in nsContentPolicyUtils.h.

I had to patch nsScriptSecurityManager. Apparently comparing a nsRefPtr (or whatever mSystemPrinicipal is) to a regular pointer does not work?!

I also had to add an ignore check in nsWindowWatcher.cpp for OpenWindowJSInternal. Is this a problem?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Apparently comparing a nsRefPtr (or whatever mSystemPrinicipal is) to a regular
> pointer does not work?!

Uh... sure should!  That change should not be needed.

Why exactly did you need to change nsContentPolicyUtils.h?

The change to nsWindowWatcher.cpp looks wrong to me, in general.  Why did you need that change?

Comment 16

[Benjamin Stover (:stechz)]

(In reply to comment #15)
> > Apparently comparing a nsRefPtr (or whatever mSystemPrinicipal is) to a regular
> > pointer does not work?!
> 
> Uh... sure should!  That change should not be needed.

I will doublecheck this.

> Why exactly did you need to change nsContentPolicyUtils.h?

I changed it so I could step over it in GDB.

> The change to nsWindowWatcher.cpp looks wrong to me, in general.  Why did
> you need that change?

Our browser command line handler uses it to open Fennec.
http://mxr.mozilla.org/mozilla-central/source/mobile/components/BrowserCLH.js#52

Comment 17

[Benjamin Stover (:stechz)]

> > Uh... sure should!  That change should not be needed.
> 
> I will doublecheck this.

Doublechecked, and this in fact is equivalent. Phew, sanity restored.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I changed it so I could step over it in GDB.

Right, but we don't want to land it that way.  ;)

> Our browser command line handler uses it to open Fennec.

OK, but it's also used in a bunch of other cases, many of which need the content policy check.

Can you condition the skipping of the security check in the window watcher on at least isCallerChrome (and possibly also hasChromeParent && uriToLoadIsChrome).

Also, I think the check should only be skipped in docshell if this new boolean is set _and_ there is no loadingPrincipal.  The boolean may need to be renamed to make this clear.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch updates the last one Ben created and has Boris' review comments addressed.

I ran this build on my Nexus One. Here are the tMain (tM), tCreateTopLevelWindow (tTLW) and tFirstPaint (tFP):

times in milliseconds and (avg)

Nexus One w/o patch:
tM   = 1282, 1252, 1317 (1284)
tTLW = 1600, 1573, 1639 (1604)
tFP  = 2527, 2508, 2568 (2534)

Nexus One with patch:
tM   = 1244, 1264, 1229 (1246)
tTLW = 1558, 1576, 1538 (1557)
tFP  = 2495, 2499, 2468 (2487)

If we assume this patch does not affect the tMain time, we can just look at the deltas from the times wrt tMain. The tTLW delta is 311ms with patch, 320ms without. tFP delta is 1241ms with patch, 1250ms without.

Not much difference at all. I need to set some breakpoints to see if we are still getting the permissions sqlite DB initialized during startup.

Comment 20

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

sec rev triage = keep watching and waiting

Comment 21

[Daniel Veditz [:dveditz]]

Now that we've switched to native Fennec are we still planning on doing this?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Daniel Veditz from comment #21)
> Now that we've switched to native Fennec are we still planning on doing this?

Given the small differences when I last measured, and the fact native Fennec uses much less JS/XBL, none for the chrome UI in fact, I think we can close this.