1277876 - Dynamic blocklist to disallow plugins triggered from specified 3rd-party domains

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Benjamin Smedberg]

Flash and other plugins are still a primary target for malware authors and distributors. In order to mitigate the risk of large-scale attacks, we're going to ask sites that are commonly loaded across the internet to disallow any plugins running from their domain.

I will be reaching out to popular social-media sites that use embedded content and sharing buttons, as well as ad networks, and ask them to add themself to this list. In the future we may decide to add sites to this list directly.

Comment 1

[Benjamin Smedberg]

One possible way to do this is to leverage the <iframe sandbox> infrastructure and have a new allow-plugins permission type. I'm not sure how practical that is or if adding that would affect existing sandboxed iframe content.

Comment 2

[Benjamin Smedberg]

Oh, I see that sandboxed iframes are never allowed to open plugins.

Comment 3

[François Marier [:francois]]

The plugin-types directive in CSP2 may also be useful here (bug 1045899).

Comment 4

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I think we may want to track this as a feature.

Comment 5

[David Durst [:ddurst]]

In the Go Faster presentation, Kinto seems like a good fit for this (rather than shavar).

Comment 6

[Benjamin Smedberg]

I have this implemented and I'm working on tests. I'm concerned about one of my original specifications, though:

* This restriction shall propagate to other windows opened from the restricted frames.

If we do this, then any site which is opened as a result of clicking on an ad will be unable to use Flash. This may be an unexpected side effect. OTOH, it reduces the effectiveness of this as a security mitigation, because it's not *that* hard to trick people into triggering links, even with popup blocking.

Comment 7

[Benjamin Smedberg]

Oh, to be clear: the current implementation just loads the list from a pref. When we need to make this more dynamic, that will be a followup (and I'll probably want somebody else to take that).

Comment 8

[Benjamin Smedberg]

Attachment: Bug 1277876 - Add a mechanism which disallows plugins from a list of domains, when those domains are loaded in a 3rd-party context.

Review commit: https://reviewboard.mozilla.org/r/64062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64062/

Comment 9

[Benjamin Smedberg]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8e0e3ff9f41

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

I'm not a peer of DOM or DocShell, neither are you. So redirect to bz.

To me, I'm a bit concerned about potential performance impact of this change when the list grows large.

Comment 11

[Chris Peterson [:cpeterson]]

Storing the list in the Shavar service (used by Tracking Protection and Safe Browsing) is the Right Thing because that's where we store all our other URL lists and list fixes can be pushed to users within 45 minutes.

Shavar is probably faster than the pref list too, since Shavar uses O(1) hash comparisons instead of O(n) linear search of the pref list using string comparisons.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Some questions:

1)  How long do we expect this blacklist to be?
2)  Is there a reason we're using a different definition of "third-party" than mozIThirdPartyUtil does?
3)  With this patch, if I set up a sandboxed iframe, then load one of the blacklisted URIs, then remove the "sandbox" attribute, and then navigate to some non-blacklisted URI the result will not have sandboxed plugins.  On the other hand, if I do all that without messing with iframe sandboxing it will.  Is that deliberate?

Comment 13

[Benjamin Smedberg]

> 1)  How long do we expect this blacklist to be?

Initially, I have 8-12 entries to try. At first this will only be deployed as part of a test pilot experiment. We'll definitely to something to move this out of a pref and use a more efficient hash lookup system.

> 2)  Is there a reason we're using a different definition of "third-party"
> than mozIThirdPartyUtil does?

No. I didn't know about mozIThirdPartyUtil, and I copied the code from WarnIfSandboxIneffective immediately below this. Happy to do something else if that's better/more-correct.

> 3)  With this patch, if I set up a sandboxed iframe, then load one of the
> blacklisted URIs, then remove the "sandbox" attribute, and then navigate to
> some non-blacklisted URI the result will not have sandboxed plugins.

I believe that the docshell mSandboxPlugins will still be `true` and so when nsDocument::StartDocumentLoad calls nsIDocShell->GetSandboxFlags the flag will apply permanently. That's in fact why I had the separate mSandboxPlugins boolean on the docshell, rather than just modifying the sandbox flags directly.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> > 3)  With this patch, if I set up a sandboxed iframe, then load one of the
> > blacklisted URIs, then remove the "sandbox" attribute, and then navigate to
> > some non-blacklisted URI the result will not have sandboxed plugins.
> 
> I believe that the docshell mSandboxPlugins will still be `true` and so when
> nsDocument::StartDocumentLoad calls nsIDocShell->GetSandboxFlags the flag
> will apply permanently. That's in fact why I had the separate
> mSandboxPlugins boolean on the docshell, rather than just modifying the
> sandbox flags directly.

