Closed
Bug 997067
Opened 11 years ago
Closed 10 years ago
Replace AutoPushJSContext in nsGlobalWindow::SecurityCheckURL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bobowen, Assigned: bholley)
References
Details
Attachments
(2 files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
(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
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fa30e1ce34c2
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
(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)
Comment 10•11 years ago
|
||
> 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 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9c61e48f276 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb76e736c87
Comment 14•10 years ago
|
||
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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•