493495 - Domain policy checks recurse into themselves when used with nsIURI object implemented in JS

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Manish Singh]

s3:// (https://addons.mozilla.org/en-US/firefox/addon/6955) implements an nsIURI object in JS to wrap URLs on Amazon S3. Changes in FF 3.5 seem to have broken this, with security checks for domain policies causing an infinite loop.

I've made a smaller test case to illustrate the problem.

Comment 1

[Manish Singh]

Attachment: Test case component

This triggers the problem. You can go to either jsprotocoltest://foo/ to see one case, or jsprotocoltest://img/ to see another. One tries to load a test file, the other tries to load an image.

Comment 2

[Manish Singh]

Attachment: Extension xpi for testing

This is an extension with said component, for ease in testing.

Comment 3

[Manish Singh]

Attachment: Debug spew excerpt when loading jsprotocoltest://foo/

Comment 4

[Manish Singh]

Attachment: Debug spew excerpt when loading jsprotocoltest://img/

Comment 5

[Manish Singh]

Implementing nsINestedURI gets rid of some of the assertions, but not all.

All this code works perfectly fine in Firefox 3, so this is a regression.

Comment 6

[Manish Singh]

Oh, and the debug spew is from a mozilla-central build from today.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Can you find which patch caused this regression by obtaining a regression range?

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

bz: can you take a look to see where the actual bug is here?

Tomcat or Ria: can we get a regression window?

Comment 9

[(mostly gone) XtC4UaLL [:xtc4uall]]

so, i found *a* regrange, unsure if it's *the* range.

i installed the extension from comment 2 and tested "jsprotocoltest://foo/" & "jsprotocoltest://img/" what give you a hang in current 1.9.1 branch and MC trunk builds.

going back i found this:

works (no hangs):
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre ID:20081105032350
http://hg.mozilla.org/mozilla-central/rev/dcec193ba5d7

fails (Fx hanging):
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081106 Minefield/3.1b2pre ID:20081106033429
http://hg.mozilla.org/mozilla-central/rev/f3993df71c29

=> range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dcec193ba5d7&tochange=f3993df71c29

Comment 10

[Johnny Stenback (:jst)]

We end up in an infinite loop that looks like this:

#106 0x023d2bc3 in nsScriptSecurityManager::CheckObjectAccess (cx=0xb5777800, 
    obj=0xafc25e40, id=-1303266756, mode=JSACC_READ, vp=0xbf88dbec)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:511
#107 0x002196a5 in js_InternalGetOrSet (cx=0xb5777800, obj=0xafc25e40, 
    id=-1303266756, fval=-1254013024, mode=JSACC_READ, argc=0, argv=0x0, 
    rval=0xbf88df54) at ../../../mozilla/js/src/jsinterp.cpp:1502
#108 0x0022a977 in js_GetSprop (cx=0xb5777800, sprop=0xaffb2830, 
    obj=0xafc25e40, vp=0xbf88df54) at ../../../mozilla/js/src/jsscope.h:365
#109 0x0022bc0a in js_NativeGet (cx=0xb5777800, obj=0xafc25e40, 
    pobj=0xb54146a0, sprop=0xaffb2830, vp=0xbf88df54)
    at ../../../mozilla/js/src/jsobj.cpp:4215
#110 0x0022dc9a in js_GetPropertyHelper (cx=0xb5777800, obj=0xafc25e40, 
    id=-1303266756, cacheResult=0, vp=0xbf88df54)
    at ../../../mozilla/js/src/jsobj.cpp:4381
#111 0x0022e2a6 in js_GetProperty (cx=0xb5777800, obj=0xafc25e40, 
    id=-1303266756, vp=0xbf88df54) at ../../../mozilla/js/src/jsobj.cpp:4391
#112 0x0018e237 in JS_GetProperty (cx=0xb5777800, obj=0xafc25e40, 
    name=0xb7af6ca8 "hostPort", vp=0xbf88df54)
    at ../../../mozilla/js/src/jsapi.cpp:3539
#113 0x0994569a in nsXPCWrappedJSClass::CallMethod (this=0xb5413580, 
    wrapper=0xafc29400, methodIndex=14, info=0xb7af6a78, 
    nativeParams=0xbf88e0d0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1645
#114 0x0993c62b in nsXPCWrappedJS::CallMethod (this=0xafc29400, 
    methodIndex=14, info=0xb7af6a78, params=0xbf88e0d0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:561
#115 0x00426d2e in PrepareAndDispatch (methodIndex=14, self=0xafc28100, 
    args=0xbf88e194)
    at ../../../../../../../mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95
#116 0x023d0d3b in GetPrincipalDomainOrigin (aPrincipal=0xafd7f560, 
    aOrigin=@0xbf88e5b4)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:147