If the iframe has already been sandboxed, your
> if (mSandboxFlags & SANDBOXED_PLUGINS) {
>   return;
> }
at the beginning of nsDocument::UpdatePluginSandbox would stop it from calling aDocShell->SandboxPlugins() at the end of the function, and thus when the sandboxing stops, the new document would be loaded without plugins being sandboxed.

Comment 15

[Benjamin Smedberg]

Comment on attachment 8770642 [details]
Bug 1277876 - Add a mechanism which disallows plugins from a list of domains, when those domains are loaded in a 3rd-party context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64062/diff/1-2/

Comment 16

[Benjamin Smedberg]

You're right! I fixed that up to check against the docshell, which is also a fairly cheap check.

Comment 17

[Benjamin Smedberg]

I'm on PTO for two weeks, but we'd like to get this landed for FF50, so I'm going to hand it off to Felipe for final review fixups and landing.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8770642 [details]
Bug 1277876 - Add a mechanism which disallows plugins from a list of domains, when those domains are loaded in a 3rd-party context.

https://reviewboard.mozilla.org/r/64062/#review63768

::: dom/base/nsDocument.cpp:2473
(Diff revision 2)
>  }
>  
> +static nsTArray<nsCString>
> +GetPluginBlockDomainList()
> +{
> +  // For the moment, we're keeping this list in a pref, comma-delimited.

I'm still pretty worried about the performance of this, especially as we add more stuff.  Please ensure bugs are filed to have a better setup here.

::: dom/base/nsDocument.cpp:2515
(Diff revision 2)
> +  nsCOMPtr<nsIChannel> parentChannel;
> +  parentDocShell->GetCurrentDocumentChannel(getter_AddRefs(parentChannel));
> +  if (!parentChannel) {
> +    return;
> +  }
> +  nsresult rv = nsContentUtils::CheckSameOrigin(aChannel, parentChannel);

Again, I'd like to understand why we're not using our existing definition of "third party" here.

And while we're at it, is there a reason we're comparing to the parent docshell's channel, not the parent document's URI?  That reason should be documented....

::: dom/base/nsDocument.cpp:2535
(Diff revision 2)
> +  // Is this document loading an origin on the blacklist?
> +  for (auto blockDomain : GetPluginBlockDomainList()) {
> +    if (blockDomain.IsEmpty()) {
> +      continue;
> +    }
> +    if (IsSubdomainOf(domain, blockDomain)) {

I have a really hard time believing we don't have an existing utility for this....

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, and please ensure we have an intent to ship for this.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1277876 - Add a mechanism which disallows plugins from a list of domains, when those domains are loaded in a 3rd-party context.

Review commit: https://reviewboard.mozilla.org/r/67924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67924/

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Ok so I've changed the patch to use mozIThirdPartyUtil, which also addresses the suggestion to check against the URI instead of the channel. (Although mozIThirdPartyUtil has a function that takes a channel too and I wonder if that would be the same.) In any case, I re-ran the test and it still works as expected.

Hopefully this is landable now, and I'll file the follow-up to improve the blocklist and let Benjamin handle that and the intent to ship when he's back.

ReviewBoard didn't merge the patches (probably due to different requester?), so here's an interdiff to make it easier: https://pastebin.mozilla.org/8887742 . The only change is in nsDocument::UpdatePluginSandbox

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Alright, I filed bug 1290547 to follow up on using Shavar instead of a pref, and sent an Intent to Implement to dev.platform about this bug

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8775861 [details]
Bug 1277876 - Add a mechanism which disallows plugins from a list of domains, when those domains are loaded in a 3rd-party context.

https://reviewboard.mozilla.org/r/67924/#review65228

::: dom/base/nsDocument.cpp:2455
(Diff revision 1)
> +  return domains;
> +}
> +
> +// returns true if 'a' is equal to or a subdomain of 'b',
> +// assuming no leading dots are present.
> +// copied from nsCookieService.cpp

Maybe I should have been clearer, but if this is getting copied around, it should perhaps live in nsNetUtil instead.

Note, though, that this implementation is pretty broken if IPv4 addresses are involved, and likely doesn't handle non-ASCII domains right...  Are those planned to be included in this list?

Maybe that all means this should not be added as a general utility after all, but then please _document_ that fact and the restrictions on the use of this code.

::: dom/base/nsDocument.cpp:2484
(Diff revision 1)
> +  if (!parentAsNav) {
> +    return;
> +  }
> +
> +  nsCOMPtr<nsIURI> parentURI;
> +  nsresult rv = parentAsNav->GetCurrentURI(getter_AddRefs(parentURI));

Again, why is the right thing to compare to the parent docshell's URI and not the parent document's?  In practice it's probably about the same, but not always.

The channel version of stuff on nsIThirdPartyUtil would check all things up the frame chain, so would have different semantics from what this code has right now.  But what semantics _does_ this code want?

Specifically, say I have a page at http://foo.com which embeds an iframe from http://x.example.com which embeds an iframe from http://y.example.com.

If "y.example.com" is on our list but x.example.com is not, should plug-ins be disabled in the innermost frame?  It's not third-party with its immediate parent, but _is_ third-party with http://foo.com....

I would really like to understand what the desired behavior here is, so I can evalute whether we're implementing it properly.  Using phrases like "3rd-party-context" which have ambiguous meaning doesn't really pin things down.  :(

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Ok, I'll leave those questions to be answered by Benjamin them as he has put all the thought into this so far. Benjamin, if you want to pass me back this bug after looking at bz's questions, feel free.

Comment 25

[Benjamin Smedberg]

> Note, though, that this implementation is pretty broken if IPv4 addresses
> are involved, and likely doesn't handle non-ASCII domains right...  Are
> those planned to be included in this list?

IPv4 will definitely not be used. There are currently no non-ASCII domains on the list, but there may be in the future so I'd like to understand how this should work. Do I have to do extra work to normalize/denormalize punycode?

> > +  nsCOMPtr<nsIURI> parentURI;
> > +  nsresult rv = parentAsNav->GetCurrentURI(getter_AddRefs(parentURI));
> 
> Again, why is the right thing to compare to the parent docshell's URI and
> not the parent document's?  In practice it's probably about the same, but
> not always.

I copied this from http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#2469 and I don't know what the practical difference is.

> Specifically, say I have a page at http://foo.com which embeds an iframe
> from http://x.example.com which embeds an iframe from http://y.example.com.
> 
> If "y.example.com" is on our list but x.example.com is not, should plug-ins
> be disabled in the innermost frame?

Yes. The original version of this code did a strict same-origin check, and I'm not sure why we replaced this with mozIThirdPartyUtils. I think a strict same-origin check is more useful here and would produce the correct behavior.

I think the suggestion was to use mozIThirdPartyUtils to replace the "IsSubdomainOf" function? But that is definitely not what we want, because mozIThirdPartyUtils uses eTLD+1 to do matching, and that's not what we want.

For example, we're probably going to have the following domains on the list:

apis.google.com (Google Plus login buttons)
platform.twitter.com (Twitter +1 buttons)

We don't want to block plugins in twitter.com or google.com, just those specific subdomains.

Comment 26

[Benjamin Smedberg]

I think I'd like to ask for review on the original patch (without mozIThirdPartyUtils) but I'd like to understand the question about the parent docshell URI versus parent document URI first.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Do I have to do extra work to normalize/denormalize punycode?

If you ensure that your list contains punycode versions of things and use GetAsciiHost on the nsIURI, that should do what you want.  If the list is going to be allowed to contain either non-ASCII or punycode, then some sort of normalization would need to be done, yes...

> I copied this from http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#2469

Yeah, I have no idea why that code is doing what it's doing.  Luckily, it doesn't matter too much what it's doing, because all it affects is whether a warning is reported to the console, so the consequences of the check getting a wrong answer are pretty much nonexistent.

The stakes are somewhat higher in this bug, so I'd like to make sure we get this right...

It's also not obvious to me that the URI of the docshell's "current document channel" necessarily matches the "current URI" of the docshell, fwiw....  I'm sorry this is all such a mess.  :(

> and I don't know what the practical difference is.

I thought about this a bit, and I think the practical differences (in terms of when the docshell's URI does not match the URI of the document loaded in that docshell; I have no idea where the URI of the docshell's document channel fits into this) are at least the following:

1)  I _think_ that for error pages, the docshell's URI is the URI of the thing
    we tried to load but the document URI is the URI of the error page.
2)  For javascript: loads that produce a document, once we fix our impl to
    follow the spec the document URI will be the thing that started the load
    and the docshell URI... who knows; depends on how we fix our impl.
3)  When pushState/popState happen the docshell's URI is not changed,
    but the document URI is changed.

