We have a lot of issues with Adobe Flash and we need to reduce instantiations. I won't re-iterate the full list of issues here. This bug is about the possibility of greatly reducing Adobe Flash instantiations by using Shumway for all Flash advertising by default. This is a promising strategy because: 1) Ads are responsible for the majority of Flash instantiations. Probably somewhere between 50% and 90%. We'll drop Adobe Flash instantiations in a huge way. 2) Shumway doesn't have many of the same security and stability issues that Adobe Flash does. 3) Many of Shumway's weaknesses aren't problematic for ads. We don't need to worry much about compatibility, because displaying ads perfectly isn't as important as primary content. If we get a slow script dialog in Shumway ad content, we can just kill it. Users won't care. Things we need to take care of: 1. Figure out how to somewhat reliably detect Flash ad content. This doesn't need to be anywhere near perfect, it just needs to detect a large percentage of Flash ads and we can improve it over time. We can start by trying the existing tracking detection infrastructure. 2. Make sure Shumway memory usage is acceptable. It might be OK already, but if it isn't the Shumway team has suggested that there is quite a bit of low hanging fruit for reducing memory usage, should this become a priority. Much of it can be fixed in 2-3 weeks. 3. Ensuring that the Shumway sandbox is secure. This shouldn't be hard. All things that require elevated privileges are already done outside the player sandbox, and verifying the remoting protocol we employ between the two parts should be manageable. 4. Ensure that our implementation of crossdomain.xml is correct and secure. 5. Get SecurityDomains working. In order to make SecurityDomains work, we have to make the player work with multiple compartments that contain copies of all builtins, including the DisplayObject classes. These all have to be able to partake in display list management while otherwise being separate from each other. Then we have to carefully audit all points where content can access things from other SecurityDomains and make sure we handle them correctly. Somewhat luckily for us Flash's security model around all this is almost ridiculously more loose than Gecko's compartments model, but this is still a substantial challenge. Which we have to solve because we have to deal with ad loaders that load banner SWFs from other domains. In most cases the loader will just do `Security.allowDomain('*')`, making all of this unnecessary, but we cannot know that before the fact. 6. If possible, just kills slow scripts that originate with Shumway ad content. Don't bother the user with a dialog. This is all do-able in a relatively short amount of time, I think, if it's a priority. There are relatively few critical unknowns involved.

