Closed Bug 1218594 Opened 4 years ago Closed 4 years ago

[e10s] Address bar can be hidden in popup windows, if url param is empty string

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
e10s m8+ ---
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed
firefox46 --- fixed

People

(Reporter: Felipe, Assigned: mconley)

References

Details

(Keywords: csectype-spoof, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main43-])

Attachments

(6 files, 6 obsolete files)

100.83 KB, image/png
Details
723 bytes, text/html
Details
10.10 KB, patch
mconley
: review+
Details | Diff | Splinter Review
10.07 KB, patch
mconley
: review+
Details | Diff | Splinter Review
1.72 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
3.83 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
Found a testcase for bug 1196706. Filing as a separate bug because of the testcase.

If the URL param to window.open is an empty string, then the features string is able to pass location=no, etc to the new window.  It's possible to create a window with a toolbar but no urlbar (i.e., only the bookmarks button, searchbar, etc., see screenshot), or no toolbar at all.
Attached file testcase.htm
Is this still e10s-only?
Flags: needinfo?(felipc)
Yep
Flags: needinfo?(felipc)
Well, um, this is scary. Because in case you haven't noticed, it also isn't opening the right window at all.

I just debugged this. We get everything right in the content process, then we chuck stuff to the parent process, do the same bunch of checks, and fail miserably.

It seems this is because we pass |true| for aChromeURL to CalculateChromeFlags. Which is because we compute a relative URL with the "" here:

https://dxr.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69/embedding/components/windowwatcher/nsWindowWatcher.cpp#1434-1466

... and use chrome://browser/content/browser.xul to do that relative URL.

We really really should not be doing that. Mike, you touched some of this even more recently than me. It seems to me like we should ensure we get the final URI to open from the content process and never compute that URL in the parent one in the first place. Thoughts?
Flags: needinfo?(mconley)
Looking.
Flags: needinfo?(mconley)
Assignee: nobody → mconley
Cc'ing smaug, who will probably end up reviewing whatever fix I create here.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> Created attachment 8679533 [details] [diff] [review]
> Fix

Does this actually fix things even with relative links like passing "foo.html" ?
So the problem is that OpenWindow accepts a URL string as one of its arguments, but that URL string can also be null.

This is fine, except that IPDL does not allow us to express something like this over an IPC protocol. The string type, when sent over IPC, cannot be null[1].

So if a null URL comes in, TabChild::ProvideWindow ends up passing the empty string up to the parent, since it can't send the null.