#117 0x023d8e35 in nsScriptSecurityManager::CheckPropertyAccessImpl (
    this=0xb7a94100, aAction=1, aCallContext=0x0, cx=0xb5777800, 
    aJSObject=0xb54147a0, aObj=0x0, aTargetURI=0x0, aClassInfo=0x0, 
    aClassName=0x3257ae "Object", aProperty=-1303266756, 
    aCachedClassPolicy=0x0)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:813
#118 0x023d9282 in nsScriptSecurityManager::CheckPropertyAccess (
    this=0xb7a94100, cx=0xb5777800, aJSObject=0xb54147a0, 
    aClassName=0x3257ae "Object", aProperty=-1303266756, aAction=1)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:528
#119 0x023d2bc3 in nsScriptSecurityManager::CheckObjectAccess (cx=0xb5777800, 
    obj=0xafc25a40, id=-1303266756, mode=JSACC_READ, vp=0xbf88e78c)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:511

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, so the range in comment 9 looks like the right range for jprotocoltest://img/ but not jsprotocoltest://foo/.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Regression range for jsprotocoltest://foo/ is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=69c86f3acc5a&tochange=8eba35e62d92 as far as I can tell.

Sadly, nothing in that list jumps out at me (just like the first regression range).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Still digging into what might be going on here, but as a start...

We start off by doing an XPC_SJOW_Call from tabbrowser's shouldLoadFavIcon; presumably the schemeIs call.  Then we end up in the security check code, end up trying to get hostport from the URI, that throws, we try to get the spec, etc.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Still testing with jsprotocoltest://foo/.

So we end up inside the XPC_SJOW_Call from shouldLoadFavicon doing the schemeIs("http") call.  We have an SJOW there because the URI came from the document via browser.contentDocument.documentURIObject.  Ever since the fix for bug 339587, that hands back an SJOW instead of null.  That's a pretty recent change, though, so I'm still not sure what broke things back in November. 

In any case, we're in XPC_SJOW_Call.  The subject principal is system; the object principal is the nsPrincipal for the page we're loading (since that's what the safe object wrapper hands back as its principal).  We compile the wrapper func with that non-system principal, then call it.  That function tries to get call the actual method on the URI object, we go to do a security check, it doesn't have the system principal, while the URI object does, we end up getting URIs for the same-origin compares, that tries to call into the system URI object with the SJOW code still on the stack, we do a security check, etc.  At least that's my best guess.

Comment 15

[Boris Zbarsky [:bzbarsky]]

So there are at least a few things going on here that I think are bad:

1)  Having that SJOW around the JS-implemented URI is bad, because it means all
    attempts to call methods on the URI will fail, right?  Should there be some
    sort of same-origin check in the XPCNativeWrapper code before wrapping in
    SJOW?
2)  We should probably force a system subject principal (sounds scary) or set
    aside the frame chain (also scary, and how do we make sure we do it on the
    right cx?) while running GetPrincipalDomainOrigin.  This might not be an
    issue if we fix item 1, I suppose.