I _think_ that's it.  I hope.  For purposes of this bug, I think #1 is not relevant, #3 is not a problem because popState/pushState can't change the hostname part of the URI, but #2 is worrying.

But taking a step back here, why is the document or docshell URI the right thing here to start with?  Specifically, let's consider the following scenarios:

A) A page at http://foo.com embeds an iframe coming from a blob: URI,
   which then embeds an iframe from http://foo.com.  If foo.com is
   on the list, should that innermost iframe be allowed to run plug-ins?
B) A page at http://foo.com embeds an <iframe srcdoc>, which then embeds
   an iframe from http://foo.com.  If foo.com is on the list, should that
   innermost iframe be allowed to run plug-ins?

In both cases, the document URI (which in these scenarios matches the docshell URI) doesn't even have a reasonable hostname (certainly not in the srcdoc case)....

If we want plug-ins to run in those two scenarios, it may make more sense to get the URIs we're interested in out of the principals of the two relevant documents.  But also see below...

> and I'm not sure why we replaced this with mozIThirdPartyUtils.

This comes back to me wanting to understand what actual policy we want to enforce.  The comment in the code talked about "third party" stuff, which has a precise meaning in our codebase, and that meaning is not "not same-origin with parent".  The right way to check "third party" is via mozIThirdPartyUtils.

