Closed Bug 791284 Opened 12 years ago Closed 12 years ago

"ASSERTION: mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID" at nsPrincipal.cpp:1088 when running browser-chrome mochitest browser_tabview_bug610242.js and calling GetAppStatus from nsDocument::InitCSP()

Categories

(Core :: Security: CAPS, defect)

Product:
Core
Component:
Security: CAPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: geekboy, Assigned: grobinson)

References

Details

Attachments

(1 file, 4 obsolete files)

Patch 01
3.08 KB, patch
sicking
: review+
Details | Diff | Splinter Review 
Assertion failure: mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID, at ../../../caps/src/nsPrincipal.cpp:1088
WARNING: shutting down early because of crash!: file ../../../../dom/plugins/ipc/PluginModuleChild.cpp, line 705
WARNING: plugin process _exit()ing: file ../../../../dom/plugins/ipc/PluginModuleChild.cpp, line 670
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug610242.js 

Stack looks like this:

 0  libxul.so!nsPrincipal::GetAppStatus [nsPrincipal.cpp : 1088 + 0x1f]
 1  libxul.so!nsDocument::InitCSP [nsDocument.cpp : 2454 + 0x8]
 2  libxul.so!nsDocument::StartDocumentLoad [nsDocument.cpp : 2426 + 0x5]
 3  libxul.so!nsHTMLDocument::StartDocumentLoad [nsHTMLDocument.cpp : 602 + 0x1c]
 4  libxul.so!nsContentDLF::CreateDocument [nsContentDLF.cpp : 430 + 0x22]
 5  libxul.so!nsContentDLF::CreateInstance [nsContentDLF.cpp : 232 + 0x1c]
 6  libxul.so!nsDirectoryViewerFactory::CreateInstance [nsDirectoryViewer.cpp : 1380 + 0x28]
 7  libxul.so!nsDocShell::NewContentViewerObj [nsDocShell.cpp : 7849 + 0x28]
 8  libxul.so!nsDocShell::CreateContentViewer [nsDocShell.cpp : 7649 + 0x17]
 9  libxul.so!nsDSURIContentListener::DoContent [nsDSURIContentListener.cpp : 120 + 0x12]


I'm calling NodePrincipal()->GetAppStatus(&appStatus) from inside InitCSP, and when I run this browser chrome mochitest it crashes with an assertion.

Only happens in this one test as far as I can tell, and only happens with my patch for bug 768029.

Link to try run where it happened:
https://tbpl.mozilla.org/?tree=Try&rev=92abaab3da91

Relevant log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15195499&tree=Try#error0
Summary: "ASSERTION: mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID" at nsPrincipal.cpp:1088 when running browser-chrome mochitest browser_bug610242.js and calling GetAppStatus from nsDocument::InitCSP() → "ASSERTION: mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID" at nsPrincipal.cpp:1088 when running browser-chrome mochitest browser_tabview_bug610242.js and calling GetAppStatus from nsDocument::InitCSP()
nsIScriptSecurityManager::GetSimpleCodebasePrincipal() can cause a principal to have UNKNOWN_APP_ID, which is called from the xpcshell stuff:
http://mxr.mozilla.org/mozilla-central/source/ipc/testshell/XPCShellEnvironment.cpp#769
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/shell/xpcshell.cpp#1401

It's also called from the Weave.js service.
http://mxr.mozilla.org/mozilla-central/source/services/sync/Weave.js#66

When I convert the ssm.getSimpleCodebasePrincipal() to ssm.getNoAppCodebasePrincipal() the test succeeds.

Cc'ing gavin, since he might know what's needed for the Weave.js stuff.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
There's likely going to be many principals with UNKNOWN_APP_ID. However they *should* never end up being the principal for a document, which is where you get the principal from when you are calling GetAppStatus, right?