3)  I still don't know what caused the original bustage, so maybe there's more
    here.  :(

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, with jsprotocoltest://img/ the problem first appeared on m-c when bug 460811 landed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

And I have no idea why that caused the problem, so far....

Comment 18

[Boris Zbarsky [:bzbarsky]]

And in that case, there's no SJOW_Call involved, by the way.

Here's the relevant part of the stack for how we get into trouble on the image document:

#11 0x00adb483 in nsXPCWrappedJS::CallMethod (this=0x1c51f150, methodIndex=3, info=0x10507c8, params=0xbfffbd94) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:563
#12 0x00477cf6 in PrepareAndDispatch (self=0x1cb09790, methodIndex=3, args=0xbfffbeb4) at /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93
#13 0x00477d55 in nsXPTCStubBase::Stub3 (this=0x1cb09790) at xptcstubsdef.inc:1
#14 0x13490e65 in nsImageDocument::CreateSyntheticDocument (this=0x13f3e00) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/html/document/src/nsImageDocument.cpp:635
#15 0x134908a3 in nsImageDocument::SetScriptGlobalObject (this=0x13f3e00, aScriptGlobalObject=0x18d21fb0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/html/document/src/nsImageDocument.cpp:378
#16 0x135c3200 in nsGlobalWindow::SetNewDocument (this=0x1ca04650, aDocument=0x13f3e00, aState=0x0, aClearScopeHint=1, aIsInternalCall=0) at /Users/bzbarsky/mozilla/vanilla/mozilla/dom/src/base/nsGlobalWindow.cpp:1934

That's us calling GetSpec() on the document URI from the image document setup.  So pushing the cx of the window on the stack in SetNewDocument is what caused that to break.  Changing security checks to use a system subject principal under GetPrincipalDomainOrigin (item 2 of comment 15) would likely fix that issue....

Blake, I'd love your thoughts here.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, a better testcase is to point the foo/ URI to the following HTML document:

  <script>
  try { document.write(window.location.href);
  } catch (e) { document.write("threw"); }
  </script>

On Fx3 this throws.  On trunk, it sorta hangs or tries to put up some sort of infinite recursion alert, etc.

That behavior regressed between 2008-07-28-02 and 2008-27-29-02.  No rev ids in buildconfig there, but the time range is something like: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-07-28+00&enddate=2008-07-29+04

Which finally makes it clear what's going on.  That's the range bug 434522 landed in, and that patch makes us get the origin to report the error.  Which it the infinite loop jst is seeing in comment 10: we try to get the origin, do a security check for access to the nsIURI jsobject, it fails, we go to report the security check failure, get the origin, etc.

So we could pretty trivially not get the origin when reporting the error if the error is inside getting the origin.  Or we could force a system principal while getting the origin to report the error.  Both should fix the hangs, but we'll still have exceptions in the other cases mentioned above (exceptions that got introduced by the other bugs this one is blocking).  Maybe that's just the breaks; as the testcase described above shows, this has never behaved very well, nor can any chrome-principal js component that can be called into from C++ with content js on the stack behave well.  Don't implement such things in JS.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I do think this should block, for what it's worth.

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK, on further reflection, I think we should fix the recursion thing (will do tomorrow) and not worry about any of the rest of it.  As comment 19 says, implementing such an object in JS (at least without implementing nsISecurityCheckedComponent appropriately on it) is fundamentally broken; changing the XPCNW behavior or the global window cx push will just hide this in some cases, while others (like the testcase from comment 19) will still broken.

Comment 22

[Manish Singh]

So what is the appropriate nsISecurityCheckedComponent implementation for this? As an experiment I just granted "allAccess" for everything, it made no difference and still triggered the recursion.

Comment 23

[Boris Zbarsky [:bzbarsky]]

That's a really good question.  It's possible that we're doing the security checks when trying to call into the nsISecurityCheckedComponent functions too; in that case there may be no appropriate implementation.  If desired we can spin off a separate bug to sort out the desired and actual behavior of nsISecurityCheckedComponent here.  But that's more or less out of scope of this regression bug.

Comment 24

[Benjamin Smedberg]

I'm not sure I understand why URIs should implement nsISecurityCheckedComponent... content should never touch nsIURIs, right?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for the recursion

Benjamin, in this case C++ code is touching URIs while content JS is on the stack.  Then we get bitten by our "get the subject principal of the stack" setup.

As far as the regression range in comment 12 goes, the relevant change there is the fix for bug 471395, which made getting an XPCWrappedJS as a property off an XPCNativeWrapper return an SJOW for the XPCWrappedJS instead of just throwing.

I also looked into what the deal is with nsISecurityCheckedComponent not working as desired in this case, and it's basically bug 360207: when we go to do the security check on the XPCWrappedJS, the JS object is passed in to the security manager, but the nsISupports pointer is not, so it can't QI to nsISecurityCheckedComponent.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 378723 [details] [diff] [review]
Patch for the recursion

Is it worth making sInPrincipalDomainOrigin a static member of nsAutoInPrincipalDomainOriginSetter? Either way, r+sr=mrbkap.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Made that change and pushed:

http://hg.mozilla.org/mozilla-central/rev/eef18b3a9e8f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/287ba792e8ea

If someone has bright ideas on how to test this, I'm all ears!

Comment 28

[Mike Shaver (:shaver emeritus)]

Crashtest from the s3:// code?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Crashtest can't exactly install an extension (or even just a JS component), can it?

If mochitest can, that might work....

Comment 30

[Manish Singh]

jsprotocoltest://img/ still infinitely recurses even with the above change. It's got a different entrypoint than the text document (this is why I made two cases).

The mDocumentURI->GetSpec(src); in nsImageDocument::CreateSyntheticDocument() is what triggers this case.

Comment 31

[Manish Singh]

Attachment: Stack for jsprotocoltest://img/

Comment 32

[Boris Zbarsky [:bzbarsky]]

Oh, hm.  I was offline when testing, so couldn't test the image testcase, and forgot to retest it when I got back.  Fix coming up for that part too.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Applies on top of the other patch

Comment 34

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/5a34c5c12c37

Comment 35

[Manish Singh]

Cool, with that change the test case works, as well as s3://. Thanks!

Comment 36

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a483a69e3096