508990 - ###!!! ASSERTION: Principal mismatch. Not good

Status: RESOLVED INVALID

(Core :: Security: CAPS, defect)

Comment 1

[Alex Vincent [:WeirdAl]]

Sorry, my browser submitted the report early.

Steps to reproduce:
(1) Build debug XULRunner, or debug Firefox (use -app)
(2) hg clone
http://hg.mozdev.org/verbosio/file/7fc1345e79e5/experimental/template-edit
template-edit
(3) xulrunner template-edit/application.ini
(4) Click "openTestWindow" button.  A XUL wizard will appear.
(5) Hit the "Next" button 4 times.  You will reach the "Build primary user
interface" page.

Right about now you should be hitting the assertion many, many times.  This is
what I was seeing when I wrote bug 474695 comment 12.

I'll be happy to reduce this testcase if desired.

Comment 2

[Alex Vincent [:WeirdAl]]

###!!! ASSERTION: Principal mismatch.  Not good: '!aAllowShortCircuit || result == doGetObjectPrincipal(origObj, PR_FALSE)', file .../mozilla-1.9.1/caps/src/nsScriptSecurityManager.cpp, line 2381

Comment 3

[Daniel Veditz [:dveditz]]

A reduced testcase would help tremendously. If it's multi-file (given the binding) try zipping it and attaching it with MIME type application/java-archive.

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: minimized testcase application

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: stack trace

From the stack trace, it looks like it asserts trying to execute the binding's constructor.

Comment 6

[Boris Zbarsky [:bzbarsky]]

2421        NS_ASSERTION(!aAllowShortCircuit ||
2422                     result == doGetObjectPrincipal(origObj, PR_FALSE),
2423                     "Principal mismatch.  Not good");
2424        
2425        return result;
(gdb) p result
$1 = (nsSystemPrincipal *) 0x847d90
(gdb) p doGetObjectPrincipal(origObj, 0)
$2 = (nsNullPrincipal *) 0x1bf78110

The key problem is here:

(gdb) frame 9
#9  0x00f9f322 in nsScriptSecurityManager::CheckFunctionAccess (this=0x83f9c0, aCx=0x11d3e00, aFunObj=0x14e0bc00, aTargetObj=0x14e0bb20) at /Users/bzbarsky/mozilla/vanilla/mozilla/caps/src/nsScriptSecurityManager.cpp:1677
(gdb) p JS_GetParent(aCx,aFunObj)
$24 = (JSObject *) 0x14e0bb20
(gdb) p JS_GetPrivate(aCx, $24)
$26 = (void *) 0x1bf7c0c0
(gdb) p (nsISupports*)$26
$27 = (XPCWrappedNative *) 0x1bf7c0c0
(gdb) p $27->mIdentity
$28 = (nsXULElement *) 0x1bf79fe0
$30 = (nsXULElement *) 0x1bf79fe0
(gdb) p $30->GetOwnerDoc()
$31 = (nsXULDocument *) 0x12d8200
(gdb) p $30->GetNodeParent()
$32 = (nsXULElement *) 0x827760
(gdb) p $30->GetNodeParent()->GetNodeParent()
$33 = (nsXULElement *) 0x827af0
(gdb) p $30->GetNodeParent()->GetNodeParent()->GetNodeParent()
$34 = (nsXULDocument *) 0x12d8200

But...

(gdb) p JS_GetParent(aCx, $24)
$35 = (JSObject *) 0x14e0b7c0
(gdb) p JS_GetParent(aCx, $35)
$36 = (JSObject *) 0x14e0b7a0
(gdb) p ((nsISupports*)JS_GetPrivate(aCx, $36))->mIdentity
$41 = (nsXMLDocument *) 0x10adc00

That last is what has the null principal.

So the issue is that the JS parent chain is not in sync with the node parent chain, which it should be for XUL elements.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And the relevant JS looks somewhat like this:

  var insertNode = getSingleNode(outputSource, {
    xmlns: XUL_NS
  });

  var outputRenderBox = document.getElementById("primaryUI-outputRenderBox");
  outputRenderBox.appendChild(insertNode);

where:

function getSingleNode(source, namespaceMap)
{
....
  const xml = openTag + source + "</root>";
  const doc = parser.parseFromString(xml, "application/xml");
  const node = doc.documentElement.firstChild;
  return (node.nextSibling ? null : node);

So there it makes sense that the node would have an XMLDocument as its grandparent.  Why are we not reparenting the node properly on that appendChild?

Comment 8

[Alex Vincent [:WeirdAl]]

Thanks, bz.  I filed this as security-sensitive based on other bugs I found in Bugzilla like this which were once classified.  Question:  should this still be security-sensitive?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Er, I thought I'd commented here with what's going on.  I wonder where that went...

The key is that we don't reparent wrappers for XUL nodes.  And since this code doesn't use new DOMParser but uses createInstance instead, it doesn't get a DOMParser with the right principal (which a web page would).

That said, a web page could trigger this assertion by simply having two separate documents with XUL nodes in them (same-origin, but different principals) and moving a node from one to the other.

I _think_ there isn't a security problem here, but could we throw in cases when moving a XUL node, not reparenting, and the two principals involved aren't equal, just in case?

Comment 10

[Alex Vincent [:WeirdAl]]

bz: I'm not sure I can simply move the node without reparenting and still hit the assertion.  For the XBL constructor to fire, the binding has to attach, and that requires the bound node be in the rendered document.

Unless I'm missing something obvious.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I have no idea what comment 10 is talking about....

Comment 12

[Brandon Sterne (:bsterne)]

[sg:nse] to match bug 474695.

Comment 13

[Josh Aas]

Opening up per comment 9 and a discussion with bz on irc.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Wow, this one was filed 11 years ago. I just checked our caps/ code and there is no string 'Principal mismatch' anymore. I guess the code changed too much in those 11 years. I'll just go ahead and mark this one as INVALID.