Closed Bug 781126 Opened 7 years ago Closed 7 years ago

AllowPlugins = false on docshell is no longer working ? | Thunderbird Permanent TEST-UNEXPECTED-FAIL | test-plugins-policy.js | Plugin has not been blocked in message as expected

Categories

(Core :: Plug-ins, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox17 + fixed

People

(Reporter: standard8, Assigned: johns)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

Since bug 745030 landed, we've been getting permanent test failures on Thunderbird:

TEST-START | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test_3paneWindowDenied
Test Failure: Plugin has not been blocked in message as expected
NPP_Destroy
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied

https://tbpl.mozilla.org/php/getParsedLog.php?id=14218472&tree=Thunderbird-Trunk#error0

The regression point is the first changeset of that bug, namely:

http://hg.mozilla.org/mozilla-central/rev/40a8737b62a1
Here's the test for Thunderbird:

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-policy/test-plugins-policy.js

What is happening in the test, and how Thunderbird handles it is this:

- Thunderbird loads an email with a plugin in it.
- nsMsgContentPolicy::ShouldLoad [1] gets called multiple times, but one of those is with a content type of nsIContentPolicy::TYPE_DOCUMENT
- At that point, Thunderbird sets allowPlugins to false on the docshell [2]
- The load of the message then proceeds
- The test calls .setColor() on the plugin element to test if the plugin was loaded or not (similar to some of the core tests)

Expected results:

- Message loaded, but plugin load denied, test pass

Actual results:

- Message loaded, plugin is also loaded and accessible, test fails

In doing debug, I can see that the docshell gets allowPlugins set to false, and a little later the plugin gets loaded, hence why I'm concluding that the docshell blocking appears to not be working any more.

[1] http://hg.mozilla.org/comm-central/annotate/8c305d4656a7/mailnews/base/src/nsMsgContentPolicy.cpp#l161
[2] http://hg.mozilla.org/comm-central/annotate/8c305d4656a7/mailnews/base/src/nsMsgContentPolicy.cpp#l687
Whiteboard: [tb-orange]
I think this should block 17, I believe that this attribute is sometimes used by extensions in Firefox.
Assignee: nobody → jschoenick
Is the problem is that the NS_CheckProcessPolicy call went away?  If so, presumably this regressed bug 393503 as well.  It's unfortunate that I never figured out how to write a test for that one.  :(
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #6)
> Is the problem is that the NS_CheckProcessPolicy call went away?  If so,
> presumably this regressed bug 393503 as well.  It's unfortunate that I never
> figured out how to write a test for that one.  :(

Do you mean NS_CheckContentProcessPolicy ?
Blocks: framesandbox
No longer blocks: 745030
This bug breaks blocking plugins in the patches for iframe sandbox in bug 341604.
I tried to take a crack at this - I added the NS_CheckContentProcessPolicy call back in somewhere around where it used to be, but plugins still aren't blocked and this check actually passes in my test case.

I'm debugging some more to try to understand what's going on :)
Looks like checkURILoad isn't being called by CheckObjectURIs for the plugins that should be blocked, because they have a null mURI, without this, the docshell/document never get asked if plugins are enabled.
It looks like mURI is set for a document with a content type that should be handled by a plugin but not for a plugin loaded by an embed or object..
Oops, didn't mean to remove that this blocks 745030
Blocks: 745030
No longer blocks: framesandbox
Oh bugzilla UI.
Blocks: framesandbox
Comment on attachment 651055 [details] [diff] [review]
Ensure plugins always pass through shouldLoad, add additional check for shouldProcess

So we never pass through shouldLoad() or shouldProcess() with no URI, which is obviously wrong

This checks shouldLoad on the document's base URI when the plugin has no URI of its own, so we can do type based blocking, and adds a shouldProcess call once the final type is known as well. However, due to bug 380556, the latter isn't so productive.
Attachment #651055 - Flags: review?(joshmoz)
Blocks: 309524
Attachment #651055 - Flags: review?(joshmoz) → review+
Calling shouldLoad twice on the same load (as described in bug 309524) isn't right.  We didn't use to do that, iirc; why do we need to do it now?
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #19)
> Calling shouldLoad twice on the same load (as described in bug 309524) isn't
> right.  We didn't use to do that, iirc; why do we need to do it now?

A) We don't know the final type when we first call should load, and we have blockers that, say, always reject TYPE_OBJECT loads - but the tag may well be loading an image
B) shouldProcess is not used properly (see bug 380556 / http://dxr.mozilla.org/mozilla-central/embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp.html#l97 )

If we fixed items blocking on shouldLoad that should be blocking on shouldProcess, and actually used shouldProcess, we could remove the double shouldLoad calls
> We don't know the final type when we first call should load, and we have blockers that,
> say, always reject TYPE_OBJECT loads - but the tag may well be loading an image

The blockers that I know of that always reject TYPE_OBJECT do it because the load can do dangerous things before you even find out whether it's really an image or not.  Which is why we have to always do that first check before we ever call AsyncOpen.

> shouldProcess is not used properly 

Its not used _consistently_ (which is what the bug you cite is about).  As in, we don't call it all the places where we should.  But in this place it _is_ called, and consumers know to look for it here.

And doing shouldLoad twice would actually break those consumers last I checked.

Again, how would just doing the shouldLoad before we asyncOpen and doing shouldProcess before handling the data differ from what we used to do before bug 745030?
Or is the problem you're running into that nsWebBrowserContentPolicy specifically ignores ShouldProcess?  If so, how did this use to work?
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #21)
> The blockers that I know of that always reject TYPE_OBJECT do it because the
> load can do dangerous things before you even find out whether it's really an
> image or not.  Which is why we have to always do that first check before we
> ever call AsyncOpen.

What dangerous things, in this case? We don't begin doing anything with the data until we've re-checked shouldLoad with the proper type. That method also means that trying to block images or subdocuments will fail unless you also block type object.

> Again, how would just doing the shouldLoad before we asyncOpen and doing
> shouldProcess before handling the data differ from what we used to do before
> bug 745030?

> Or is the problem you're running into that nsWebBrowserContentPolicy
> specifically ignores ShouldProcess?  If so, how did this use to work?

Previously we would only ever call shouldProcess for objects that loaded via channels, which many plugins do not - you can load flash without a URI, java never has a URI, etc.. The actual blocking, then, depended on calling shouldLoad() with TYPE_OBJECT - regardless of what type we'd end up loading. Some consumers even tried to work around this, but incorrectly [1]

With the way it works after this patch, we would call shouldLoad() for type object, but if rejected, see if it passes with image or subdocument instead. If the channel then changes our type to something else, we re-call shouldLoad with the proper type. This second shouldLoad call is superfluous, but needed because of the aforementioned nsWebBrowserContentPolicy bug. We finally call shouldProcess once with the proper type (which, for the consumers I checked in the browser, accomplishes nothing, but is proper)

I agree that this is ugly, but the result is that if you have a content policy that responds correctly to shouldLoad or shouldProcess, it will work as expected without tricks like [1], and current arguably-wrong content policy in the browser will continue to work right.

> And doing shouldLoad twice would actually break those consumers last I checked.

shouldLoad seems to me to be a "here's a URI and type, should I load it?" question posed to the API - it is not a guarantee we will immediately load that uri or that we wont ask to load that uri as a different type later -- what consumers would this cause issues with, specifically?


[1] http://dxr.mozilla.org/mozilla-central/extensions/permissions/nsContentBlocker.cpp.html#l176
( this should actually be removed after this patch lands )
> What dangerous things, in this case?

Well, for example <object data="javascript:whatever"> will run the script in the context of the page before it has any idea what the resulting data looks like.

> That method also means that trying to block images or subdocuments will fail unless you
> also block type object.

Right, unless we correctly call ShouldProcess.  I'm not saying this stuff is bug-free, exactly.  ;)

> The actual blocking, then, depended on calling shouldLoad() with TYPE_OBJECT

OK.  And we're still doing that, right?

> we would call shouldLoad() for type object, but if rejected, see if it passes with
> image or subdocument instead.

I'm not sure this is a great idea: it assumes shouldLoad calls are idempotent, and I'm not sure that assumption is correct with things like adblock and whatnot.

> This second shouldLoad call is superfluous, but needed because of the aforementioned
> nsWebBrowserContentPolicy bug.

Ah, so this is only needed because now we do shouldLoad calls with image/subdocument types too before starting the load?  Was that added in bug 745030, or is it being added in this patch?

> what consumers would this cause issues with, specifically?

Various extensions block loads but provide UI to start them later (e.g. flashblock).  So they end up storing various metadata per shouldLoad call and making multiple such calls can confuse them.  I know some extensions ended up having to add code to deal with the extra shouldLoad calls from image preloading...

What I would prefer here is that we move to a model where we call shouldLoad once before we start loading and then shouldProcess once before processing and adjust any in-tree content policy impls that can't deal with that.  And that means calling shouldProcess for all types, not just plug-ins, and for plug-ins in all cases.

If this is too high-risk or has other difficulties, then my second preference would be restoring the call sequence we used to have (which we we know "works" in the sense of not introducing new issues) while we sort those difficulties out.
Attachment #651055 - Attachment is obsolete: true
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

Well lets just fix content policy then

This changes nsWebBrowserContentPolicy to enforce docshell.allowPlugins in shouldProcess, rather than shoudLoad, since the final object type is unknown at that point.

It further changes nsContentBlocker to do the same, removing old ugly hacks that introspected nsObjectLoadingContent to try to "guess" at a type (which was wrong, in many ways)

BONUS: I may have fixed a case wherein click-to-play was being inadvertently bypassed when explicit permissions were set -- nsContentBlocker does not take into account object tags with a null URI (which can happen with java or even weird cases of flash)
Attachment #652646 - Flags: review?(jst)
Comment on attachment 652647 [details] [diff] [review]
Part 2 - nsObjectLoadingContent should call shouldLoad and shouldProcess sanely

With part 1 landed, we're free to just call shouldLoad(TYPE_OBJECT) before opening a URI, and shouldProcess(PROPER_TYPE) afterwards. We also don't need to worry about mURI being null, since content policy handles that correctly now.
Attachment #652647 - Flags: review?(jst)
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

Looks good to me, though I spotted two minor typos below. And we should have bz look these changes over too before we ship this, but feel free to land and follow up with bz after the fact.

- In nsContentBlocker::ShouldLoad():

+  // The final type of an object tag mate mutate before it reaches

s/mate/may/

- In nsContentBlocker::ShouldProcess():

+  // For objects, we only check policy in shouldProcess, as the final type isnt

s/isnt/isn't/
Attachment #652646 - Flags: review?(jst) → review+
Attachment #652647 - Flags: review?(jst) → review+
Attachment #652646 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/85faeeb2306d
https://hg.mozilla.org/mozilla-central/rev/812ea773f166
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

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

::: extensions/permissions/nsContentBlocker.cpp
@@ +212,5 @@
> +    if (!shouldLoad) {
> +      if (fromPrefs) {
> +	*aDecision = nsIContentPolicy::REJECT_TYPE;
> +      } else {
> +	*aDecision = nsIContentPolicy::REJECT_SERVER;

Tabs :(
Also, it seems this has regressed bug 781265. :(
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #48)
> Also, it seems this has regressed bug 781265. :(

The newer regression ranges are now pointing towards bug 775965 for that
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

bz - this landed while you were away to unblock iframe sandboxes, but jst wanted me to ping you and make sure you're okay with this approach
Attachment #652646 - Flags: feedback?(bzbarsky)
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

I think this is OK, as long as nsContentBlocker is not supposed to prevent the network access...

Some nits: "mate mutate" should be "may mutate", "isnt" needs an apostrophe, and the indent on the second line of the TestPermission call in nsContentBlocker::ShouldProcess is somewhat off.

You should probably explicitly reject if aContentLocation is null instead of relying on the undocumented behavior of TestPermission doing so.
Attachment #652646 - Flags: feedback?(bzbarsky) → feedback+
Whiteboard: [tb-orange]
You need to log in before you can comment on or make changes to this bug.