I compared the memory consumption with 10 tabs, having the top 10 ad sponsored sites loaded (https://pastebin.mozilla.org/8822420, based on current Alexa reports). On average, having Shumway enabled uses only ~10% more memory than having those sites opened with the Flash Player (tested in Firefox 35 on Mac OS X 10.10).

What process(es) were you measuring?

This patch uses our tracking blocklist to detect Flash ads, then we play them with Shumway. Works pretty well. Some ads don't get detected, but that's fine, we can improve the blocklist over time. Occasionally an ad won't render, or render properly, but it's not too common and doesn't impact the user experience on the page much.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > What process(es) were you measuring? Currently we are using test/psshu.js to measure entire tree of browser subprocesses (by using "ps" command https://github.com/mozilla/shumway/blob/master/test/psshu.js#L181), which includes "plugin-container".

(In reply to Josh Aas (Mozilla Corporation) from comment #3) > This patch uses our tracking blocklist to detect Flash ads, then we play > them with Shumway. Works pretty well. Some ads don't get detected, but > that's fine, we can improve the blocklist over time. We should consider using an Adblock Plus or uBlock list that is designed to match ads instead of our tracker list. https://easylist.adblockplus.org/ https://github.com/gorhill/uBlock

(In reply to Chris Peterson [:cpeterson] from comment #5) > We should consider using an Adblock Plus or uBlock list that is designed to > match ads instead of our tracker list. I'd be all for any improvements in ad detection for this feature, but I wouldn't block deployment on it. The current stuff detects enough ads that it greatly reduces Flash instantiations, not worth delaying the whole thing to improve detection.

I did some detection frequency testing with the patch here. Browsed the Web while logging instantiations. This is rough and not reproducible since I didn't keep track of which pages I visited over ~20 minutes, but it looks like we're displaying 2/3 of all Flash content with Shumway, and about 80% of all Flash ads with Shumway. A 2/3 reduction in Adobe Flash instantiations would be a great start. Wonder what our crash/hang stats would look like with that.

Adds a pref. Requesting review from Monica, curious what else we'd need to do before testing this on Nightly.

Comment on attachment 8567207 [details] [diff] [review] proof of concept patch v1.1 Review of attachment 8567207 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks fine, but you can't check in before confirming with ckolos and tblow. ::: browser/app/profile/firefox.js @@ +1660,5 @@ > pref("shumway.disabled", true); > #else > pref("shumway.disabled", false); > pref("shumway.swf.whitelist", "http://g-ecx.images-amazon.com/*/AiryBasicRenderer*.swf"); > +pref("shumway.ads", true); We can't turn on tracking protection updates for the whole nightly population without coordinating with Mark's team. I'll send you an email intro to the right people. ::: netwerk/base/nsIURIClassifier.idl @@ +61,5 @@ > * make network requests. The result is an error code with which the channel > * should be cancelled, or NS_OK if no result was found. > */ > + nsresult classifyLocalWithURI(in nsIURI aURI, > + in boolean aTrackingProtectionEnabled); needs a uuid rev ::: toolkit/components/url-classifier/SafeBrowsing.jsm @@ +107,5 @@ > > debug = Services.prefs.getBoolPref("browser.safebrowsing.debug"); > this.phishingEnabled = Services.prefs.getBoolPref("browser.safebrowsing.enabled"); > this.malwareEnabled = Services.prefs.getBoolPref("browser.safebrowsing.malware.enabled"); > + this.trackingEnabled = true; This needs to be Services.prefs.getBoolPref("privacy.trackingprotection.enabled) || Services.prefs.getBoolPref("shumway.ads") It is important to be able to hotfix everything off in case of emergency. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +170,5 @@ > return NS_OK; > } > > static nsresult > +TablesToResponse(const nsACString& tables) This needs a comment that we guarantee that the table list is constructed correctly at call time, so we don't need to suppress found tables at callback time.

Comment on attachment 8567207 [details] [diff] [review] proof of concept patch v1.1 Review of attachment 8567207 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +170,5 @@ > return NS_OK; > } > > static nsresult > +TablesToResponse(const nsACString& tables) Where do we guarantee this?

> > static nsresult > > +TablesToResponse(const nsACString& tables) > > This needs a comment that we guarantee that the table list is constructed > correctly at call time, so we don't need to suppress found tables at > callback time. Welp, bugzilla ate this part. Anyway, it's far from obvious now that this doesn't break selectively disabling the malware/phishing checks.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #11) > > > static nsresult > > > +TablesToResponse(const nsACString& tables) > > > > This needs a comment that we guarantee that the table list is constructed > > correctly at call time, so we don't need to suppress found tables at > > callback time. > > Welp, bugzilla ate this part. Anyway, it's far from obvious now that this > doesn't break selectively disabling the malware/phishing checks. BuildTables does this. https://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1175 Doug brought up a good point. How do we know that shumway has been audited to be less vulnerable than Flash? Sorry if this has already been done -- I'm just not very familiar with this area.

>BuildTables does this. Cool. >How do we know that shumway has been audited to be less vulnerable than Flash? Uh, Shumway is a piece of JavaScript. Unless it's running with special privileges, it's protected by the JS sandbox and any vulnerability in Shumway would already have been a vulnerability in our existing JavaScript implementation. So Shumway is more secure because it doesn't add any attack surface that wasn't already there, whereas Flash adds a whole bunch of native code that is special cased all over. So the question is really if the Shumway add-on has any special priviledges normal JS wouldn't have, right?

TL;TR: Shumway needs extensive security work and testing before it can be enabled for anything except very restricted, carefully chosen targets. It also needs to run out of process so we don't substantially worsen the user experience of ad-heavy websites. (In reply to Josh Aas (Mozilla Corporation) from comment #8) > Adds a pref. Requesting review from Monica, curious what else we'd need to > do before testing this on Nightly. Non-negotiably: - Substantial security testing and implementation of missing features + bug fixes - JSPlugins to run Shumway in its own process (In reply to Gian-Carlo Pascutto [:gcp] from comment #13) > >How do we know that shumway has been audited to be less vulnerable than Flash? Obviously, it's not affected by the whole range of attack vectors around memory corruption. (At least not more than all JS is.) But yes, it opens up potential attack vectors of its own that we need to close/verify. > > So the question is really if the Shumway add-on has any special priviledges > normal JS wouldn't have, right? It does, but that's not even all there is to this. Flash has a security model that is sufficiently different from that of html/js that it can only be implemented using elevated privileges. Most importantly, it has a mechanism to load resources from other domains that is roughly equivalent to but different from CORS: slightly simplified, you can store a special `crossdomain.xml` file on a server that gets queried whenever a resource from that server is loaded in a way that would leak information[1]. If that file contains a rule that allows the loading SWF's domain, the resource gets loaded, otherwise a SecurityError is thrown. We have an implementation of the crossdomain.xml mechanism, but since it hasn't undergone extensive testing, we are entirely sure that it has bugs. Additionally, SWFs can be loaded as child content (*very* roughly analogous to iframes, but with substantial differences) from any domain without checking crossdomain.xml. Whether the main SWF has access to the child SWF's content (or the other way around) is determined using calls to `system.SecurityDomain.allowDomain`[2,3]. Using this function, access can be granted and revoked at arbitrary times. We currently don't support the underlying mechanisms at all, but mbx is working on that and we should have something soon-ish. Then there's loading of resources in a way that doesn't leak information[4]. This includes images and videos loaded from other domains that're just displayed but the data of which can't be read by the loading SWF. We don't yet implement the required security checks. Doing so wouldn't be all that hard and our story on hardening this is quite good, but it needs doing and testing. Finally, there's small-ish things like clipboard access, which must only be granted under active user input event handlers. Much of this is probably not relevant to ads, but we have to at least be sure to have it properly disabled. [1] http://www.adobe.com/devnet/articles/crossdomain_policy_file_spec.html [2] http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/system/Security.html [3] http://help.adobe.com/en_US/flex/using/WS2db454920e96a9e51e63e3d11c0bf6167e-7fff.html#WS2db454920e96a9e51e63e3d11c0bf69084-7f11 [4] Well, supposedly: there are the usual side channels such as filter calculation duration, duration of function calls, etc.

Addresses issues Monica pointed out and puts Shumway out of process. Later on we'll put Shumway out of process via JS plugins.

To augment what Till says in comment 14: * An example of an issue with crossdomain.xml that could cause universal CSRF is in bug 1029253. * An example of what SWF-to-SWF domain sandboxing means, see bug 1070213. We get a huge win by eliminating memory vulns in the Flash Player by moving things to Shumway. However, we also have to get the Flash Player security features correct in order to achieve parity there.

Please include telemetry for the proportion of ads that are getting redirected to shumway in this bug or file a followup.

This will probably drive coverage down: http://venturebeat.com/2015/02/25/google-now-automatically-converts-flash-ads-to-html5/

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #18) > This will probably drive coverage down: > http://venturebeat.com/2015/02/25/google-now-automatically-converts-flash- > ads-to-html5/ True. The question is to which extent. That depends on the market share adwords has for SWF banner ads, and the percentage of ads they're able to convert. I don't have sources for either, sadly.

Btw, now that https://bugzilla.mozilla.org/show_bug.cgi?id=1144786 has landed, this patch needs to be updated to call ClassifyLocalWithTables("<whatever the name of the shumway table is>" and also to register that table for updates in SafeBrowsing.jsm. Additionally, if you'd like to check redirects you can QI the underlying channel to nsIRedirectHistory and call ClassifyLocal on each redirect (https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIRedirectHistory.idl).

Andrew is working on the Gecko API to identify which SWFs we should load in Shumway instead of Flash.

jst says the Shavar-based whitelist does not need to block Nightly testing because we can temporarily use the "shumway.swf.whitelist" pref.

This is just a version of Josh's patch updated to use ClassifyLocalWithTables. It still does not check redirects.

Comment on attachment 8602855 [details] [diff] [review] prototype running Flash ads with Shumway by default. (does not work with e10s) This doesn't work with e10s, because you can't use the URL classifier in a content process, so that will require some additional work.

I was using the table incorrectly. This prototype is just using the tracking database rather than some custom Shumway thing, so I swapped over to using ClassifyLocal and now it actually Shumwayifies things sometimes.

(In reply to Andrew McCreight [:mccr8] from comment #26) > I was using the table incorrectly. This prototype is just using the tracking > database rather than some custom Shumway thing, so I swapped over to using > ClassifyLocal and now it actually Shumwayifies things sometimes. Interesting given that all that ClassifyLocalWithTables() does is set the "tables" string and then call ClassifyLocal(). Looking at your previous patch though, I can see that you didn't use the correct table name in your call to ClassifyLocalWithTables(). You had "-track-" instead of "mozpub-track-digest256".

(In reply to François Marier [:francois] from comment #27) > Looking at your previous patch though, I can see that you didn't use the > correct table name in your call to ClassifyLocalWithTables(). You had > "-track-" instead of "mozpub-track-digest256". Yeah. I've never looked at this code before, and I got confused with all the strings flying around various places.

The tentative name for the Shumway whitelist table is "mozpub-shumway-digest256". The Shumway whitelist source that the Cloud Services team will import into the shavar service is hosted on GitHub here: https://github.com/mozilla/shumway-whitelist

Andrew, rtilder has seeded the initial Shumway whitelist on the shavar server. Can you please verify that your patch works with the new "mozpub-shumway-digest256" shavar table? Are there any other issues blocking this bug? Ryan says: [The Shumway whitelist] is now part of the existing publishing system and is retrievable from prod. [rtilder@bamf ~]$ cat foofoo mozpub-shumway-digest256;a:11,17,200001,1435681803 [rtilder@bamf ~]$ curl -s -d @foofoo 'https://tracking.services.mozilla.com/downloads?client=BIGBEEFY&appver=7&pver=2.2' | head -4 n:2700 i:mozpub-shumway-digest256 ad:1435681803,17,11,200001 a:1436902204:32:1696 [rtilder@bamf ~]$ I've excluded the binary data for obvious reasons. Don't worry about the content of the response if you don't already understand it. Suffice to say that it should populate the Shumway whitelist table in the client correctly.

(In reply to Chris Peterson [:cpeterson] from comment #30) > Andrew, rtilder has seeded the initial Shumway whitelist on the shavar > server. Can you please verify that your patch works with the new > "mozpub-shumway-digest256" shavar table? Are there any other issues blocking > this bug? This bug is waiting on the JS plugins work, as per the dependency. The patch will have to be completely rewritten, though I'm hoping it will be simple to write. I'll look into how to use another table.