Closed Bug 1210439 Opened 4 years ago Closed 4 years ago

It looks like swapFrameLoaders might result in docshell allow* flags being reset

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox43 + fixed
firefox44 + fixed

People

(Reporter: Gijs, Assigned: Gijs)

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main43-])

Attachments

(2 files)

See patch in bug 1195735. I have to re-set the allowJavascript flags there because otherwise I'm seeing JS still being executed.

It's possible there is a different cause here (like the frames/browsers being in a panel that gets hidden or somesuch) but it seems like we should investigate this.
I was first thinking this would be a regression from bug 840488, but maybe not.
If I'm reading the code correctly: when we swap we remove docshell and add it to be a child of some
other (chrome) docshell, so nsDocShell::SetDocLoaderParent gets called, and that resets various flags based on the state of the parent.
Since all the allow* flags are true by default, I think nsDocShell::SetDocLoaderParent shouldn't
override the value if it is false already.
This testcase reproduces the problem. I'll look at the fix you just described.
Attached patch ,Splinter Review
I'm not really sure about isActive and the private browsing stuff - I guess it makes sense to inherit that? Maybe? Dunno. :-\
Attachment #8668539 - Flags: review?(bugs)
Comment on attachment 8668539 [details] [diff] [review]
,


>   nsCOMPtr<nsIDocShell> parentAsDocShell(do_QueryInterface(parent));
>   if (parentAsDocShell) {
>-    if (NS_SUCCEEDED(parentAsDocShell->GetAllowPlugins(&value))) {
>+    if (NS_SUCCEEDED(parentAsDocShell->GetAllowPlugins(&value)) && mAllowPlugins) {
I would just check mAllowPlugins and then call the method on parent. Same with others.




>     if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
>       SetIsActive(value);
>     }
And yes, this is different thing. Not about anything being allowed

>     value = parentAsDocShell->GetAffectPrivateSessionLifetime();
>     SetAffectPrivateSessionLifetime(value);
This is a bit odd flag, but looks like it is set to false only on top level xul stuff and then inherited from there. So I don't see reason to change, at least not in practice.
Attachment #8668539 - Flags: review?(bugs) → review+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
No sec-severity here... to the best of my knowledge this doesn't affect anything using allowJavascript in-tree apart from the panel thing (and that only in my patch that just landed, and I worked around it in there). I haven't checked for any of the other flags.

I tried to check add-ons. There's a surprising number of uses of allowJavascript, but not a lot of swapFrameLoaders, that I could find - but it's hard because of how we used to ship the SDK with add-on SDK add-ons, which now all have the utils.js file that has this thing...

I *think* this is safe to just land and uplift to aurora/beta (assuming green try and no issues with landing), but I'd like to be slightly more sure before I go ahead. Bobby/Dan, can you chime in?
Flags: needinfo?(dveditz)
Flags: needinfo?(bobbyholley)
Redirecting to smaug.
Flags: needinfo?(bobbyholley) → needinfo?(bugs)
having scripting enabled when some addon has disabled it isn't a security bug itself, right?
Landing to m-c and aurora should be fine, but I wonder if some addon actually has hacks to fix this in its code. This might break such addon?
Bug 1195735 is fixed without this, right? So is there any reason to land this to beta?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #7)
> having scripting enabled when some addon has disabled it isn't a security
> bug itself, right?

"likely yes, but...". Depends what it's disabled on...

Generally, when there are cases where JS runs when we don't expect it to lead to sec bugs one way or another (e.g. bug 1126352 / bug 1182778).

> Landing to m-c and aurora should be fine, but I wonder if some addon
> actually has hacks to fix this in its code. This might break such addon?

To fix this, presumably they would save values before swapFrameLoaders and then re-write them afterwards? I don't expect this change to break such code.

> Bug 1195735 is fixed without this, right?

Yes.

> So is there any reason to land this to beta?

Not unless we are worried this bug is breaking security assumptions, which I'm still not sure about either way. I don't have enough experience to work out which side to be erring on ("caution" being perhaps too simplistic).
(In reply to Olli Pettay [:smaug] from comment #7)
> having scripting enabled when some addon has disabled it isn't a security
> bug itself, right?

If some code has gone to the trouble of disabling JS they are almost certainly relying on it: at best it's lack of confidence in their sanitization, at worst there is zero sanitization and this is the only thing preventing a chrome XSS.

(In reply to :Gijs Kruitbosch from comment #5)
> I *think* this is safe to just land and uplift to aurora/beta (assuming
> green try and no issues with landing), but I'd like to be slightly more sure
> before I go ahead. Bobby/Dan, can you chime in?

Since the known security problem is fixed elsewhere we don't need to push this to beta. Aurora is fine, that gives us some bake-time to make sure this doesn't accidentally break add-ons.
Flags: needinfo?(dveditz)
Group: core-security → dom-core-security
https://hg.mozilla.org/mozilla-central/rev/d0377efe20f4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8668539 [details] [diff] [review]
,

Approval Request Comment
[Feature/regressing bug #]: n/a, sec bug
[User impact if declined]: potential for security issues affecting add-ons
[Describe test coverage new/current, TreeHerder]: this is pretty core code, so it has "don't make me completely silly broken" coverage in treeherder via a number of tests that move tabs between windows etc. I didn't write or land a specific regression test for the functionality here. I am not planning to do so at this time.
[Risks and why]: low risk due to the test coverage, the fact that this is just aurora, and I'd like to get this uplifted so that we move the security fix out a bit, but not too much - there is a tiny potential of add-on breakage, but we likely won't find out about that until it hits aurora or beta.
[String/UUID change made/needed]: nope
Attachment #8668539 - Flags: approval-mozilla-aurora?
Group: dom-core-security → core-security-release
Comment on attachment 8668539 [details] [diff] [review]
,

Diagnostic patch;  ok to uplift to aurora.
Attachment #8668539 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking this to where we're landing it, since it's a sec bug and if it reveals (or causes) any issues we should follow up.
Based on comment 10, this should be fixed for FF.
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main43-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.