In the parent, OpenWindowInternal checks for the presence of aUrl, and if it finds it, tries to construct an absolute URL based on the DOM window that is trying to open the window (in the e10s case, that's the browser window itself).

I think we had run into something similar with the features string - we check its length, and pass nullptr into OpenWindow from Tabparent if the features string has 0 length. I've done the same thing for the URL.

(In reply to :Gijs Kruitbosch (at OSCON, limited availability) from comment #8)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> > Created attachment 8679533 [details] [diff] [review]
> > Fix
> 
> Does this actually fix things even with relative links like passing
> "foo.html" ?

TabParent does the work of assembling an absolute URL, since the TabChild passes up the base URL of the requesting DOM window.

However, I've modified my patch to ensure that we're not entering that block if we're opening a window requested from content.


[1]: Unlike actors, which can be nullable. See bug 524220.
Comment on attachment 8679547 [details] [diff] [review]
Fix

How's my thinking, smaug?
Attachment #8679547 - Flags: review?(bugs)
So this is my first security bug where I can probably write a regression test quite easily.

The thing is, a regression test makes it pretty obvious what the security bug is, and how to reproduce it.

What do we do in this case? A regression test certainly feels like the right thing to do...

felipe, what do you think?
Flags: needinfo?(felipc)
Normally you write the test in a separate patch and the test gets landed after the fix ships to release. You can request sec-approval at that point. If we end up fixing this in a release before e10s is on by default we probably don't need to worry about hiding this though as I believe it's e10s-only.
Thanks!
Flags: needinfo?(felipc)
Comment on attachment 8679547 [details] [diff] [review]
Fix

To be more consistent with non-e10s, I think we should try to pass the 
'null'-ness of uri, features and name to parent side.

So, in nsWindowWatcher::OpenWindowInternal
if (!aName) {
  name.SetIsVoid(true);
}
Same for features.

And TabChild::ProvideWindowCommon shouldn't convert null uri to empty string.
It should mark the url void, if aURI is null.
Attachment #8679547 - Flags: review?(bugs)
WindowWatcher is very fragile code, having magical differences with empty vs null handling, so no
wonder we missed this :(
(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8679547 [details] [diff] [review]
> Fix
> 
> To be more consistent with non-e10s, I think we should try to pass the 
> 'null'-ness of uri, features and name to parent side.
> 
> So, in nsWindowWatcher::OpenWindowInternal
> if (!aName) {
>   name.SetIsVoid(true);
> }
> Same for features.
> 
> And TabChild::ProvideWindowCommon shouldn't convert null uri to empty string.
> It should mark the url void, if aURI is null.

So, just so I understand, we want to use the Void state of strings (which is presumably serialized properly over IPC), and then in TabParent, check for the void-ness to determine whether or not the pass nullptr to OpenWindow?
Flags: needinfo?(bugs)
Comment on attachment 8679606 [details] [diff] [review]
Fix v3

Something like this?
Attachment #8679606 - Flags: review?(bugs)
Comment on attachment 8679606 [details] [diff] [review]
Fix v3

>   NS_ConvertUTF8toUTF16 url(spec);
>   nsString name(aName);
>   nsAutoCString features(aFeatures);
>+  if (!features.Length()) {
>+    features.SetIsVoid(true);
>+  }
We don't want to make empty->void or void->empty conversions ever.
If you get void from the caller, just keep it.
I'm suggesting window watcher would set the string void.
Especially in case of features empty != void.
Flags: needinfo?(bugs)
Attachment #8679606 - Attachment is obsolete: true
Attachment #8679606 - Flags: review?(bugs)
Comment on attachment 8679640 [details] [diff] [review]
Fix v4

Review of attachment 8679640 [details] [diff] [review]:
-----------------------------------------------------------------

> I'm suggesting window watcher would set the string void.

I think I'm still not understanding how I should approach this. :/ Where should nsWindowWatcher set the string void? If you mean in the child before it calls ProvideWindow, the nsWindowWatcher never handles a string - it's passing an nsIURI*.

What I've done here is set the string to void if we detect that nullptr had been passed in for the nsIURI* in TabChild::ProvideWindowCommon, and then pass along a nullptr if we detect that case in TabParent::RecvCreateWindow.

You're right that I shouldn't have modified aFeatures - I thought it'd be more consistent, but an empty string does mean something completely different for the features case.
Attachment #8679640 - Flags: review?(bugs)
nsWindowWatcher passes features nsAutoCString to ProvideWindow, and also name.
I think both those should be marked as void when needed.
Comment on attachment 8679640 [details] [diff] [review]
Fix v4

In other words, I'm hoping we fix the other inconsistency too while we're fixing these issues.
Attachment #8679640 - Flags: review?(bugs) → review-
We were losing the void-ness in the needless flip-flopping between UTF-16 and UTF-8, so I've changed the IPDL to accept nsCStrings instead of nsStrings for the URI strings.
Comment on attachment 8680051 [details] [diff] [review]
Fix v5

This makes it so that when nsWindowWatcher sees null URI's and feature strings coming in, it sets its internal string representations to void.
Attachment #8680051 - Flags: review?(bugs)
Comment on attachment 8680051 [details] [diff] [review]
Fix v5

We should still fix SendBrowserFrameOpenWindow. Want to file a followup?
(I think SendBrowserFrameOpenWindow should get also null values if null is passed to TabChild::ProvideWindowCommon).
And/or file a followup to fix NS_ConvertUTF8toUTF16 and NS_ConvertUTF16toUTF8 so that it doesn't drop voidness.


Btw, in some cases initializing to NullString() by default might make the code simpler.
Attachment #8680051 - Flags: review?(bugs) → review+
Attachment #8680734 - Attachment description: Fix rebased → Fix rebased (r+'d by smaug)
Attachment #8680734 - Flags: review+
This vulnerability can affect any user running with e10s. Liz, do we want to try uplifting this fix? And if so, how far?
Flags: needinfo?(lhenry)
Let's talk with Sylvestre about uplifting to 44 aurora. We could maybe take it in early beta 43 as well. It sounds like we should if we're going to run e10s experiments on beta.
Flags: needinfo?(lhenry)
Can you nominate it with the uplift request form and all that?  Should be fine for aurora, we just want to understand what your assessment of the riskiness of the fix is.
Flags: needinfo?(mconley)
Note, also please ask for sec approval.
Flags: needinfo?(abillings)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #36)
> Note, also please ask for sec approval.

I don't think sec-moderate needs sec approval. ( https://wiki.mozilla.org/Security/Bug_Approval_Process#Process, point (A))

Also, considering this only affects e10s, I don't think the impact is that high, especially if we uplift to aurora quickly. AIUI we still can't enable e10s in beta at all, so there ends the impact of this bug, right?
Sec-moderate doesn't need sec-approval.
Flags: needinfo?(abillings)
(In reply to :Gijs Kruitbosch from comment #37)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #36)
> > Note, also please ask for sec approval.
> 
> I don't think sec-moderate needs sec approval. (
> https://wiki.mozilla.org/Security/Bug_Approval_Process#Process, point (A))
> 
> Also, considering this only affects e10s, I don't think the impact is that
> high, especially if we uplift to aurora quickly. AIUI we still can't enable
> e10s in beta at all, so there ends the impact of this bug, right?

The uplift happened yesterday, so the e10s code we've been testing on Dev Edition has just moved to the beta repository. I believe there are tentative plans to flip on e10s for a small window of time there OR to offer e10s for opt-in for at least some of the population. So I agree that uplifting this to 43 beta makes sense.
Flags: needinfo?(mconley)
Gahhhh, bitrotted by the ServiceWorker openWindow stuff. :/

Working up another rebased patch...
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> Gahhhh, bitrotted by the ServiceWorker openWindow stuff. :/
> 
> Working up another rebased patch...

Err, this landed already?

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=4b8c70ee14ed
Flags: needinfo?(mconley)
Huh. Is it typical to not update security bugs when things land? I'd wager not. :/

Hey TomCat, I noticed you landed this - thanks! I just noticed that bug 1173870 just landed in mozilla-inbound which touches the same region of code. It's possible this is going to conflict like hell when you attempt to merge the two integration branches.

It's also possible that Mercurial will magically figure everything out, but I'm not holding my breath.

I'm happy to rebase a new patch on top of the patches from bug 1173870, so if it comes down to it, I guess feel free to back out this patch before the merge?

How do you want to proceed?
Flags: needinfo?(mconley) → needinfo?(cbook)
Keywords: checkin-needed
Sorry, the bug that's bitrotted me is bug 1172870.
I just talked with cbook over IRC.

We're going to back this out, and I'll write a new patch. The one I had originally written can be used for the Aurora and Beta uplifts (assuming I get approval).
Flags: needinfo?(cbook)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #44)
> I just talked with cbook over IRC.
> 
> We're going to back this out, and I'll write a new patch. The one I had
> originally written can be used for the Aurora and Beta uplifts (assuming I
> get approval).

the backout is https://hg.mozilla.org/integration/fx-team/rev/0b7dbe691d1c
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #43)
> Sorry, the bug that's bitrotted me is bug 1172870.

Sorry. And Bug 1191724 may cause some issues too.
s/issues/need for merge/
Ah, forgot about it being sec-moderate. Adding tracking flags as well since this is a crash and sec issue.   
I haven't started the build yet for 43 beta 1.  We probably want to make sure this lands before starting the build.
Flags: needinfo?(sledru)
Mike, sorry, what patch is it exactly that still needs to land, or needs approval for aurora/beta uplift?  I think this is the last blocker before our early build of beta 1.  

I am just about to get on a plane. Can you talk with either Sylvestre or Lawrence to clear this up?
Flags: needinfo?(mconley)
Hey lizzard,

attachment 8680734 [details] [diff] [review] should be good to land on Aurora and Beta (though would you rather the rebased version has time to bake on central before uplifting?)
Flags: needinfo?(mconley) → needinfo?(lhenry)
Comment on attachment 8680734 [details] [diff] [review]
Fix rebased for aurora (r+'d by smaug)

Approval Request Comment
[Feature/regressing bug #]:

Not sure when this was introduced - but probably bug 989501, if I had to guess (so it's been around for as long as we've had window opening from content working with e10s)

[User impact if declined]:

Users can have their location bar hidden in popup windows which content can open and control.

[Describe test coverage new/current, TreeHerder]:

We have test coverage for window opening that still passes with this patch. This patch hasn't yet landed on inbound or central due to a merge conflict, which I'm currently writing a new patch against.

[Risks and why]: 

I think this is pretty low risk. We're just being more careful about distinguishing between a nullptr URL char*, and an empty-string URL char*.

[String/UUID change made/needed]:

None.
Attachment #8680734 - Flags: approval-mozilla-beta?
Attachment #8680734 - Flags: approval-mozilla-aurora?
Attachment #8680734 - Attachment description: Fix rebased (r+'d by smaug) → Fix rebased for aurora / beta (r+'d by smaug)
Attachment #8680734 - Attachment is obsolete: true
Attachment #8680734 - Flags: approval-mozilla-beta?
Attachment #8680734 - Flags: approval-mozilla-aurora?
Comment on attachment 8680734 [details] [diff] [review]
Fix rebased for aurora (r+'d by smaug)

Bah, silly bzexport.

See comment 51.
Attachment #8680734 - Attachment is obsolete: false
Attachment #8680734 - Flags: approval-mozilla-beta?
Attachment #8680734 - Flags: approval-mozilla-aurora?
Ah, re-reading lizzard's message, she's on a plane. I'll follow up with sledru or lmandel.
Flags: needinfo?(lhenry)
Comment on attachment 8680734 [details] [diff] [review]
Fix rebased for aurora (r+'d by smaug)

Should be in the first beta of 43. Thanks
Flags: needinfo?(sledru)
Attachment #8680734 - Flags: approval-mozilla-beta?
Attachment #8680734 - Flags: approval-mozilla-beta+
Attachment #8680734 - Flags: approval-mozilla-aurora?
Attachment #8680734 - Flags: approval-mozilla-aurora+
Attachment #8680734 - Attachment description: Fix rebased for aurora / beta (r+'d by smaug) → Fix rebased for aurora (r+'d by smaug)
Attachment #8680734 - Attachment is obsolete: false
Comment on attachment 8681656 [details] [diff] [review]
Rebased for Beta (r=smaug,a=sledru)

Had to rebase for beta.
Attachment #8681656 - Attachment description: Rebased for Beta → Rebased for Beta (r=smaug,a=sledru)
Attachment #8681656 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2bfbc962e6c3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #59)
> Landed on aurora:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/7063e8c719a2
> 
> Beta is closed, so adding checkin-needed for attachment 8681656 [details] [diff] [review]
> [diff] [review] when it re-opens.

adjusting the flags
Group: firefox-core-security → core-security-release
Filed bug 1227296 for NS_ConvertUTF8toUTF16 and NS_ConvertUTF16toUTF8 not preserving voidness, as per comment 31.
Whiteboard: [post-critsmash-triage]
Minusing for advisory since it is an internal report and e10s is pref'd off on release.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main43-]
The fix for this has shipped. Needinfo'ing myself for a testcase.
Flags: needinfo?(mconley)
Comment on attachment 8703779 [details] [diff] [review]
Fix some bad test assertion strings

Noticed some strings needed to be flipped.
Flags: needinfo?(mconley)
Attachment #8703779 - Flags: review?(felipc)
Comment on attachment 8703780 [details] [diff] [review]
Regression test

Confirmed with a local backout of the fix that this fails, and passes with the fix applied.
Attachment #8703780 - Flags: review?(felipc)
Attachment #8703779 - Flags: review?(felipc) → review+
Attachment #8703780 - Flags: review?(felipc) → review+
Depends on: 1276013
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.