Feed reader appears to have an inappropriate security model. From testing, it seems to allow the child process to change arbitrary preferences, which pretty much game over for sandboxing on its own, but it also allows for execution of arbitrary commands via nsIShellService. When I first looked at this, I actually thought that feed-reader was safe, due to it only listening for one message on the parentprocessmessagemanager. (FeedConverter:ExecuteClientApp) I prevoiusly thought calling window.messageManager.addMessageListener() in the parent, wouldn't expose a messageHandler to the child. (and thus I thought the handlers at [1] were not exposed to the child) However I've been doing some testing in the Browser Content Toolbox, and it seems possible to send messages to fire these messages. Definitely need to get some confirmation here. Steps to reproduce: 1. Start a browser running e10s enabled 2. open any remote page (about:home) and the Browser Content Toolbox 3. Paste the script from the attachment to launch Calculator.app (mac only) If this test approach is valid (ie its not some devtools weirdness im overlooking) then we need to change the way the feedwriter is remoted. At a minimum we need to whitelist the pref names that can be changed, but also ensure that the file path can ONLY come from the file picker in the parent process. [1] http://searchfox.org/mozilla-central/source/browser/base/content/browser-feeds.js#281

Reason #6512352345234 to kill our feed reader. More seriously, I don't think you're wrong generally. Switching the feed reader to using AsyncPrefs (which has a builtin list of allowed prefs) might be possible (though I'm not sure if the subset of prefs the feedreader needs is well-contained, and it looks like we'd need to add complex pref support to asyncprefs). (In reply to Paul Theriault [:pauljt] from comment #0) > also > ensure that the file path can ONLY come from the file picker in the parent > process. How would you even do this, though? The code that sends a message to the parent (never mind the code that receives it) is umpteen degrees of separation/abstraction away from the filepicker code. :-(

