Closed Bug 997067 Opened 10 years ago Closed 10 years ago

Replace AutoPushJSContext in nsGlobalWindow::SecurityCheckURL

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bobowen, Assigned: bholley)

References

Details

Attachments

(2 files)

This has been broken out of bug 988383, to that it can be handled separately.

(In reply to Bobby Holley (:bholley) from bug 988383 comment #17)
> Comment on attachment 8398482 [details] [diff] [review]
> Part 5: Replace AutoPushJSContext in nsGlobalWindow::SecurityCheckURL v1
> 
> Review of attachment 8398482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is really sensitive code - in fact, this was the function that nailed
> us in pwn2own this year - see bug 982906. So we should tread carefully here.

I can't see that bug, would you CC me. :)

> In particular, I don't think this patch is right, because the push will
> affect the subject principal, which is what
> nsScriptSecurityManager::CheckLoadURIFromScript is based on. Can you file
> this as a separate bug and CC me and Boris? We'll try to figure out what
> this function actually needs to be doing.

Do you mean that the original code is wrong?
This is just a typedef of the original function, although it would need to change because of bug 989528.
Setting S-S flag just in case.
CCed you.

I'm setting this to sec-audit, but please change that or remove the sec-audit if there's a real problem, thanks.
Keywords: sec-audit
(In reply to Bob Owen (:bobowen) from comment #0)
> Do you mean that the original code is wrong?
> This is just a typedef of the original function, although it would need to
> change because of bug 989528.

Well, I'm saying that the analysis of "for error reporting" is incorrect here, because the push actually affects the subject principal of the code that invokes nsScriptSecurityManager::CheckLoadURIFromScript.

The code at present is correct though, so I don't think this is s-s.
Group: core-security
Keywords: sec-audit
I can clean this up.
Assignee: nobody → bobbyholley
This has a tiny behavior change in the case of self-navigation by a non-current
inner, but I think that the new behavior is more correct.
Attachment #8407672 - Flags: review?(bzbarsky)
Considering what is actually done with sourceWindow here, I don't think this
check is doing anything useful in the modern world.
Attachment #8407674 - Flags: review?(bzbarsky)
Comment on attachment 8407672 [details] [diff] [review]
Part 1 - Build BuildURIFromBase into SecurityCheckURL and condense logic. v1

Does this not change behavior in the !GetContextInternal() (so presumably closed?) case?  We used to throw and now we'll press on?

Please keep the comments about matching windowwatcher.

The non-current inner change is that we used to get the outer as the sourceWindow but now will get the inner?  But does that match windowwatcher behavior?  We _really_ want to make sure we're calling checkLoadURI on the actual URI we'll load.

r+ with those issues sorted out.
Attachment #8407672 - Flags: review?(bzbarsky) → review+
Comment on attachment 8407674 [details] [diff] [review]
Part 2 - Remove the special behavior for chrome navigating non-chrome windows. v1

Again, does this match what we'll do later to construct the actual URI we plan to load?
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] from comment #7)
> Does this not change behavior in the !GetContextInternal() (so presumably
> closed?) case?  We used to throw and now we'll press on?

I'm pretty sure that OpenInternal will bail out if we've been torn down, because mDocShell will be null, so GetWebBrowserChrome() will return null, and we'll bail out, right?
 
> Please keep the comments about matching windowwatcher.

Whoops, fixed.

> The non-current inner change is that we used to get the outer as the
> sourceWindow but now will get the inner?

Actually, this isn't true, because |this| is always an outer window here. OpenInternal does FORWARD_TO_OUTER at the top. So we're all good here.

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8407674 [details] [diff] [review]
> Again, does this match what we'll do later to construct the actual URI we
> plan to load?

The algorithm is here: http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#1339

IIUC, it matches the new behavior.
Flags: needinfo?(bobbyholley)
> and we'll bail out, right?

OK, good.

> Actually, this isn't true, because |this| is always an outer window here.

I was talking about code that used to do this:

  sourceWindow = do_QueryInterface(scriptcx->GetGlobalObject());

which will return an outer.  But now we do:

  sourceWindow = do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(topCx))

which I guess still returns an outer.  OK, so no change.  What _is_ the change then?

> IIUC, it matches the new behavior.

I agree, though it takes some unpacking to do that.  It might be nice to use GetDynamicScriptGlobal in window watcher too, just to make it abundantly clear that the same thing is happening.
Comment on attachment 8407674 [details] [diff] [review]
Part 2 - Remove the special behavior for chrome navigating non-chrome windows. v1

r=me, I think.  This part worries me.  :(
Attachment #8407674 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #10)
> > Actually, this isn't true, because |this| is always an outer window here.
> which I guess still returns an outer.  OK, so no change.  What _is_ the
> change then?

There is no change. The "Actually" was self-directed.
https://hg.mozilla.org/mozilla-central/rev/d9c61e48f276
https://hg.mozilla.org/mozilla-central/rev/4bb76e736c87
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1092388
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.