1335526 - SEC_SANDBOXED document loads fatally assert with Assertion failure: mDocGroup->MatchesKey(docGroupKey), at nsDocument.cpp:2985

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

Similar to bug 1334047, this was hidden on our test machines due to the URLs we were using causing us to get an empty doc group key here.

This can for example happen for the resulting document from an XHR.  We first assign the principal with a call stack like this:

#0  mozilla::dom::DocGroup::GetKey (aPrincipal=0x7fafe019dcf0, aKey=...) at /home/ehsan/moz/src3/dom/base/DocGroup.cpp:20
#1  0x00007fb00bad43ac in nsDocument::SetScopeObject (this=0x7faff3e7d000, aGlobal=0x7fafdca05910) at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:4509
#2  0x00007fb00bad5be0 in nsDocument::SetScriptHandlingObject (this=0x7faff3e7d000, aScriptObject=0x7fafdca05910) at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:4855
#3  0x00007fb00d7e5bbc in NS_NewDOMDocument (aInstancePtrResult=0x7ffce4d2c5f0, aNamespaceURI=..., aQualifiedName=..., aDoctype=0x0, aDocumentURI=0x7fafdfcfc8b0,
    aBaseURI=0x7fafdfcfc8b0, aPrincipal=0x7fafe019dcf0, aLoadedAsData=true, aEventObject=0x7fafdca05910, aFlavor=DocumentFlavorLegacyGuess)
    at /home/ehsan/moz/src3/dom/xml/XMLDocument.cpp:147
#4  0x00007fb00d96c025 in mozilla::dom::XMLHttpRequestMainThread::OnStartRequest (this=0x7fafe038b800, request=0x7fafdcab9c20, ctxt=0x0)
    at /home/ehsan/moz/src3/dom/xhr/XMLHttpRequestMainThread.cpp:1997
#5  0x00007fb00a29f41d in mozilla::net::nsStreamListenerWrapper::OnStartRequest (this=0x7fafebfd9550, aRequest=0x7fafdcab9c20, aContext=0x0)
    at /home/ehsan/moz/src3/netwerk/base/nsStreamListenerWrapper.h:30

And then the second time we hit it with this call stack:

#0  mozilla::dom::DocGroup::GetKey (aPrincipal=0x7fafe1664ac0, aKey=...) at /home/ehsan/moz/src3/dom/base/DocGroup.cpp:20
#1  0x00007fb00bacee6f in nsIDocument::GetDocGroup (this=0x7faff3e7d000) at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:2983
#2  0x00007fb00bacede8 in nsDocument::SetPrincipal (this=0x7faff3e7d000, aNewPrincipal=0x7fafe1664ac0) at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:2969
#3  0x00007fb00bacafa8 in nsDocument::ResetToURI (this=0x7faff3e7d000, aURI=0x7fafdfcfc8b0, aLoadGroup=0x7fafd73d9ab0, aPrincipal=0x7fafe1664ac0)
    at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:2203
#4  0x00007fb00d7e690b in mozilla::dom::XMLDocument::ResetToURI (this=0x7faff3e7d000, aURI=0x7fafdfcfc8b0, aLoadGroup=0x7fafd73d9ab0, aPrincipal=0x7fafe1664ac0)
    at /home/ehsan/moz/src3/dom/xml/XMLDocument.cpp:276
#5  0x00007fb00baca871 in nsDocument::Reset (this=0x7faff3e7d000, aChannel=0x7fafdcab9c20, aLoadGroup=0x7fafd73d9ab0) at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:2087
#6  0x00007fb00d7e687f in mozilla::dom::XMLDocument::Reset (this=0x7faff3e7d000, aChannel=0x7fafdcab9c20, aLoadGroup=0x7fafd73d9ab0)
    at /home/ehsan/moz/src3/dom/xml/XMLDocument.cpp:263