(In reply to :Gijs from comment #1) > (In reply to Paul Theriault [:pauljt] from comment #0) > > also > > ensure that the file path can ONLY come from the file picker in the parent > > process. > > How would you even do this, though? The code that sends a message to the > parent (never mind the code that receives it) is umpteen degrees of > separation/abstraction away from the filepicker code. :-( So, tbh, I think this means we should remove the ability to change feed reader handler apps from the feed preview page, and replace it with a link that opens about:preferences#applications in a separate tab.

Note comment 67 - this was actually caught in review, but missed in the 'r+ with revisions' (it never got a revision). We will probably need to advocate for a policy change to avoid this in future once this bug is closed.

(In reply to Paul Theriault [:pauljt] from comment #3) > Note comment 67 - this was actually caught in review, but missed in the 'r+ > with revisions' (it never got a revision). We will probably need to advocate > for a policy change to avoid this in future once this bug is closed. Er, comment 67 in bug 1109714

> How would you even do this, though? The code that sends a message to the > parent (never mind the code that receives it) is umpteen degrees of > separation/abstraction away from the filepicker code. :-( If its too hard to keep the data in the parent, then we could just implement the pref whitelist (similar to [1] perhaps), and then add some message validation (maybe cache the file name in the parent side of the messender, and validate that its the same when we get it back from the child). JKT any chance you could look at this? [1] http://searchfox.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm

(In reply to :Gijs from comment #2) > (In reply to :Gijs from comment #1) > > (In reply to Paul Theriault [:pauljt] from comment #0) > > > also > > > ensure that the file path can ONLY come from the file picker in the parent > > > process. > > > > How would you even do this, though? The code that sends a message to the > > parent (never mind the code that receives it) is umpteen degrees of > > separation/abstraction away from the filepicker code. :-( > > So, tbh, I think this means we should remove the ability to change feed > reader handler apps from the feed preview page, and replace it with a link > that opens about:preferences#applications in a separate tab. That also works for me, and maybe makes more sense. Definitely less attack surface this way and then we can remove the messages altogether which is a win.

(In reply to Paul Theriault [:pauljt] from comment #5) > If its too hard to keep the data in the parent, then we could just implement > the pref whitelist (similar to [1] perhaps), and then add some message > validation (maybe cache the file name in the parent side of the messender, > and validate that its the same when we get it back from the child). Oh, hm, I assumed it was just using an <input type=file> for this, but it looks like it all goes via explicit messages, so you're right that we could cache things in https://dxr.mozilla.org/mozilla-central/rev/106a96755d3bcebe64bbbc3b521d65d262ba9c02/browser/base/content/browser-feeds.js#198 somehow.

(In reply to :Gijs from comment #7) > (In reply to Paul Theriault [:pauljt] from comment #5) > > If its too hard to keep the data in the parent, then we could just implement > > the pref whitelist (similar to [1] perhaps), and then add some message > > validation (maybe cache the file name in the parent side of the messender, > > and validate that its the same when we get it back from the child). > > Oh, hm, I assumed it was just using an <input type=file> for this, but it > looks like it all goes via explicit messages, so you're right that we could > cache things in > https://dxr.mozilla.org/mozilla-central/rev/ > 106a96755d3bcebe64bbbc3b521d65d262ba9c02/browser/base/content/browser-feeds. > js#198 somehow. We could even just send a name and/or icon and identifier down to the child, not giving the child access to the full path of the app at all. That also avoids passing up full paths (back) into the parent (that it then has to use/trust). If asked to open anything with an identifier it doesn't know, it can't do anything. :-)

I thought I would upload a WIP patch of the direction I was going in. - I fixed the Mime parser being in content which actually fixes a separate bug and stops it complaining in console. - I moved pref setting decisions to browser-feeds.js - Cleaned up code that was using the same vars - Prevented a race condition of missing _document in receiveMessage - Created setApplicationLauncherMenuItem to make a clearer messaging interface to the client - Created a unified SetFeedPref method for all pref setting - Created GetReaderForType and GetAlwaysUseState so the child doesn't have to even look at prefs (still some code to remove for this also) TODO: - Move the web content handler code into parent process, so get handlers is a new message the client can call to enumerate the select box but nothing else - Ensure client calls the parent before using a web handler - Establish if the second call to this._chooseClientApp in subscribe is ever called - Check there isn't anything else also

The STR to add a new web feed reader is using Same Origin of the example here: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerContentHandler So go to example.com and paste that into console, which will open up a permission prompt. We should probably make these content handlers HTTPS only too as it's a super powerful feature to consume all of a mime type.

Again another WIP patch moving even more code into the parent process and simplifying the API used by the child.

Comment on attachment 8842272 [details] [diff] [review] bug-1341191-2.patch Review of attachment 8842272 [details] [diff] [review]: ----------------------------------------------------------------- I applied and tested with this patch. I observed a couple problems: 1. after you select a new external app, it isn't selected as the chosen app in the select box (minor but confusing) 2. after you select a new external app the subscribe button is broken (nothing happens when I click subscribe) I think I found the issue (or one of them). See below. ::: browser/base/content/browser-feeds.js @@ +471,4 @@ > receiveMessage(msg) { > switch (msg.name) { > + case "FeedWriter:GetSubscriptionUI": > + const response = this._initSubscriptionUIResponse(msg.data.feedType); There is a bug at this line: response contains UI elements and other things that can't be serialised. Debugging I see the following when breaking after this line: >>>console.log(response) Object { handlers: Array[1], showFirstRunUI: false, defaultMenuItem: XPCWrappedNative_NoHelper, selectedMenuItem: XPCWrappedNative_NoHelper } They need to be something which can be sent (ie a string or whatever you are expecting in the child)

Error that dumps to Browser Console is: 2:55 PM Sending message that cannot be cloned. Are you trying to send an XPCOM object? browser-feeds.js:475:8

Fixes the issue with unserialisable message, will check tomorrow if the other issues are gone.

I tested this but i hit the same error - still cant subscribe with external program. Not sure if Im missing something but I will have to test again in the morning. Might need to clear my profile or something.

It looks like this is still in a lot of flux and Paul has already looked at this, so not going to deep-dive, but one thing I noticed when looking over a previous version of the patch is that it created some member variables where we previously used local variables (for the handlers list), but didn't null those out in the code nulling out the document/_window references. Any additional permanent dom references should probably be nulled out, too, to avoid leaks.

Fixed launching applications and nulling out the handlersList.

- Removed a race condition of setting the selected web handler by unifying the _setSelectedHandlerResponse code into the init code. - Made selected code unselect all other selected.

Fixes issue mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=1341401#c1 Auto defaulting an external client to open when the user has set it to open. - Observer code has been moved into the parent which fixed a tonne of race conditions I found. - Moving the observer also fixes multiple feed pages open at the same time selecting the right menu item - Filed Bug 1344061 after noticing having the preference pane open causes this issue. - Mostly been testing with just normal feed and not the video and audio types. - Moving to the prefObserver removes the need for a AlwaysUseStateResponse - Should be able to remove FeedWriter:GetReaderForType also will check...

- I checked over that we no longer needed the selected code from choose client app as we use pref observing to handle this. - This also removed the outstanding issue: "filename didn't get picked up when selecting from file picker. Selecting again then worked." - GetReaderForType was then able to be removed too. - I checked that wccr.getWebContentHandlerByURI won't allow a user to push in anything into the message and get escalation there too. - I removed the PREF_*SELECTED_APP observing as SetApplicationLauncherMenuItem handles this change, the user likely just searched for a new app and we are updating the interface there. - I checked over our browser/components/feeds/ and toolkit/components/feeds/ tests and all passed. Sone of this is my fav way of doing this however I wanted to go for the minimal transitions of this code possible. This is however ready for review and I can file follow up bugs on work that really should be done also.

Just tested all the things I could think of an it seems to work for me (it even worked with yahoo though this first time flow is spectacularly bad). In terms of solving the security bug, your changes have removed the vulnerable messages and the IPC that remains looks safe to me. I'm not thrilled that we still have "FeedConverter:ExecuteClientApp" (since it means that ability to write this pref means trivial code exec) but so long as we have the preview page I guess we can't avoid that.

Comment on attachment 8843593 [details] [diff] [review] Bug 1341191 - fix messaging priv escalation of feed reader, simplify messaging from parent to child. Reduce race conditions on page. Review of attachment 8843593 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-feeds.js @@ +3,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +const globalMessageManager = Cc["@mozilla.org/globalmessagemanager;1"] > + .getService(Ci.nsIMessageListenerManager); Nit: Services.mm gets you this. :-) @@ +53,5 @@ > + getService(Ci.nsIPrefBranch); > + > + let shouldLog = false; > + try { > + shouldLog = prefB.getBoolPref("feeds.log"); This can use Services.prefs.getBoolPref. Better yet, XPCOMUtils.defineLazyPreferenceGetter . :-) @@ +423,5 @@ > + prefs.addObserver(PREF_VIDEO_SELECTED_READER, this, false); > + prefs.addObserver(PREF_VIDEO_SELECTED_WEB, this, false); > + prefs.addObserver(PREF_AUDIO_SELECTED_ACTION, this, false); > + prefs.addObserver(PREF_AUDIO_SELECTED_READER, this, false); > + prefs.addObserver(PREF_AUDIO_SELECTED_WEB, this, false); Is there a specific reason not to use weak observers here? Related: is uninit() guaranteed to be called? If not, this leaks. @@ +454,5 @@ > + // This can happen in one feed and we want to message all feed pages > + this._prefChangeCallback = setTimeout(() => { > + this._prefChangeCallback = null; > + this._prefChanged(data); > + }, 2000); DeferredTask can simplify this for you. The hardcoded 2000ms here is also a bit of a code smell. @@ +479,5 @@ > + [Ci.nsIFeed.TYPE_VIDEO]: this._getReaderForType(Ci.nsIFeed.TYPE_VIDEO) > + }; > + globalMessageManager > + .broadcastAsyncMessage("FeedWriter:PreferenceUpdated", > + response); Wouldn't it be simpler to just have the pref observers on the content side directly? @@ +549,5 @@ > + }, > + > + _getReaderForType(feedType) { > + let prefs = Services.prefs; > + handler = "bookmarks"; This variable doesn't seem to be declared? Please make sure you run ./mach eslint on these changes, as well as any tests we might have...

:gijs comments addressed Regarding the observers on the content side and the messaging, I wanted to simplify the content side so it was more obvious if it was doing anything bad. I also hope in follow up bugs we can clean up the use of the observers as I'm pretty sure we can make about:preferences use this object also which means we won't need to observe at all just make a one way message in response to all the open pages when the prefs are set.

Comment on attachment 8844127 [details] [diff] [review] bug-1341191-7.patch Dan are you the right person for sec approval for this?

Comment on attachment 8844127 [details] [diff] [review] bug-1341191-7.patch Ignore that sorry :dveditz

Comment on attachment 8844127 [details] [diff] [review] bug-1341191-7.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? A sandbox escape would have to happen to exploit the code however they have full ability to execute anything on the machine based on the STR. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not explicitly, the code has been changed to be very explicit about preferences being set and executed and moved into the parent process. The code was manually tested so to work to the same functionality. Which older supported branches are affected by this flaw? Exploit started in Firefox 47 with Bug 1109714 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? None, I suspect it would be easier to disable the feature for older branches if we have a known sandbox escape. How likely is this patch to cause regressions; how much testing does it need? Currently we have very few tests for the UX of feed preview selection and in its current state it seems very broken. I suspect we should file follow up testing and UX fixes. This patch fixes a bunch of issues with subscribing and autoloading the reader, ultimately how it is in stable seems broken to me and mostly unusable. So I would suggest the risk of further breakage is low. Specifically the always use flow and web reader flow didn't work for me at all (See comments in the bug).

Just for clarity, did you still want me to review this patch or were you planning on getting r+ from someone else, or are you asking for sec-approval so you can push to try, or something else? :-)

I was looking for sec-approval so it can get merged and/or uplifted etc. I still notice this bug breakage with feeds which would be good to uplift as part of this however unrelated to the security issue (this was fixed at one point however not sure when it went broken again): https://bugzilla.mozilla.org/show_bug.cgi?id=1242669

(In reply to Jonathan Kingston [:jkt] from comment #28) > I was looking for sec-approval so it can get merged and/or uplifted etc. Right, but right now the patch doesn't have r+... :-)

Ah I thought you gave me a conditional r+ based on the changes required.

(In reply to Jonathan Kingston [:jkt] from comment #30) > Ah I thought you gave me a conditional r+ based on the changes required. That would have been ironic given that comment #3 argued against doing that on security-sensitive bugs. :-)

Comment on attachment 8844127 [details] [diff] [review] bug-1341191-7.patch I don't think this is working quite right. There's a few things: 1) on current nightly, the "always use <whatever> to subscribe to feeds" is disabled by default. After your patch, it's checked by default 2) if I follow the following steps: a) open Firefox with your patch on a clean profile b) open http://feeds.bbci.co.uk/news/rss.xml c) in the dropdown, select "my yahoo!", and click 'subscribe' d) click back. Now you're looking at the feed again (which is a little surprising, because we said we always wanted to subscribe using 'my yahoo' e) in the dropdown, select 'choose application' and pick an app of your choice (I picked google chrome, just because I could) f) untick the checkbox g) click 'subscribe now' h) close the extra app (chrome or whatever) i) on the feed page which is still open in Firefox, click any of the article links, then click 'back' expected: you see the feed page. actual: chrome launches and you don't see the feed page Keeping the review flag because I'm still looking at this...

(In reply to Jonathan Kingston [:jkt] from comment #28) > I was looking for sec-approval so it can get merged and/or uplifted etc. > > I still notice this bug breakage with feeds which would be good to uplift as > part of this however unrelated to the security issue (this was fixed at one > point however not sure when it went broken again): > https://bugzilla.mozilla.org/show_bug.cgi?id=1242669 I can't reproduce this being broken on vanilla Nightly. Can you clarify?

Comment on attachment 8844127 [details] [diff] [review] bug-1341191-7.patch Review of attachment 8844127 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-feeds.js @@ +440,5 @@ > + prefs.removeObserver(PREF_VIDEO_SELECTED_READER, this); > + prefs.removeObserver(PREF_VIDEO_SELECTED_WEB, this); > + prefs.removeObserver(PREF_AUDIO_SELECTED_ACTION, this); > + prefs.removeObserver(PREF_AUDIO_SELECTED_READER, this); > + prefs.removeObserver(PREF_AUDIO_SELECTED_WEB, this); Now that this is a weak ref, I would assume you no longer need to remove the observers. @@ +533,5 @@ > + > + _setPref(aPrefName, aPrefValue, aIsComplex = false) { > + LOG(`FeedWriter._setPref ${aPrefName}`); > + // Ensure we have a pref that is settable > + if (aPrefName && SETTABLE_PREFS.indexOf(aPrefName) !== -1) { make SETTABLE_PREFS a Set please, and use .has(). Ditto for VALID_READERS and VALID_ACTIONS and EXECUTABLE_PREFS. @@ +572,5 @@ > + _getAlwaysUseState(feedType) { > + let alwaysUse = false; > + try { > + if (Services.prefs.getCharPref(getPrefActionForType(feedType)) != "ask") > + alwaysUse = true; Nit: return Services.prefs.getCharPref(...) != "ask"; and at the end of the method, return false, and omit the 'alwaysUse' declaration in the beginning. ::: browser/components/feeds/FeedWriter.js @@ +156,5 @@ > }, > > + _setCheckboxCheckedState(aValue) { > + let checkbox = this._document.getElementById("alwaysUse"); > + if (aValue && checkbox) { This looks like it's at least partially responsible for what I was seeing - why are we only doing something if the value is true?

Comment 33: STR: 1. Go to the feed preview of https://www.npr.org/rss/podcast.php?id=510289 2. Select My Yahoo and Always use and click subscribe now 3. Return to https://www.npr.org/rss/podcast.php?id=510289 and notice it isn't checked Manually setting a default in the prefs or using about:preferences#applications causes the page to do a blank white page. To set the default action manually: 1. about:preferences#applications 2. Search Web Feed 3. Change the dropdown to something else This patch actually makes that more common as I fixed the always-use checkbox storage which was why it wasn't happening as much before. I'm still trying to dig into why this happens however it isn't clear. It appears browser/components/feeds/FeedConverter.js wccr.getAutoHandler(TYPE_MAYBE_FEED) is the issue with the call not being received by WebContentConverter.js. All I can assume is the request is of the wrong type and being prevented by the IDL there perhaps.

Comment on attachment 8844528 [details] [diff] [review] bug-1341191-8.patch Feedback resolved :) thanks

My patch also fixes Bug 1322839 and Bug 1242669. Bug 1242669 didn't hang like that however it reproduced Comment 35. I deleted the code in FeedConverter.js that checked for getAutoHandler and redirected as since this was moved to e10s the code no longer worked as WebContentConverter.js returned a different API at the bottom of it's file based on if it is running in content. Moving this to use the messaging to the feed preview on init solves process issue and also TYPE_IMAGE (This is actually a common feed type) which wasn't being caught pre e10s move also. wccr.getAutoHandler() isn't checked more than once though which means if the autohandler is set to "My Yahoo" at browser startup it will always redirect to "My Yahoo" until the pref is changed and the browser is restarted. :Gijs The only difference between patch-8 and patch-9 is fixing Comment 35. :pauljt I would appreciate another check over also if you have the time :). Thanks

Comment on attachment 8844706 [details] [diff] [review] bug-1341191-9.patch Review of attachment 8844706 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jonathan Kingston [:jkt] from comment #38) > Created attachment 8844706 [details] [diff] [review] > bug-1341191-9.patch > > My patch also fixes Bug 1322839 and Bug 1242669. Bug 1242669 didn't hang > like that however it reproduced Comment 35. > > I deleted the code in FeedConverter.js that checked for getAutoHandler and > redirected as since this was moved to e10s the code no longer worked as > WebContentConverter.js returned a different API at the bottom of it's file > based on if it is running in content. Moving this to use the messaging to > the feed preview on init solves process issue and also TYPE_IMAGE (This is > actually a common feed type) which wasn't being caught pre e10s move also. Can we have a followup for this, though, as it now seems that the feed preview page loads (visibly!) before we load the yahoo or whatever other web handler is registered? It should be able to redirect immediately... > wccr.getAutoHandler() isn't checked more than once though which means if the > autohandler is set to "My Yahoo" at browser startup it will always redirect > to "My Yahoo" until the pref is changed and the browser is restarted. Ah, that's kind of unfortunate... is this also a regression? I don't really understand why/how this works the way it does. The code looks to me like it's checking the prefs every time the subscription UI loads. So why doesn't this work? In fact, with the latest patch, even after a pref change in the application panel in the options, *and* a browser restart, I *still* get redirected to yahoo. That doesn't seem OK to land. :-(

> Can we have a followup for this, though, as it now seems that the feed preview page loads (visibly!) before we load the yahoo or whatever other web handler is registered? It should be able to redirect immediately... We could follow up with this however it seems now that I have fixed the storage of the alwaysUse the case where the user would get a white screen when viewing a feed would be much more common. I could perhaps break the checkbox again and file that in a follow up also. The flicker of the preview was "expected" because I made the content page handle the redirection. I would have to fix the code a lot more to get it working in the Converter code rather than the preview page. >> wccr.getAutoHandler() isn't checked more than once though which means if the >> autohandler is set to "My Yahoo" at browser startup it will always redirect >> to "My Yahoo" until the pref is changed and the browser is restarted. > Ah, that's kind of unfortunate... is this also a regression? I don't really understand why/how this works the > way it does. The code looks to me like it's checking the prefs every time the subscription UI loads. So why > doesn't this work? getAutoHandler reads from a cache in _setAutoHandler which doesn't seem to be updated when the pref updates, it also as you mention appears to not update on pref update (my experience was the browser would close before storing the pref even if setting it in about:config). So in summary I think patch 8 would better with a broken always use again. The follow up bug can fix the remaining issue(s).

- Fixes new eslint issue after rebase - Unfixes checkbox storage as mentioned in other comments to reduce visibility of "white screen of death" issue.

Comment on attachment 8845173 [details] [diff] [review] bug-1341191-8-a.patch Review of attachment 8845173 [details] [diff] [review]: ----------------------------------------------------------------- Something went wrong with the rebase here, this patch touches a gazillion files.

Sorry about that.

Comment on attachment 8845360 [details] [diff] [review] bug-1341191-8-b.patch Review of attachment 8845360 [details] [diff] [review]: ----------------------------------------------------------------- Alright, let's do this, then. Let's just make sure that we file all the relevant followup issues.

Comment on attachment 8845360 [details] [diff] [review] bug-1341191-8-b.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? A sandbox escape would have to happen to exploit the code however they have full ability to execute anything on the machine based on the STR. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not explicitly, the code has been changed to be very explicit about preferences being set and executed and moved into the parent process. The code was manually tested so to work to the same functionality. Which older supported branches are affected by this flaw? Exploit started in Firefox 47 with Bug 1109714 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? None, I suspect it would be easier to disable the feature for older branches if we have a known sandbox escape. How likely is this patch to cause regressions; how much testing does it need? Currently we have very few tests for the UX of feed preview selection and in its current state it seems very broken. I suspect we should file follow up testing and UX fixes. This patch fixes a bunch of issues with subscribing and autoloading the reader, ultimately how it is in stable seems broken to me and mostly unusable. So I would suggest the risk of further breakage is low. Specifically the always use flow and web reader flow didn't work for me at all (See comments in the bug).

sec-approval+ for trunk. We'll want branch patches (of some sort) made and nominated.

Applied onto release branch prior tip commit was: changeset: 369624:66777ae38a64 fxtree: release user: ffxbld <release@mozilla.com> date: Fri Mar 17 15:09:56 2017 -0700 summary: No bug - Tagging a38b8538f42412d2c606417264df4abcc183dd57 with FIREFOX_52_0_1_BUILD2, FIREFOX_52_0_1_RELEASE a=release CLOSED TREE

Applied onto beta branch prior tip commit was: changeset: 376823:32dbe106b561 fxtree: beta user: L10n Bumper Bot <release+l10nbumper@mozilla.com> date: Sat Mar 18 05:00:11 2017 -0700 summary: Bumping Fennec l10n changesets a=l10n-bump

Applied onto aurora branch prior tip commit was: changeset: 375255:81dc67758a0f fxtree: aurora user: Masatoshi Kimura <VYV03354@nifty.ne.jp> date: Sat Mar 11 19:06:40 2017 +0900 summary: Bug 1345222 - Fix user-set ClearType params detection. r=mchang a=lizzard

Applied onto inbound branch prior tip commit was: changeset: 348345:145f874ee97a fxtree: inbound user: Mats Palmgren <mats@mozilla.com> date: Sat Mar 18 18:44:27 2017 +0100 summary: Bug 1347979 - Don't call methods that may flush in nsRange::GetInnerTextNoFlush. r=smaug

Applied onto esr52 branch prior tip commit was: changeset: 355377:035fb01aae64 user: ffxbld date: Sat Mar 18 08:00:04 2017 -0700 summary: No bug, Automated HPKP preload list update from host bld-linux64-spot-034 - a=hpkp-update

Hey Al, I have attached patches for each release we have branches for, hope this is sufficient. Thanks

(In reply to Jonathan Kingston [:jkt] from comment #52) > I have attached patches for each release we have branches for, hope this is > sufficient. I'll go give approvals on them. Normally, you can just nominate the patch for the branch it is for an either someone in security or in release managers will come along and approve it afterwards.

A new eslint rule (bug 1345294) was enabled in the time since this patch was written, so eslint was failing like https://treeherder.mozilla.org/logviewer.html#?job_id=85121832&repo=mozilla-inbound I landed a followup in https://hg.mozilla.org/integration/mozilla-inbound/rev/58671e4ff465454f4998d3648cd04b7b1d7ede3e that will hopefully handle it correctly. The rule is only enabled in 55+, so it shouldn't be needed on any of the uplifts.

https://hg.mozilla.org/mozilla-central/rev/6e89a1a18f06 https://hg.mozilla.org/mozilla-central/rev/58671e4ff465

Paul, I tried reproducing the issue with the STR from comment#0 but didn't have any luck reproducing. Is there another way I can test and verify this?

(In reply to Kamil Jozwiak [:kjozwiak] from comment #60) > Paul, I tried reproducing the issue with the STR from comment#0 but didn't > have any luck reproducing. Is there another way I can test and verify this? This STR in comment one is really more of a proof of concept than a security bug. Its not something which you can verify through testing (other than observing that the PoC doesn't work any more). But I can confirm that its verified as fixed through code review. You can look here: https://dxr.mozilla.org/mozilla-beta/source/browser/base/content/browser-feeds.js#586 That's the altered version of what was vulnerable before - instead of taking the preference name and value from the child, the preference name is chosen from a whitelist in the parent if I am reading this correctly. I test this before the patch landed, and you can see also that the previously vulnerable message listener ("FeedWriter:SetFeedCharPref") has been removed from the codebase. I also verified this in 53 beta, just now. If you want to replicate, the steps are: 1. (optional maybe?) enable e10s (set browser.tabs.remote.force-enable to true) 2. open any remote page (about:home) and the Browser Toolbox (NOTE: NOT the Browser Content Toolbox, we want to look at the parent process) 3. Open the debugger tab in the Toolbox, and search for browser-feeds.js There you can confirm by visual inspection that the vulnerable message FeedWriter:SetFeedCharPref doesnt exist in this file. But I've just done that, so I guess I'll set this to verified and you can change it if this isn't appropriate.

BTW in case this isn't clear, this is a sandbox escape bug - it doesn't apply to non-e10s builds because they don't have content processes to sandbox, so there is nothing to escape from.

Whoops needinfo should have been removed ages ago.

Comment on attachment 8845360 [details] [diff] [review] bug-1341191-8-b.patch Review of attachment 8845360 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-feeds.js @@ +52,5 @@ > + > +XPCOMUtils.defineLazyPreferenceGetter(this, "SHOULD_LOG", > + "feeds.log", false); > + > +function LOG(str) { browser-feeds.js is loaded using a <script> tag, so it shares the global scope of browser.xul... and this patch is adding plenty of symbols exposed in the global scope :-(. Especially things with names as generic as "LOG"; I hope nothing else has started to unintentionally rely on this file.

Good point, let me raise a follow up. Probably not a hard patch to clear up all of these globals. From what I can see it is only there. As you mention though that is a wide scope though, sorry about this.