Closed Bug 1335526 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

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 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.
Attachment #8832267 - Flags: review?(wmccloskey)
Attachment #8832267 - Flags: review?(bzbarsky)
Attachment #8832267 - Flags: feedback+
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.
Attachment #8832267 - Flags: review?(bzbarsky) → review+
(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.)
Flags: needinfo?(bzbarsky)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a273aee1be72
Ensure that sandboxed channel's result principal is unique; r=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.  :(
Flags: needinfo?(bzbarsky)
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
Flags: needinfo?(ehsan)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac28455fa32
Backed out changeset a273aee1be72 for bustage. r=backout
(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.
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f57fc29ac6
Ensure that sandboxed channel's result principal is unique; r=bzbarsky
https://hg.mozilla.org/mozilla-central/rev/97f57fc29ac6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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...
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan) → needinfo?(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....
Flags: needinfo?(bzbarsky)
Depends on: 1337179
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.