If the policy we want is based on "not same origin with parent", that's fine, but then we should just check whether our principal is same-origin with the parent document's principal and not mess around with URIs at all for purposes of the same-origin check.  This isn't a technical decision in terms of this implementation code; it's a decision about how we want this feature to behave.  Just looking for a clear requirements spec here.  This is the part I would really like to sort out: what behavior do we want?  Once we have that clear I can just tell you how to implement that behavior.

> I think the suggestion was to use mozIThirdPartyUtils to replace the "IsSubdomainOf" function?

No, not at all.  As you note, it wouldn't do the right thing.

Comment 28

[Benjamin Smedberg]

I may need feedback on the specification (and one purpose of this is to try it and see what it breaks). We have two related goals:

* Security: make it so commonly-embedded domains cannot exploit users by triggering Flash automatically.
* Reduction in Flash prompts: currently when you browse the web with Flash set as click-to-activate, most sites on the internet will prompt you to enable Flash because various advertising/tracking scripts try to use it. In order to switch Flash click-to-activate by default, we need to hard-block Flash on these sites where users don't actually need it.

Currently the target list is:

* common advertising and tracking networks: google adwords/doubleclick/facebook.net advertising (which includes fbcdn.net)/google analytics/omniture
* common injected +1/like buttons: facebook, google plus, twitter
* common 3rd-party utility code that loads in frames: disqus

Because this is a hard block that users cannot override, we want to reduce the risk by applying these rules only when people are browsing other sites: twitter, facebook, etc can still choose to use Flash when the user navigates to that site directly.

> > Do I have to do extra work to normalize/denormalize punycode?
> 
> If you ensure that your list contains punycode versions of things and use
> GetAsciiHost on the nsIURI, that should do what you want.  If the list is
> going to be allowed to contain either non-ASCII or punycode, then some sort
> of normalization would need to be done, yes...

We control the list so we can require it to be punycode.  I'll change this to use GetAsciiHost instead of GetHost.

> But taking a step back here, why is the document or docshell URI the right
> thing here to start with?  Specifically, let's consider the following
> scenarios:
> 
> A) A page at http://foo.com embeds an iframe coming from a blob: URI,
>    which then embeds an iframe from http://foo.com.  If foo.com is
>    on the list, should that innermost iframe be allowed to run plug-ins?

Yes.

> B) A page at http://foo.com embeds an <iframe srcdoc>, which then embeds
>    an iframe from http://foo.com.  If foo.com is on the list, should that
>    innermost iframe be allowed to run plug-ins?

I don't know a lot about srcdoc, but if it's a same-origin document then yes.

> In both cases, the document URI (which in these scenarios matches the
> docshell URI) doesn't even have a reasonable hostname (certainly not in the
> srcdoc case)....

What really matters though is the channel principal: because my call is to nsContentUtils::CheckSameOrigin(nsIChannel*, nsIChannel*) which does a CAPS principal comparison.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> we want to reduce the risk by applying these rules only when people
> are browsing other sites: twitter, facebook, etc can still choose
> to use Flash when the user navigates to that site directly.

OK, so from this point of view, if https://foo.facebook.com is loaded at toplevel and loads an iframe from https://bar.facebook.com which loads an iframe from https://baz.facebook.com, should plug-ins run in the outer iframe?  In the inner iframe?  The right risk mitigation answer would seem to be "yes", right?  Does the answer depend on whether "baz" and "foo" are actually the same thing (but "bar" is a different thing)?

What about the case when https://facebook.com is loaded at toplevel and loads an iframe from https://something-unrelated.com which loads an iframe from https://facebook.com?  Should plug-ins run in that innermost iframe?  My gut feeling is that the answer here is "no", but I don't know enough about the structure of the Facebook and Google web properties to say for sure...

> because my call is to nsContentUtils::CheckSameOrigin(nsIChannel*, nsIChannel*) which does a CAPS principal comparison.

Not quite.  It compares the principal of the first channel to the URI of the second channel.  It's meant to be used before the principal of the second channel is actually known, during HTTP redirects...   Anyway, this is secondary, for the moment, to figuring out the right general policy.

Comment 30

[Benjamin Smedberg]

Our other blocklist is good enough and we have click-to-play.