#7  0x00007fb00bacc61f in nsDocument::StartDocumentLoad (this=0x7faff3e7d000, aCommand=0x7fb01111324c <kLoadAsData> "loadAsData", aChannel=0x7fafdcab9c20,
    aLoadGroup=0x7fafd73d9ab0, aContainer=0x0, aDocListener=0x7ffce4d2c620, aReset=true, aSink=0x0) at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:2478
#8  0x00007fb00d7e7813 in mozilla::dom::XMLDocument::StartDocumentLoad (this=0x7faff3e7d000, aCommand=0x7fb01111324c <kLoadAsData> "loadAsData", aChannel=0x7fafdcab9c20,
    aLoadGroup=0x7fafd73d9ab0, aContainer=0x0, aDocListener=0x7ffce4d2c620, aReset=true, aSink=0x0) at /home/ehsan/moz/src3/dom/xml/XMLDocument.cpp:524
#9  0x00007fb00d96c50b in mozilla::dom::XMLHttpRequestMainThread::OnStartRequest (this=0x7fafe038b800, request=0x7fafdcab9c20, ctxt=0x0)
    at /home/ehsan/moz/src3/dom/xhr/XMLHttpRequestMainThread.cpp:2040
#10 0x00007fb00a29f41d in mozilla::net::nsStreamListenerWrapper::OnStartRequest (this=0x7fafebfd9550, aRequest=0x7fafdcab9c20, aContext=0x0)
    at /home/ehsan/moz/src3/netwerk/base/nsStreamListenerWrapper.h:30

The channel final URI is the same, but I think this bug is stemming from us creating a unique null principal every time here <http://searchfox.org/mozilla-central/rev/4e0c5c460318fb9ef7d92b129ac095ce04bc4795/caps/nsScriptSecurityManager.cpp#296>.  This causes us to get two different UUIDs and ultimately the docgroup key comparison fails.

Comment 1

[(no longer active)]

Attachment: Ensure that sandboxed channel's result principal is unique

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8832267 [details] [diff] [review]
Ensure that sandboxed channel's result principal is unique

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

This makes sense to me, but I'd feel better if Boris looked it over. It seems safe, since we're unlikely to use the same loadinfo if we actually expect to get a different null principal. But it's caps code and I don't know what I'm doing.

Since it took me a minute to track down, the two calls to GetChannelResultPrincipal are here (I think):
http://searchfox.org/mozilla-central/rev/4e0c5c460318fb9ef7d92b129ac095ce04bc4795/dom/xhr/XMLHttpRequestMainThread.cpp#1990
http://searchfox.org/mozilla-central/rev/4e0c5c460318fb9ef7d92b129ac095ce04bc4795/dom/base/nsDocument.cpp#2082

I'm a little skeptical of the need for the IPC parts, but I guess it can't hurt.