Do you have any idea how come we create a document and gave it a principal with UNKNOWN_APP_ID?
(In reply to Jonas Sicking (:sicking) from comment #2)
> There's likely going to be many principals with UNKNOWN_APP_ID. However they
> *should* never end up being the principal for a document,
That sounds odd? Why not? Is that documented somewhere?
There's just too many places where we create principals where it's either too hard or outright impossible without major API changes to get enough context that we can figure out which app a principal is created for.

However in most of these cases getting the right appid/browserflag also doesn't matter. A simple example is when creating principals for stylesheets. We never use the principal of a stylesheet for anything where the appid/browserflag matters.

However our intent was to always ensure to find the right appid/browserflag when creating principals that were used to access storage. I.e. when accessing localStorage/cookies/indexedDB/etc. And that obviously include any principal which is used to execute script, which means any principal used as the principal for a document.

Technically speaking some document principals aren't ever used to execute script, but to keep things simple we always ensured to find the right appid/browserflag when creating principals for documents.

We probably did miss some places though. Otherwise we wouldn't have this bug :)
Well, as far as I see nothing guarantees that mAppId isn't UNKNOWN_ADD_ID so
assertion shouldn't be used.
I don't think it's true that we should only assert for things that we have clear and safe guarantees for. In fact, I would argue that the things that are important to assert for are the things where enforcing the conditions is complex and so there's a higher risk of bugs. That's when an assertion can be useful to catch such bugs.

I definitely agree that we shouldn't assert things that webpages can cause to happen. The intent is that that isn't the case here. If it is, that's a bug that needs to be fixed. If you see any such cases please point them out.

Either way, removing the assertion is the wrong fix here. Anything that can cause the assertion to fire is potentially a security problem and needs to be fixed.
(In reply to Jonas Sicking (:sicking) from comment #2)
> Do you have any idea how come we create a document and gave it a principal
> with UNKNOWN_APP_ID?

Displaying the weave log apparently is one place.

I set a breakpoint in the test I'm crashing and it put me here:

http://mxr.mozilla.org/mozilla-central/source/services/sync/Weave.js#66
66     let principal = ssm.getSimpleCodebasePrincipal(uri);

Calls:
http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1998
1998 nsScriptSecurityManager::GetSimpleCodebasePrincipal(nsIURI* aURI,
1999                                                     nsIPrincipal** aPrincipal)
2000 {
2001   return GetCodebasePrincipalInternal(aURI,
2002                                       nsIScriptSecurityManager::UNKNOWN_APP_ID,
2003                                       false, aPrincipal);
2004 }

Notice line 2002.
And you can get the same assertion by applying my patch in bug 768029, opening Firefox, and loading "about:sync-log".
Ah, indeed, that needs to call getNoAppCodebasePrincipal. And I reviewed that even :( This stuff is indeed hard to get right, which is exactly why we need these assertions.

We should probably add an assertion in nsDocument which asserts earlier if someone tries to use a channel with a UNKNOWN_APP_ID owner to create a document.
Attached patch patch (obsolete) — — Splinter Review 
Threw this together.  This what you had in mind, Jonas?
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #661638 - Flags: review?(jonas)
Comment on attachment 661638 [details] [diff] [review]
patch

Review of attachment 661638 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +2374,5 @@
> +  uint32_t appId;
> +  nsresult rv = NodePrincipal()->GetAppId(&appId);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  MOZ_ASSERT(appId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
> +             "Document should never have UNKNOWN_APP_ID");

You should put all of this in an #ifdef DEBUG. And add a {} scope around it so that the nsresult rv declaration doesn't leak outside of it.
Attachment #661638 - Flags: review?(jonas) → review+
Attached patch patch (obsolete) — — Splinter Review 
Carrying over r=sicking
Attachment #661638 - Attachment is obsolete: true
Attachment #661656 - Flags: review+
Spent some time in gdb.  Here's the top of the stack (trimmed):
#0  nsExpandedPrincipal::GetAppId  at src/caps/src/nsPrincipal.cpp:1529
#1  nsDocument::StartDocumentLoad  at src/content/base/src/nsDocument.cpp:2377
#2  nsXMLDocument::StartDocumentLoad  at src/content/xml/document/src/nsXMLDocument.cpp:498
#3  nsXMLHttpRequest::OnStartRequest  at src/content/base/src/nsXMLHttpRequest.cpp:2173
#4  nsCORSListenerProxy::OnStartRequest  at src/content/base/src/nsCrossSiteListenerProxy.cpp:449
#5  mozilla::net::nsHttpChannel::CallOnStartRequest  at src/netwerk/protocol/http/nsHttpChannel.cpp:961
#6  mozilla::net::nsHttpChannel::ContinueProcessNormal  at src/netwerk/protocol/http/nsHttpChannel.cpp:1455
#7  mozilla::net::nsHttpChannel::ProcessNormal  at src/netwerk/protocol/http/nsHttpChannel.cpp:1390
#8  mozilla::net::nsHttpChannel::ProcessResponse  at src/netwerk/protocol/http/nsHttpChannel.cpp:1225
#9  mozilla::net::nsHttpChannel::OnStartRequest  at src/netwerk/protocol/http/nsHttpChannel.cpp:4815
#10 nsInputStreamPump::OnStateStart  at src/netwerk/base/src/nsInputStreamPump.cpp:417
#11 nsInputStreamPump::OnInputStreamReady  at src/netwerk/base/src/nsInputStreamPump.cpp:368
#12 nsInputStreamReadyEvent::Run  at src/xpcom/io/nsStreamUtils.cpp:82
#13 nsThread::ProcessNextEvent  at src/xpcom/threads/nsThread.cpp:624
#14 NS_ProcessNextEvent_P  at src/obj-ff/xpcom/build/nsThreadUtils.cpp:220
#15 nsXMLHttpRequest::Send  at src/content/base/src/nsXMLHttpRequest.cpp:3031
#16 nsXMLHttpRequest::Send  at src/content/base/src/nsXMLHttpRequest.h:346
#17 nsXMLHttpRequest::Send  at src/content/base/src/nsXMLHttpRequest.h:356
#18 nsXMLHttpRequest::Send  at src/content/base/src/nsXMLHttpRequest.h:375
#19 mozilla::dom::XMLHttpRequestBinding::send  at src/obj-ff/dom/bindings/XMLHttpRequestBinding.cpp:461
#20 mozilla::dom::XMLHttpRequestBinding::genericMethod  at src/obj-ff/dom/bindings/XMLHttpRequestBinding.cpp:1082
#21 js::CallJSNative  at src/js/src/jscntxtinlines.h:372
...

And here's the tl;dr:

The test is using Components.utils.sandbox to set up an XHR to "http://localhost:4444/simple".  That's causing a new XHR to be created that instantiates an XMLDocument.  

As that document loads, it fails when the call to GetAppId returns an error: NS_NOT_AVAILABLE.
http://mxr.mozilla.org/mozilla-central/source/caps/src/nsPrincipal.cpp#1529  

That's because the NodePrincipal() is an nsExpandedPrincipal, which doesn't make an app ID available.

Not sure what the right thing to do is here because I'm in way over my head with the XHR code.  I could add a check to the assertion to catch rv == NS_NOT_AVAILABLE, but that doesn't seem like the right path.  Maybe we want to skip the assertion for things loaded as data?  (if !(nsCRT::strcmp(kLoadAsData, aCommand) == 0))  Need some advice here.
I have no idea what nsExpandedPrincipal is :(

cc'ing people who might be able to help out here.

I definitely would prefer that all documents get principals with real appids etc. But maybe we are intentionally giving these documents principals that are neutered in some important ways.
(In reply to Jonas Sicking (:sicking) from comment #17)
> I have no idea what nsExpandedPrincipal is :(

I invented the EPs for jetpack content-scripts. They are basically an array of principals, and sandbox with an EP can have same origin access to all the origins listen in the array. It came with a custom ctor for XHR for sandboxes, this way it's possible to XHR to those origins from the sandbox. 

That being said I have no idea what are appids, and even less clue why they are throw and error for EPs (not my implementation for sure...). Shouldn't we simply move all these appid bits from nsPrincipal to nsBasePrincipal by the way?
nsExpandedPrincipals are designed to let you subsume more than one origin, but not everything (as SystemPrincipal does).

We implemented nsExpandedPrincipal::GetAppID etc to throw because they're only used in contexts (like jetpack scopes) where an app id doesn't make sense. But maybe they should return unknown?
(In reply to Bobby Holley (:bholley) from comment #19)
> We implemented nsExpandedPrincipal::GetAppID etc to throw because they're
> only used in contexts (like jetpack scopes) where an app id doesn't make
> sense. But maybe they should return unknown?

I don't think they should return UNKNOWN_APP_ID, since we want to have an assertion detect for creation of documents that have that instead of NO_APP_ID or an ID.

Would it make sense to do the same thing we did in Weave.js?  Maybe return NO_APP_ID instead of UKNOWN_APP_ID and not throw.  Would an app ever have an nsExpandedPrincipal?

Or should I just make the assertion conditional on a successful nsresult?  It would be easy to only assert if GetAppId succeeds.
(In reply to Sid Stamm [:geekboy] from comment #20)
> (In reply to Bobby Holley (:bholley) from comment #19)
> I don't think they should return UNKNOWN_APP_ID, since we want to have an
> assertion detect for creation of documents that have that instead of
> NO_APP_ID or an ID.

Maybe NO_APP_ID then?

> Would it make sense to do the same thing we did in Weave.js?  Maybe return
> NO_APP_ID instead of UKNOWN_APP_ID and not throw.  Would an app ever have an
> nsExpandedPrincipal?

They shouldn't. This should just be for pseudo-privileged extension code that we're trying to avoid give a full SystemPrincipal to.

> Or should I just make the assertion conditional on a successful nsresult? 
> It would be easy to only assert if GetAppId succeeds.

It depends. What's the preferred way to ask if a principal is associated with an app? If the answer is "get the ID", then I think we should not throw, and instead return something that says "I have nothing to do with the whole app thing".
(In reply to Bobby Holley (:bholley) from comment #21)
> (In reply to Sid Stamm [:geekboy] from comment #20)
> > (In reply to Bobby Holley (:bholley) from comment #19)
> > I don't think they should return UNKNOWN_APP_ID, since we want to have an
> > assertion detect for creation of documents that have that instead of
> > NO_APP_ID or an ID.
> 
> Maybe NO_APP_ID then?

Jonas?  What do you think?

> It depends. What's the preferred way to ask if a principal is associated
> with an app? If the answer is "get the ID", then I think we should not
> throw, and instead return something that says "I have nothing to do with the
> whole app thing".

I think one either asks for the AppId or asks for the AppStatus.  Both currently throw in nsExpandedPrincipal.  Perhaps we should make those successfully return NO_APP_ID instead.
Attached patch proposed patch (obsolete) — — Splinter Review 
bholley: here's a patch that tweaks nsExpandedPrincipal to not throw on app-related queries.  Can you let me know if this kind of thing would work?  Seems to solve the problem that caused the backout of the previous patch.
Attachment #661656 - Attachment is obsolete: true
Attachment #661902 - Flags: review?(bobbyholley+bmo)
Comment on attachment 661902 [details] [diff] [review]
proposed patch

This seems reasonable, but I don't really know how this stuff gets used, so I think we need sicking to do the review. On that note, I feel like I ought to understand this stuff better. Do we have any documents on the b2g security model that I should read?


>+#ifdef DEBUG
>+  {
>+    // GetAppId throws for nsExpandedPrincipals, so don't check when the App ID
>+    // is not available

This is no longer true, right?


>     // Ensure that the about page has the same privileges as a regular directory
>     // view. That way links to files can be opened.
>     let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
>                 .getService(Ci.nsIScriptSecurityManager);
>-    let principal = ssm.getSimpleCodebasePrincipal(uri);
>+    let principal = ssm.getNoAppCodebasePrincipal(uri);
>     channel.owner = principal;

I don't know the context here
Attachment #661902 - Flags: review?(jonas)
Attachment #661902 - Flags: review?(bobbyholley+bmo)
Attachment #661902 - Flags: feedback+
(In reply to Bobby Holley (:bholley) from comment #24)
> Comment on attachment 661902 [details] [diff] [review]
> proposed patch
> 
> This seems reasonable, but I don't really know how this stuff gets used, so
> I think we need sicking to do the review. On that note, I feel like I ought
> to understand this stuff better. Do we have any documents on the b2g
> security model that I should read?

This is a good place to start:
https://wiki.mozilla.org/Apps/Security

> >+    // GetAppId throws for nsExpandedPrincipals, so don't check when the App ID
> >+    // is not available
> 
> This is no longer true, right?

Ah, yeah, sorry, from a previous patch where I did an if(NS_SUCCEEDED...) around the call to GetAppId().  I'll delete it.


> >-    let principal = ssm.getSimpleCodebasePrincipal(uri);
> >+    let principal = ssm.getNoAppCodebasePrincipal(uri);
> >     channel.owner = principal;
> 
> I don't know the context here

Jonas r+'ed the change and suggested it in comment 9 (this is the line of code that triggered filing of this bug).
Thanks for the r+, Jonas.  This isn't the only thing that's hitting that assertion; there are a couple others surfaced in this try run (also mentioned on bug 768029):
https://tbpl.mozilla.org/?tree=Try&rev=90e71284981c

I think the patches to nsExpandedPrincipal will take care of a lot of the documents-with-UNKNOWN_APP_ID cases, but not all of them so I think we should do something about the assertions since until we root out all the documents-with-UNKNOWN_APP_IDs, we're gonna get assertions in the patch for bug 768029.  Perhaps nsIPrincipal::GetApp{Id,Stauts} can just return a failure code and the caller can handle the failure?
Attached patch patch (obsolete) — — Splinter Review 
Addressed bholley's review in comment 24 (removed a comment in the code).  Carrying over r=sicking, f=bholley
Attachment #661902 - Attachment is obsolete: true
Attachment #663229 - Flags: review+
Attachment #663229 - Flags: feedback+
Attachment #663229 - Attachment is patch: true
Jonas: do we still want this?
Flags: needinfo?(jonas)
The right fix here seems to me to be to make nsExpandedPrincipal always ensure that the principals that it is made to subsume are always NS_NO_APP principals. And then make nsExpandedPrincipal return NS_NO_APP for appid.

And yes, I do think we still want this.
Flags: needinfo?(jonas)
Looks like bug 799152 made the changes this patch wanted in nsPrincipal.cpp.  Didn't notice that one land.

The debugging in nsDocument::StartDocumentLoad should still probably go in and the one line change in Weave.js should probably still go in to fix the tests.
Assignee: sstamm → grobinson
Attached patch Patch 01 — — Splinter Review 
I've applied Sid's patch to m-c, omitting the changes to nsPrincipal.cpp that were subsumed by Bug 799152. The patch applies cleanly and the test that was causing an assertion, browser/components/tabview/test/browser_tabview_bug610242.js, passes.

I've pushed this to try to see if all of the other assorted breakage is also fixed: https://tbpl.mozilla.org/?tree=Try&rev=b0d9801020ee
Attachment #663229 - Attachment is obsolete: true
Comment on attachment 750188 [details] [diff] [review]
Patch 01

Try run was good.
Attachment #750188 - Flags: review?(jonas)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/808d77db43f1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 736036
Comment on attachment 750188 [details] [diff] [review]
Patch 01

Review of attachment 750188 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +2346,5 @@
> +#ifdef DEBUG
> +  {
> +    uint32_t appId;
> +    nsresult rv = NodePrincipal()->GetAppId(&appId);
> +    NS_ENSURE_SUCCESS(rv, rv);

This return in debug builds only looks fishy to me...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: