Closed Bug 298064 Opened 17 years ago Closed 17 years ago

nsContentUtils::GetDocumentFromCaller() is broken.

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta4

People

(Reporter: jst, Assigned: peterv)

References

Details

(Keywords: verified1.8.0.9, verified1.8.1.1, Whiteboard: [sg:low])

Attachments

(3 files, 6 obsolete files)

Right now nsContentUtils::GetDocumentFromCaller() is broken, it now gets the
document from the calling context, which may or may not be what you want, what
you most likely always want is the document where the calling code came from.

We use the caller document in document.open() etc to propagate security info
etc, and that really should be that of the calling code, not the calling context.
Blocking beta, but not branch.
Flags: blocking1.8b4+
Peter - can you take a look?
Assignee: jst → peterv
Attached patch v1 (obsolete) — Splinter Review
This seems to do the trick (thanks to jst for setting me on the right track).
Brendan: are we doing the right thing here by using
JS_GetScopeChain/JS_GetParent, it does work but is it correct?
Jst: shouldn't the code to get callerPrincipal be moved out of the if at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855
?
Attachment #193916 - Flags: superreview?(brendan)
Attachment #193916 - Flags: review?(jst)
(In reply to comment #4)
> Created an attachment (id=193916) [edit]
> v1
> 
> This seems to do the trick (thanks to jst for setting me on the right track).
> Brendan: are we doing the right thing here by using
> JS_GetScopeChain/JS_GetParent, it does work but is it correct?
> Jst: shouldn't the code to get callerPrincipal be moved out of the if at
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855
> ?

Yeah, seems like it should be. Wanna do a double whammy and include that in this
fix as well? :)
(In reply to comment #5)
> (In reply to comment #4)
> > Jst: shouldn't the code to get callerPrincipal be moved out of the if at
> >
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855
> > ?
> 
> Yeah, seems like it should be. Wanna do a double whammy and include that in this
> fix as well? :)

Sure, I'll try to attach a new patch with that included later tonight.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta4
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #193916 - Attachment is obsolete: true
Attachment #193935 - Flags: superreview?(brendan)
Attachment #193935 - Flags: review?(jst)
Attachment #193916 - Flags: superreview?(brendan)
Attachment #193916 - Flags: review?(jst)
Attached file Testcase file B (obsolete) —
Attached file Testcase file A (obsolete) —
Comment on attachment 193935 [details] [diff] [review]
v1.1

Ok, you don't want the scope chain, if we are willing to break compatibility
with Gecko here in the interest of sanity and (perhaps, need to test; also
likely irrelevant) Netscape 2-4.

You want to change GetDocumentFromCaller to take a JSObject *obj argument that
it uses as the start of the loop up the parent chain to a global object.  This
obj param should be the Image constructor, e.g., being called.

That is, var img = new Image will work as with your patch, but var img2 = new
otherWindow.Image will work as it should, not as it does today (which I believe
involves using the wrong document's nodeinfo list -- please correct me if I am
wrong).

Need a third testcase covering the img2 case and somehow detects the difference
in results, and some results from various browsers.

/be
Attachment #193935 - Flags: superreview?(brendan) → superreview-
(In reply to comment #10)
> (From update of attachment 193935 [details] [diff] [review] [edit])
> Ok, you don't want the scope chain, if we are willing to break compatibility
> with Gecko here in the interest of sanity and (perhaps, need to test; also
> likely irrelevant) Netscape 2-4.

Of course, your patch to use the scope chain is incompatible with the broken,
longstanding Gecko implementation anyway.

Still, it'd be good to have a testcase and results from various browsers.  We
may choose to break compat here because no one will care.  I'm keen to see how
you'd detect the difference in which document's nodeinfo list was used.

/be
So I either need to walk the JS stack until I find a function or a constructor
and use the function object or the constructor object, or I need to pass those
objects on from where I can reach them.
The first solution works if I add a JS_GetFrameConstructorObject to
jsdbgapi.h/cpp which does |return fp->argv && (fp->flags & JSFRAME_CONSTRUCTING)
!= 0 ? JSVAL_TO_OBJECT(fp->argv[-2]) : NULL;|. The other solution works too, I
can get at argv[-2] from nsHTMLImageElement::Initialize and
nsHTMLOptionElement::Initialize, but it means that we have to create elements
without a nodeinfo (Initialize gets called after creation). I'd rather not do
that (and avoid the "No nsINodeInfo passed to nsGenericElement, PREPARE TO
CRASH!!!" assertion), so I'm leaning towards the first solution. I need to know
whether adding JS_GetFrameConstructorObject would be ok though.
(In reply to comment #12)
> I need to know
> whether adding JS_GetFrameConstructorObject would be ok though.

I don't see the need, given JS_GetFrameFunctionObject -- why not use just that,
or condition it with JS_IsConstructing?

/be
Really, it doesn't matter whether the JS callee is a constructor or not.  If you
are hacking a native method reached from a new Image or Image() call or
whatever, and it's creating a new Image object, you want it to use static scoping.

/be
(In reply to comment #13)
> I don't see the need, given JS_GetFrameFunctionObject -- why not use just that,
> or condition it with JS_IsConstructing?

Because JS_GetFrameFunctionObject checks |fp->argv && fp->fun| and when coming
through a constructor (like from new Image()) fp->fun is null.
Attached file Testcase file B
Attachment #193936 - Attachment is obsolete: true
Attached file Testcase file A (obsolete) —
IE 6, Opera 8 and Safari 2.0 all pass this one, and with my patch (as described
in comment 12) Firefox does too. It's not trivial to test this in older
browsers, I guess it could be done but do we really care given these results?
Attachment #193937 - Attachment is obsolete: true
Attached file Testcase file A
Attachment #194171 - Attachment is obsolete: true
(In reply to comment #15)
> (In reply to comment #13)
> > I don't see the need, given JS_GetFrameFunctionObject -- why not use just that,
> > or condition it with JS_IsConstructing?
> 
> Because JS_GetFrameFunctionObject checks |fp->argv && fp->fun| and when coming
> through a constructor (like from new Image()) fp->fun is null.

Oh duh -- XPConnect makes its own callable objects.  Ok, all we really need here
is fp->argv[-2], with safety null-checking for fp->argv.  Don't conflate it with
JS_IsConstructing, call it JS_GetFrameCalleeObject and add it to the patch?

/be
Attached patch v2 (obsolete) — Splinter Review
Attachment #193935 - Attachment is obsolete: true
Attachment #194235 - Flags: superreview?(brendan)
Attachment #193935 - Flags: review?(jst)
Comment on attachment 194235 [details] [diff] [review]
v2

Thanks, looks good, only thought:

Isn't there a subroutine to get the global object by walking up the parent
chain?	Maybe jst knows.

/be
Attachment #194235 - Flags: superreview?(brendan)
Attachment #194235 - Flags: superreview+
Attachment #194235 - Flags: review?(jst)
(In reply to comment #21)
> Isn't there a subroutine to get the global object by walking up the parent
> chain?	Maybe jst knows.

There's nsJSUtils::GetStaticScriptGlobal, it would add one QI (it returns
nsIScriptGlobalObject and we want nsPIDOMWindow). Jst?
Comment on attachment 194235 [details] [diff] [review]
v2

Yeah, use nsJSUtils here, one more QI won't hurt much. r=jst
Attachment #194235 - Flags: review?(jst) → review+
Attached patch v2.1Splinter Review
Here's the patch that I checked in on the trunk. Carrying over r=jst,
sr=brendan and asking for branch approval.
Attachment #194235 - Attachment is obsolete: true
Attachment #194685 - Flags: superreview+
Attachment #194685 - Flags: review+
Attachment #194685 - Flags: approval1.8b4?
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I'll verify this with tomorrow's builds.
(In reply to comment #18)
> Created an attachment (id=194172) [edit]
> Testcase file A
> 

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050903 Firefox/1.6a1

I'll try to verify on Linux and Mac later.
Attachment #194685 - Flags: approval1.8b4? → approval1.8b4+
Keywords: fixed1.8
This appears to have caused bug 312363, showing mixed content if a child frame has been document.written() into as seen in a real-world bank site.
The nsContentUtils.cpp changes that went in with this fix was backed out on the branch. See bug 312363. (removing fixed1.8 keyword)
Keywords: fixed1.8
Should this bug be reopened, and used for patching to fix the two callers (for new Image and new Option from JS) that need fixing?  Bug 312363 is about document.open and it is fixed.

/be
Brendan, this patch is still in on the trunk. I had the intention of leaving things that way and simply fixing document.open on the trunk (I'm about to attach a new patch to that bug).
Flags: testcase+
fyi, failed ff 1.0.8/winxp/20060221
This is not fixed in bon echo 1.8 20060910/winxp. 
Flags: blocking1.8.1?
(In reply to comment #32)
> This is not fixed in bon echo 1.8 20060910/winxp. 

Yeah, the patch was backed out from the 1.8 branch (see comment 28). If we want this fixed on the trunk, the trunk patch in bug 312363 should be nominated/checked into the 1.8 branch.
Bkap/Brendan - what do we need to do here?
(In reply to comment #33)
> (In reply to comment #32)
> > This is not fixed in bon echo 1.8 20060910/winxp. 
> 
> Yeah, the patch was backed out from the 1.8 branch (see comment 28). If we want
> this fixed on the trunk,

I think Blake meant "branch" here.

> the trunk patch in bug 312363 should be
> nominated/checked into the 1.8 branch.

That sounds like the thing to do for 1.8.  Blake, can you confirm?

/be
(In reply to comment #35)
> That sounds like the thing to do for 1.8.  Blake, can you confirm?

Yeah, that's what I meant.
Could you nominate the appropriate patch?  It's marked fixed1.8.
(In reply to comment #37)
> Could you nominate the appropriate patch?  It's marked fixed1.8.

It's marked fixed1.8 because we backed out the offending patch from the branch. I'll nominate the correct patch now (checking to make sure that it applies correctly first).
Plusing the blocking flag on this so we don't lose it.  Sounds like we want the patch in 312363...
Flags: blocking1.8.1? → blocking1.8.1+
Moving this out to 1.8.1.1 since we can't seem to get a straight answer on what the right approach here is...
Flags: blocking1.8.1.1?
Flags: blocking1.8.1-
Flags: blocking1.8.1+
mrbkap: sounds like we want this one, or some kind of fix, but what patch? Please ask for approval on what we need.

Would we want this for 1.8.0.9 too?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9? → blocking1.8.0.9+
Removing blocking flags, peterv wants us to take the fix in bug 312363.  I will update that bug with the flags.
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Fixed on branches by the check-in of attachment 238242 [details] [diff] [review] from bug 312363.
Verified no crash using testcase on comment #18 with the following:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061128 Minefield/3.0a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre
and 1.8.0.9 on Intelmac, 1.8.1.1 linux 
Status: RESOLVED → VERIFIED
Whiteboard: [sg:fix] → [sg:low]
Group: security
Flags: in-testsuite+ → in-testsuite?
I checked in a test for this in bug 445004.
Flags: in-testsuite? → in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.