::: caps/nsScriptSecurityManager.cpp
@@ +292,5 @@
>      }
>  
>      if (loadInfo) {
>          if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {
> +          return loadInfo->GetSandboxedLoadingPrincipal(aPrincipal);

I'd feel better if we could assert that aPrincipal != null after this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8832267 [details] [diff] [review]
Ensure that sandboxed channel's result principal is unique

r=me.  Luckily, we don't have to add this to session history or session restore stuff, because we only need this on a per-channel basis....

That said, if a document's principal mutating like this is a problem, we may also have a problem with XMLDocument::Load(), which creates a whole new (same-origin, as far as it goes) channel.  Might be worth a followup to look into the state of things there.

Comment 4

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #2)
> Since it took me a minute to track down, the two calls to
> GetChannelResultPrincipal are here (I think):
> http://searchfox.org/mozilla-central/rev/
> 4e0c5c460318fb9ef7d92b129ac095ce04bc4795/dom/xhr/XMLHttpRequestMainThread.
> cpp#1990
> http://searchfox.org/mozilla-central/rev/
> 4e0c5c460318fb9ef7d92b129ac095ce04bc4795/dom/base/nsDocument.cpp#2082

These two are in fact the two call sites involved in this bug.

> I'm a little skeptical of the need for the IPC parts, but I guess it can't
> hurt.

The IPC parts were just added for completeness, it's hard to know exactly for a fact that they aren't needed, so I decided to play safe.


(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> That said, if a document's principal mutating like this is a problem, we may
> also have a problem with XMLDocument::Load(), which creates a whole new
> (same-origin, as far as it goes) channel.  Might be worth a followup to look
> into the state of things there.

Is this what you mean? <http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/dom/xml/XMLDocument.cpp#397>  The principal is in fact staying the same here.  (And the tests in dom/xml are passing with my changes.)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a273aee1be72
Ensure that sandboxed channel's result principal is unique; r=bzbarsky

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Is this what you mean?

No.  I mean the bit after that when we NS_NewChannel and then call StartDocumentLoad().  But I guess the key part is we pass false for aReset, so StartDocumentLoad doesn't mess with principals.  What a mess.  :(

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a273aee1be7203952c39b4fd00df915e6f9ab15a
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=73584563&repo=mozilla-inbound

[task 2017-02-01T16:40:54.057942Z] 16:40:54     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/caps/Unified_cpp_caps0.cpp:47:0:
[task 2017-02-01T16:40:54.058865Z] 16:40:54     INFO -  /home/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp: In member function 'nsresult nsScriptSecurityManager::GetChannelResultPrincipal(nsIChannel*, nsIPrincipal**, bool)':
[task 2017-02-01T16:40:54.059658Z] 16:40:54     INFO -  /home/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:296:70: error: could not convert 'loadInfo.nsCOMPtr<T>::operator-><nsILoadInfo>()->nsILoadInfo::GetSandboxedLoadingPrincipal(aPrincipal)' from 'nsresult' to 'bool'
[task 2017-02-01T16:40:54.060365Z] 16:40:54     INFO -             MOZ_ALWAYS_TRUE(loadInfo->GetSandboxedLoadingPrincipal(aPrincipal));
[task 2017-02-01T16:40:54.060450Z] 16:40:54     INFO -                                                                        ^
[task 2017-02-01T16:40:54.061116Z] 16:40:54     INFO -  /home/worker/workspace/build/src/config/rules.mk:1006: recipe for target 'Unified_cpp_caps0.o' failed
[task 2017-02-01T16:40:54.061188Z] 16:40:54     INFO -  gmake[5]: *** [Unified_cpp_caps0.o] Error 1

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac28455fa32
Backed out changeset a273aee1be72 for bustage. r=backout

Comment 9

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6)
> > Is this what you mean?
> 
> No.  I mean the bit after that when we NS_NewChannel and then call
> StartDocumentLoad().  But I guess the key part is we pass false for aReset,
> so StartDocumentLoad doesn't mess with principals.  What a mess.  :(

Yeah... But that should be fine as far as the doc group calculation is concerned.  However there are other types of bugs lurking around in the codebase still as my try push shows.  I need to debug stuff further.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f57fc29ac6
Ensure that sandboxed channel's result principal is unique; r=bzbarsky

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/97f57fc29ac6

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm.  I wonder about the copy constructor copying this cached thing.  It means that if inside a sandboxed iframe we see a redirect from foo.com to bar.com, both channels have the same GetChannelResultPrincipal().

On the other hand, if we don't copy a redirect from foo.com to foo.com would get different result principals...

Comment 13

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12)
> Hmm.  I wonder about the copy constructor copying this cached thing.  It
> means that if inside a sandboxed iframe we see a redirect from foo.com to
> bar.com, both channels have the same GetChannelResultPrincipal().
> 
> On the other hand, if we don't copy a redirect from foo.com to foo.com would
> get different result principals...

Given the iframe is sandboxed, it should be considered as cross origin to everything.  So it seems to me that the second case would be fine (and should already be the case before these changes.)  Is there something that I'm missing about that?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I think that would be fine, personally.  bkelly is trying to use channel result principals for something in serviceworkers where it's not clear what behavior we want, but it's also not clear to me that we would want channel result principals there to start with....