Last Comment Bug 371976 - (CVE-2009-3385) execution of flash in seamonkey mail
(CVE-2009-3385)
: execution of flash in seamonkey mail
Status: RESOLVED FIXED
[sg:vector-critical] mail worms, what...
: fixed-seamonkey2.0, fixed1.9.0.15, verified1.9.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 523895
  Show dependency treegraph
 
Reported: 2007-02-27 10:09 PST by georgi - hopefully not receiving bugspam
Modified: 2015-07-08 05:10 PDT (History)
19 users (show)
mbeltzner: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
reed: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.4+
.4-fixed


Attachments
Does this help? (3.15 KB, patch)
2007-03-01 22:32 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Proposed patch (2.42 KB, patch)
2009-09-24 13:52 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
-pU8 (3.52 KB, patch)
2009-09-24 15:53 PDT, neil@parkwaycc.co.uk
bugs: review+
Details | Diff | Review
With additional assertion (3.69 KB, patch)
2009-09-25 15:01 PDT, neil@parkwaycc.co.uk
neil: review+
jonas: superreview+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
dveditz: approval1.8.1.next+
Details | Diff | Review
1.9.1 branch patch (2.68 KB, patch)
2009-09-29 01:55 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Fixed 1.9.1 branch patch (2.68 KB, patch)
2009-09-29 02:00 PDT, neil@parkwaycc.co.uk
bugs: review+
Details | Diff | Review
With extra goodness ;-) (3.78 KB, patch)
2009-09-29 02:23 PDT, neil@parkwaycc.co.uk
bugs: review+
Details | Diff | Review
1.8.1 branch patch (4.00 KB, patch)
2009-09-29 02:25 PDT, neil@parkwaycc.co.uk
bugs: review+
Details | Diff | Review

Description georgi - hopefully not receiving bugspam 2007-02-27 10:09:33 PST
reading html message in seamonkey containing:
<iframe src='http://server/flash.swf'></html>

causes exection of flash in preview pane if remote content
is shown.
replying to the message causes execution of flash in editor.
flash fails to load javascript uris.
data:application/x-shockwave-flash;base64, uris instantiate
the flash plugin but don't show the content (same applies to
browser, probably a bug).

active content in email in general is not very good idea.
Comment 1 georgi - hopefully not receiving bugspam 2007-02-27 10:11:53 PST
thunderbird seems safe, probably because doesn't recognize that flash is installed.

bug in the description: </html> should be </iframe>
Comment 2 David :Bienvenu 2007-02-27 10:12:26 PST
Thunderbird turns off plugins by default.
Comment 3 georgi - hopefully not receiving bugspam 2007-02-27 14:18:27 PST
(In reply to comment #2)
> Thunderbird turns off plugins by default.
> 

the potential problem is not plugins being enabled by the user.

in seamonkey plugins are also disabled for mailnews and don't seem to work for direct html content, but work in <iframe>.

the potental problem is flash is active content.
Comment 4 neil@parkwaycc.co.uk 2007-02-28 15:28:30 PST
Plugins are the very first thing Thunderbird checks. However, SeaMonkey only wants to block plugins in mail. To do this it seems to check whether the root docshell is a mail docshell. Maybe it's doing this incorrectly?

http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgContentPolicy.cpp#231
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-28 16:03:36 PST
That code is correct.  However in this case the load is not done with TYPE_OBJECT -- it's done with TYPE_SUBDOCUMENT.  At least on trunk.  G30rgi, are you testing branch or trunk?

On trunk, we should add a ShouldProcess call, I think.
Comment 6 georgi - hopefully not receiving bugspam 2007-03-01 01:30:18 PST
(In reply to comment #5)

>G30rgi,
> are you testing branch or trunk?
> 

flash loads on both latest trunk and seamonkey1.1.1.
a simple way to reproduce is compose mail and insert the following html via "insert html"
<iframe src="http://www.flashfunpages.com/couple.swf"></iframe>
loads in editor, in preview pane "show remote content" is needed


Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-03-01 02:44:35 PST
Hmm... yeah, this could be a problem on branch too.

So how about we add a ShouldProcess call in http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsPluginDocument.cpp#92 ?  That seems like the right place for it...
Comment 8 georgi - hopefully not receiving bugspam 2007-03-01 02:49:39 PST
when the flash is <embed>ed it doesn't execute, so bz's suggestion seems nice
Comment 9 georgi - hopefully not receiving bugspam 2007-03-01 02:51:09 PST
mean when it is embeded either in mail or in an iframe
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-03-01 22:32:59 PST
Created attachment 256996 [details] [diff] [review]
Does this help?
Comment 11 georgi - hopefully not receiving bugspam 2007-03-02 02:19:56 PST
(In reply to comment #10)
> Created an attachment (id=256996) [details]
> Does this help?
> 

according to my tests it doesn't help at all. flash is executed as without the patch. do you need a sample local folder with flash (comment #6 is easy to do)?
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-03-02 02:38:10 PST
Oh.  Well, I guess it wouldn't help given that nsMsgContentPolicy::ShouldProcess is:

579 {
580   *aDecision = nsIContentPolicy::ACCEPT;
581   return NS_OK;
582 }

That'll have to be fixed to, to do something more like nsMsgContentPolicy::ShouldLoad.

As for a sample folder, that wouldn't really help much -- I don't have a flash plugin and don't plan to change that.  ;)
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-03-02 02:38:52 PST
Just to be clear, I'm not going to be in a position to do the content policy change, if you want it before mid-April; someone else will need to step up to do that.
Comment 14 georgi - hopefully not receiving bugspam 2007-03-02 02:42:43 PST
i also don't use flash except in some very rare cases like this one ;)

since this is marked as critical probably the option for enabling plugins in mailnews should be removed? 
Comment 15 Robert Kaiser (not working on stability any more) 2007-04-06 11:35:51 PDT
So, do we have _someone_ working on this?
It would be good to get a fix in ASAP for 1.1.x (and trunk as well, of course).
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-04-06 12:58:24 PDT
I'm not working on the content policy end; I don't really know that code.  If someone does, they should step up.
Comment 17 Daniel Veditz [:dveditz] 2007-04-06 14:01:38 PDT
What is the potential exploit here? Merely running the plugin can't be 'critical' because we safely run them in a browsing context. <embed>ed plugins would worry me more since a scriptable kind like flash could load javascript: urls and probably sniff out the content of a reply or forward. If they're loaded as an <iframe src=> doesn't our same-origin policy protect us from that?

Clearly there's privacy implications in loading it at all (webbugs) and the risk of having a larger attack surface, but unless there's more that I'm missing the impact sounds "low" for now.

Of course if the seamonkey compose window is chrome-privileged then it's definitely 'critical'. I sure hope we're not that foolish.
Comment 18 georgi - hopefully not receiving bugspam 2007-04-06 23:18:27 PDT
probably the one who marked this as critical should comment.

flash runs in editor, but couldn't get chrome so far.

by default plugins are blocked in mail and this bug circumvents this.

Comment 19 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-04-07 07:51:14 PDT
Lowering severity based on comment 17.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-04-07 23:26:06 PDT
I think this is in fact critical -- in a browsing context we run flash off sites the user visit.  Here we're running any flash anyone wants to send.
Comment 21 georgi - hopefully not receiving bugspam 2007-04-08 00:13:25 PDT
(In reply to comment #20)
> I think this is in fact critical -- in a browsing context we run flash off
> sites the user visit.  Here we're running any flash anyone wants to send.
> 

i have mixed feelings about this.
don't have an exploit, but *suspect* this may be nasty.

probably it is not only flash - every pluging should do, java tries to start but complains in the java console.

shutdown or mozbugs comment on this?
Comment 22 georgi - hopefully not receiving bugspam 2007-04-08 00:30:02 PDT
potential exploit vector is DOM tricks in editor.

A. dynamically create <img src="file:///etc/passwd">
B. kill a lot of DOM and see if it crashes.

couldn't access DOM from flash though
Comment 23 georgi - hopefully not receiving bugspam 2007-09-21 00:32:21 PDT
is this exploitable via the quicktime qtnext bug?
Comment 24 georgi - hopefully not receiving bugspam 2008-01-31 23:16:20 PST
[sg:investigate] because may be directly exploitable via plugin exploit
Comment 25 Robert Kaiser (not working on stability any more) 2008-09-09 07:26:54 PDT
I wonder if this is still an issue and if it's still critical to SeaMonkey 2 Alpha 1, for which we freeze tonight and which we're planning to release very soon now.
If it is, can we get a fix in please?
Comment 26 georgi - hopefully not receiving bugspam 2008-09-10 00:10:41 PDT
>I wonder if this is still an issue

yes it is
Comment 27 Robert Kaiser (not working on stability any more) 2008-09-11 04:59:46 PDT
Due to the unclarity if it even could be exploited, unblocking Alpha 1 on this, but marking to be on the radars for Alpha 2 and final, as well as clearly a wanted+ for final.
Has someone made sure that Thunderbird is not affected as well, by real tesing? This sounds like it would have potential to even work there.

It would be really nice to have someone to look into that, who could we pull in to work on a fix?
Comment 28 georgi - hopefully not receiving bugspam 2008-09-11 07:03:44 PDT
>Has someone made sure that Thunderbird is not affected as well, by real tesing?

see comment #1
Comment 29 georgi - hopefully not receiving bugspam 2008-09-11 07:31:40 PDT
thunderbird seems safe according to my tests.

it reports flash is installed (i consider this really insane) though flash is not displayed neither in preview nor in editor.

though it is fetched from the server in editor and if remote content is shown.
Comment 30 georgi - hopefully not receiving bugspam 2008-09-11 07:46:54 PDT
so i see at least 2 potential problems here.

1. plugins bugs being exploited via email, c.f. quicktime bug.
2. plugins run in editor when replying/forwarding inline. probably plugins being able to modify editor's DOM will mean file stealing via <img src="file:///">
Comment 31 georgi - hopefully not receiving bugspam 2008-09-12 03:34:26 PDT
same situation on macosx 10.4 ppc. in addition quicktime is executed.
Comment 32 georgi - hopefully not receiving bugspam 2008-09-12 04:07:34 PDT
per mw22 test windoze is affected => ALL
Comment 33 georgi - hopefully not receiving bugspam 2008-09-29 09:09:03 PDT
just for your information Bug 392929 proposes something very close to enabling plugins in thunderbird on purpose.
Comment 34 Robert Kaiser (not working on stability any more) 2008-11-28 05:29:53 PST
I don't see anyone working on this and have no clue who could do it, so setting non-blocking for a2. It'll stay on the request radar for 2.0 final, but I don't know what to do about it. I hoped that something like what's being done for bug 374577 would apply here, but to be honest, I have no clue about it.
Comment 35 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-11-28 20:33:41 PST
Uh...  What part of comment 12 was unclear?

This bug really confuses me, honestly.  I debugged it.  I posted a core patch that's a prerequisite for fixing it.  I pointed out the mailnews-specific code that is also buggy and needs fixing, and sorta explained how to go about fixing it.

What more needs to happen here, short of me fixing the mailnews code myself?
Comment 36 Robert Kaiser (not working on stability any more) 2009-01-31 15:15:37 PST
Ian, Neil tells me you have touched the mailnews content policy code last, could you please look into this? It looks like bz has pointed out the strategy of what to do, but somebody needs to actually implement it.
Comment 37 neil@parkwaycc.co.uk 2009-08-11 13:26:42 PDT
So, I started debugging this, and I found that ShouldLoad does indeed get called with an aContentType of TYPE_OBJECT, with this call stack:

>msgbase!nsMsgContentPolicy::ShouldLoad [mailnews\base\src\nsmsgcontentpolicy.cpp @ 215]
>gklayout!nsContentPolicy::CheckPolicy+0x2dc [mozilla\content\base\src\nscontentpolicy.cpp @ 157]
>gklayout!nsContentPolicy::ShouldLoad+0x5b [mozilla\content\base\src\nscontentpolicy.cpp @ 218]
>gklayout!NS_CheckContentLoadPolicy+0x213 [mozilla\content\base\public\nscontentpolicyutils.h @ 221]
>gklayout!nsObjectLoadingContent::LoadObject+0x4b7 [mozilla\content\base\src\nsobjectloadingcontent.cpp @ 1111]
>gklayout!nsObjectLoadingContent::LoadObject+0x1ca [mozilla\content\base\src\nsobjectloadingcontent.cpp @ 976]
>gklayout!nsHTMLSharedObjectElement::StartObjectLoad+0x99 [mozilla\content\html\content\src\nshtmlsharedobjectelement.cpp @ 434]
>gklayout!nsHTMLSharedObjectElement::StartObjectLoad+0x11 [mozilla\content\html\content\src\nshtmlsharedobjectelement.cpp @ 133]
>gklayout!nsRunnableMethod<nsHTMLSharedObjectElement,void>::Run+0x22 [mozilla\xpcom\glue\nsthreadutils.h @ 265]
>gklayout!nsContentUtils::RemoveScriptBlocker+0xe7 [mozilla\content\base\src\nscontentutils.cpp @ 4448]
>gklayout!nsContentUtils::RemoveRemovableScriptBlocker+0x43 [mozilla\content\base\public\nscontentutils.h @ 1391]
>gklayout!mozAutoDocUpdate::~mozAutoDocUpdate+0x45 [mozilla\content\base\src\mozautodocupdate.h @ 71]
>gklayout!nsGenericElement::doInsertChildAt+0x5d5 [mozilla\content\base\src\nsgenericelement.cpp @ 3237]
>gklayout!nsGenericElement::InsertChildAt+0x55 [mozilla\content\base\src\nsgenericelement.cpp @ 3151]
>gklayout!nsINode::AppendChildTo+0x26 [mozilla\content\base\public\nsinode.h @ 435]
>gklayout!nsPluginDocument::CreateSyntheticPluginDocument+0x4d8 [mozilla\content\html\document\src\nsplugindocument.cpp @ 314]
>gklayout!nsPluginDocument::StartDocumentLoad+0x76 [mozilla\content\html\document\src\nsplugindocument.cpp @ 235]
>gklayout!nsContentDLF::CreateDocument+0x124 [mozilla\layout\build\nscontentdlf.cpp @ 440]
>gklayout!nsContentDLF::CreateInstance+0x5f6 [mozilla\layout\build\nscontentdlf.cpp @ 297]
>docshell!nsDocShell::NewContentViewerObj+0x177 [mozilla\docshell\base\nsdocshell.cpp @ 7130]
>docshell!nsDocShell::CreateContentViewer+0x88 [mozilla\docshell\base\nsdocshell.cpp @ 6975]
>docshell!nsDSURIContentListener::DoContent+0x130 [mozilla\docshell\base\nsdsuricontentlistener.cpp @ 138]
>docshell!nsDocumentOpenInfo::TryContentListener+0x2e4 [mozilla\uriloader\base\nsuriloader.cpp @ 736]
>docshell!nsDocumentOpenInfo::DispatchContent+0x59c [mozilla\uriloader\base\nsuriloader.cpp @ 434]
>docshell!nsDocumentOpenInfo::OnStartRequest+0x1c0 [mozilla\uriloader\base\nsuriloader.cpp @ 280]
>necko!nsHttpChannel::CallOnStartRequest+0x2ed [mozilla\netwerk\protocol\http\src\nshttpchannel.cpp @ 849]
>necko!nsHttpChannel::OnStartRequest+0x2df [mozilla\netwerk\protocol\http\src\nshttpchannel.cpp @ 5112]
>necko!nsInputStreamPump::OnStateStart+0x9f [mozilla\netwerk\base\src\nsinputstreampump.cpp @ 439]
>necko!nsInputStreamPump::OnInputStreamReady+0x70 [mozilla\netwerk\base\src\nsinputstreampump.cpp @ 395]
>xpcom_core!nsInputStreamReadyEvent::Run+0x48 [mozilla\xpcom\io\nsstreamutils.cpp @ 113]
>xpcom_core!nsThread::ProcessNextEvent+0x1f3 [mozilla\xpcom\threads\nsthread.cpp @ 528]
>xpcom_core!NS_ProcessNextEvent_P+0x51 [mozilla\xpcom\glue\nsthreadutils.cpp @ 230]
>gkwidget!nsBaseAppShell::Run+0x5d [mozilla\widget\src\xpwidgets\nsbaseappshell.cpp @ 170]
>tkitcmps!nsAppStartup::Run+0x69 [mozilla\toolkit\components\startup\src\nsappstartup.cpp @ 193]
>xul!XRE_main+0x2a13 [mozilla\toolkit\xre\nsapprunner.cpp @ 3392]
>seamonkey!NS_internal_main+0x111 [suite\app\nssuiteapp.cpp @ 103]
>seamonkey!wmain+0x101 [mozilla\toolkit\xre\nswindowswmain.cpp @ 108]
>seamonkey!wmainCRTStartup+0x12c
>kernel32!BaseProcessStart+0x23

However, the subsequent call to NS_CP_GetDocShellFromContext fails because the plugin document's script global doesn't get called until later:

>gklayout!nsDocument::SetScriptGlobalObject+0x116 [mozilla\content\base\src\nsdocument.cpp @ 3543]
>gklayout!nsPluginDocument::SetScriptGlobalObject+0x29 [mozilla\content\html\document\src\nsplugindocument.cpp @ 200]
>gklayout!nsGlobalWindow::SetNewDocument+0x172e [mozilla\dom\base\nsglobalwindow.cpp @ 1947]
>gklayout!nsGlobalWindow::SetNewDocument+0x1d [mozilla\dom\base\nsglobalwindow.cpp @ 1510]
>gklayout!DocumentViewerImpl::InitInternal+0x521 [mozilla\layout\base\nsdocumentviewer.cpp @ 953]
>gklayout!DocumentViewerImpl::Init+0x1b [mozilla\layout\base\nsdocumentviewer.cpp @ 697]
>docshell!nsDocShell::SetupNewViewer+0xe85 [mozilla\docshell\base\nsdocshell.cpp @ 7271]
>docshell!nsDocShell::Embed+0x35 [mozilla\docshell\base\nsdocshell.cpp @ 5479]
>docshell!nsDocShell::CreateContentViewer+0x47f [mozilla\docshell\base\nsdocshell.cpp @ 7057]
>docshell!nsDSURIContentListener::DoContent+0x130 [mozilla\docshell\base\nsdsuricontentlistener.cpp @ 138]
>docshell!nsDocumentOpenInfo::TryContentListener+0x2e4 [mozilla\uriloader\base\nsuriloader.cpp @ 736]
>docshell!nsDocumentOpenInfo::DispatchContent+0x59c [mozilla\uriloader\base\nsuriloader.cpp @ 434]
>docshell!nsDocumentOpenInfo::OnStartRequest+0x1c0 [mozilla\uriloader\base\nsuriloader.cpp @ 280]
>necko!nsHttpChannel::CallOnStartRequest+0x2ed [mozilla\netwerk\protocol\http\src\nshttpchannel.cpp @ 849]
>necko!nsHttpChannel::OnStartRequest+0x2df [mozilla\netwerk\protocol\http\src\nshttpchannel.cpp @ 5112]
>necko!nsInputStreamPump::OnStateStart+0x9f [mozilla\netwerk\base\src\nsinputstreampump.cpp @ 439]
>necko!nsInputStreamPump::OnInputStreamReady+0x70 [mozilla\netwerk\base\src\nsinputstreampump.cpp @ 395]
>xpcom_core!nsInputStreamReadyEvent::Run+0x48 [mozilla\xpcom\io\nsstreamutils.cpp @ 113]
>xpcom_core!nsThread::ProcessNextEvent+0x1f3 [mozilla\xpcom\threads\nsthread.cpp @ 528]
>xpcom_core!NS_ProcessNextEvent_P+0x51 [mozilla\xpcom\glue\nsthreadutils.cpp @ 230]
>gkwidget!nsBaseAppShell::Run+0x5d [mozilla\widget\src\xpwidgets\nsbaseappshell.cpp @ 170]
>tkitcmps!nsAppStartup::Run+0x69 [mozilla\toolkit\components\startup\src\nsappstartup.cpp @ 193]
>xul!XRE_main+0x2a13 [mozilla\toolkit\xre\nsapprunner.cpp @ 3392]
>seamonkey!NS_internal_main+0x111 [suite\app\nssuiteapp.cpp @ 103]
>seamonkey!wmain+0x101 [mozilla\toolkit\xre\nswindowswmain.cpp @ 108]
>seamonkey!wmainCRTStartup+0x12c
>kernel32!BaseProcessStart+0x23

So either the plugin should get instantiated after the document gets its script global object, or NS_CP_GetDocShellFromContext should get the docshell from the document's container (which fixes this bug), or we need to work around it.
Comment 38 Johnny Stenback (:jst, jst@mozilla.com) 2009-09-08 17:41:29 PDT
How come nsPluginDocument::CreateSyntheticPluginDocument()'s check for a "messagepane" docshell doesn't catch this?

http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsPluginDocument.cpp#258

Regarding making NS_CP_GetDocShellFromContext() get the document from the document container... I don't think we want to do that since that would still let the content policy check happen as early on as it is happening here, which is before the document has a global window. Passing a document, or any node, for which there is no window object yet out to JS doesn't seem like something we want to do.

Seems to me that bz's patch, in combination with code that would prevent nsObjectLoadingContent from doing a content policy check in this particular case would be what we want here. What we're looking at here is a network response that's already coming in and happens to be something that's handled by a plugin, in that case we don't want the nsObjectLoadingContent code to start a new load (and I'd guess it doesn't today), we want it to just initiate itself enough and let the plugin load happen through the stream that's already on its way in...
Comment 39 neil@parkwaycc.co.uk 2009-09-09 04:47:13 PDT
(In reply to comment #38)
> How come nsPluginDocument::CreateSyntheticPluginDocument()'s check for a
> "messagepane" docshell doesn't catch this?
> 
> http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsPluginDocument.cpp#258
As I said in my email, it's because it's loaded in a frame.

> Regarding making NS_CP_GetDocShellFromContext() get the document from the
> document container... I don't think we want to do that since that would still
> let the content policy check happen as early on as it is happening here, which
> is before the document has a global window. Passing a document, or any node,
> for which there is no window object yet out to JS doesn't seem like something
> we want to do.
Then I'd appreciate it if you could fix it so that that doesn't happen!
Comment 40 Jonas Sicking (:sicking) PTO Until July 5th 2009-09-16 16:43:29 PDT
Would this work:

Remove the line at
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7118

and add the following code:
docv->SetContainer(aContainer);

at line
http://mxr.mozilla.org/mozilla-central/source/layout/build/nsContentDLF.cpp#436

I.e. set the container on the doc viewer at the same time as we set it on the document.

(Why does both documents and documentviewers have a container, seems like a source of bugs, like this one)
Comment 41 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-16 17:52:51 PDT
It's possible to have document viewers not tied to a document (specifically to change the document in a viewer) and vice versa....  I'd be fine with just storing the container in the document, thought.
Comment 42 neil@parkwaycc.co.uk 2009-09-17 03:25:46 PDT
(In reply to comment #40)
> Would this work:
> 
> Remove the line at
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7118
That just causes assertions. I restored the line to make my build work again.

> and add the following code:
> docv->SetContainer(aContainer);
> 
> at line
> http://mxr.mozilla.org/mozilla-central/source/layout/build/nsContentDLF.cpp#436
Sorry, but that has no effect; the plugin document still has no script global.
Comment 43 neil@parkwaycc.co.uk 2009-09-24 13:52:51 PDT
Created attachment 402655 [details] [diff] [review]
Proposed patch

So, it turns out that the lack of a script global object was really bad. So bad in fact, that none of our plugin content policies such as the one that triggers when docshell.allowPlugins is false, were getting applied to media documents.

This moves code around to make plugin documents act more like image documents.
Comment 44 Robert Kaiser (not working on stability any more) 2009-09-24 14:22:03 PDT
Given the finding of Neil that "none of our plugin content policies [...] were getting applied to media documents", I think this should block 1.9.1.4 actually.
Comment 45 Reed Loden [:reed] (use needinfo?) 2009-09-24 14:29:00 PDT
Considering the patches are both in core code, what component does this bug really belong in?
Comment 46 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-24 14:34:15 PDT
Probably Core:DOM.
Comment 47 Jonas Sicking (:sicking) PTO Until July 5th 2009-09-24 15:02:20 PDT
Can you diff this with -Pu8 please?
Comment 48 neil@parkwaycc.co.uk 2009-09-24 15:53:26 PDT
Created attachment 402697 [details] [diff] [review]
-pU8
Comment 49 Daniel Veditz [:dveditz] 2009-09-25 11:07:43 PDT
We're technically past code-freeze for 1.9.[01] branches, but this seems pretty critical given the high adoption and bug rate of Flash and other plugins.

Do we really block plugins by docshellitem name? What if some other xul app happens to use that name in a non-mail setting? I thought we used app-type for this kind of check.
Comment 50 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-25 11:20:01 PDT
We get the app-type from the docshell; that first hunk in the patch should make that work, I'd think.

Can we just remove the messagepane hack, given that?
Comment 51 Jonas Sicking (:sicking) PTO Until July 5th 2009-09-25 12:11:53 PDT
Comment on attachment 402697 [details] [diff] [review]
-pU8

Smaug has graciously offered to help!
Comment 52 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-09-25 12:55:47 PDT
So why can't we iterate ancestor docshells in CreateSyntheticPluginDocument and
check if some of them is APP_TYPE_MAIL?
(Or is it enough to check only the root docshell)
Comment 53 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-09-25 13:08:27 PDT
(In reply to comment #44)
> Given the finding of Neil that "none of our plugin content policies [...] were
> getting applied to media documents",
Or is this bug now about this and not so much about the initial problem that
plugin document may load flash in Seamonkey-mail?

Basically, are we trying to change content policy or plugin document handling?
Comment 54 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-09-25 13:17:20 PDT
I guess both problems get fixed with the patch, if mailnews content policy check
works.

The messagepane hack is ugly. Does mailnews really open some helper app
if a plugin is tried to be loaded as a message (how to even do that)?
Comment 55 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-09-25 13:31:08 PDT
Comment on attachment 402697 [details] [diff] [review]
-pU8

Still, r=me, but please file a followup to fix messagepane handling.
It has been there for ages (since Bug 90256), but better to fix it.

And could you add an assertion to CreateSyntheticDocument to check that either we don't have presshell yet or it hasn't done the initial reflow yet.
(CreateSyntheticPluginDocument doesn't notify, so it should be called early enough.)
Comment 56 Daniel Holbert [:dholbert] 2009-09-25 14:23:57 PDT
This landed as:
http://hg.mozilla.org/mozilla-central/rev/3ef7c0074486
but it broke the build & needed this bustage fix:
http://hg.mozilla.org/mozilla-central/rev/1aaedcad1634

(Should this be RESOLVED|FIXED? Perhaps once the followup bug from comment 55 is filed?)
Comment 57 neil@parkwaycc.co.uk 2009-09-25 15:01:14 PDT
Created attachment 402931 [details] [diff] [review]
With additional assertion
Comment 58 neil@parkwaycc.co.uk 2009-09-26 10:17:28 PDT
Comment on attachment 402931 [details] [diff] [review]
With additional assertion

Let's try this again now that I'm awake.
Comment 59 neil@parkwaycc.co.uk 2009-09-26 10:19:47 PDT
Comment on attachment 402931 [details] [diff] [review]
With additional assertion

This patch is needed to delay synthetic plugin document content creation until content policy (including Firefox content policy) is capable of dealing with it. Without this patch it is difficult to block plugin content that is served as a document e.g. as the source of a frame.
Comment 60 georgi - hopefully not receiving bugspam 2009-09-28 04:18:25 PDT
note that as per the description editor displays the flash when replying too so it isn't just the messagepane
Comment 61 neil@parkwaycc.co.uk 2009-09-28 05:02:59 PDT
(In reply to comment #60)
> note that as per the description editor displays the flash when replying too so
> it isn't just the messagepane
Same core bug though; editor's use of content policy is getting ignored.
Comment 62 georgi - hopefully not receiving bugspam 2009-09-28 05:12:47 PDT
>Same core bug though; editor's use of content policy is getting ignored.

ok, never mind. wrote this because of the hardcoded "messagepane" in the patch.
Comment 63 neil@parkwaycc.co.uk 2009-09-28 05:26:27 PDT
(In reply to comment #62)
> >Same core bug though; editor's use of content policy is getting ignored.
> ok, never mind. wrote this because of the hardcoded "messagepane" in the patch.
Which only exists because I need to hoist that hunk up to the caller so that I can call the rest of method at a safer time.
Comment 64 Daniel Veditz [:dveditz] 2009-09-28 12:36:46 PDT
Comment on attachment 402931 [details] [diff] [review]
With additional assertion

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz

Approved for 1.8.1.24, a=dveditz
Comment 65 neil@parkwaycc.co.uk 2009-09-28 16:07:46 PDT
Pushed changeset bb94e4ce52a4 to releases/mozilla-1.9.1

Fix checked in to CVS HEAD and MOZILLA_1_8_BRANCH.
Comment 66 neil@parkwaycc.co.uk 2009-09-28 16:34:54 PDT
And backed out again because of smaug's assertion :-(

[I don't think I can change the 1.9.1 flag back to wanted though]
Comment 67 Daniel Holbert [:dholbert] 2009-09-28 17:40:25 PDT
Specifically, this failed on 1.9.1 & earlier because the "DidInitialReflow" method has been renamed (in Bug 468645) since those branches.

In those older branches, this method is called "GetDidInitialReflow", and it returns its result via an outparam:
http://mxr.mozilla.org/firefox/source/layout/base/nsIPresShell.h?mark=277-277#272
http://mxr.mozilla.org/mozilla1.9.1/source/layout/base/nsIPresShell.h?mark=277-277#272
Comment 68 Daniel Holbert [:dholbert] 2009-09-28 17:46:33 PDT
(In reply to comment #66)
> [I don't think I can change the 1.9.1 flag back to wanted though]

Yeah -- that's kinda sucky, but I don't think it's a huge issue here -- this bug has blocking1.9.1=".4+", which is stronger than status1.9.1="wanted".

Hence, I'm clearing "status1.9.1", since this was backed out and is no longer ".4-fixed" (though hopefully it will be again, soon :) ).
Comment 69 neil@parkwaycc.co.uk 2009-09-29 01:55:15 PDT
Created attachment 403444 [details] [diff] [review]
1.9.1 branch patch
Comment 70 neil@parkwaycc.co.uk 2009-09-29 02:00:15 PDT
Created attachment 403446 [details] [diff] [review]
Fixed 1.9.1 branch patch

Compiling isn't enough!
Comment 71 neil@parkwaycc.co.uk 2009-09-29 02:23:13 PDT
Created attachment 403447 [details] [diff] [review]
With extra goodness ;-)
Comment 72 neil@parkwaycc.co.uk 2009-09-29 02:25:57 PDT
Created attachment 403448 [details] [diff] [review]
1.8.1 branch patch

Not a significant merge but requesting review for completeness.
Comment 73 neil@parkwaycc.co.uk 2009-09-29 03:11:30 PDT
Pushed changeset 51d39b428a2d to releases/mozilla-1.9.2

Pushed changeset fe6e8f6b22ba to releases/mozilla-1.9.1

Fix rechecked in to CVS HEAD and MOZILLA_1_8_BRANCH.
Comment 74 georgi - hopefully not receiving bugspam 2009-09-29 04:18:21 PDT
btw, the extension "flashblock" seems to stop this for firefox in <iframe src="flash.swf" and opening flash from locationbar
Comment 75 Al Billings [:abillings] 2009-09-30 16:48:23 PDT
When I test this with an iframe in a message in the 9/30 Seamonkey 1.9.1 build, I get a prompt to download the .swf file. Should this file even be getting retrieved at all?
Comment 76 neil@parkwaycc.co.uk 2009-09-30 16:58:45 PDT
This fix only applies to the case where the frame's source is detected being handled by a plugin, so the browser actually has to have Flash installed and configured so that opening a .swf file will play it.

I'm not actually sure at this time of night as it is here what the expected behaviour is for unknown content served in a frame.
Comment 77 Al Billings [:abillings] 2009-09-30 17:07:47 PDT
Verified for 1.9.1 (after reinstalling Flash) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090930 SeaMonkey/2.0pre.

There are no 1.9.0-based builds to verify this for 1.9.0.15.
Comment 78 georgi - hopefully not receiving bugspam 2009-10-01 02:56:44 PDT
> Should this file even be getting retrieved at all?

i need to "show remote content" to retrieve the file.
Comment 79 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-12 07:38:44 PST
Comment on attachment 402931 [details] [diff] [review]
With additional assertion

Did this go in to mozilla-1.9.2? I don't see a checkin comment, but I notice that it's been marked as fixed.

(Removing approval request as this is a blocker)
Comment 80 neil@parkwaycc.co.uk 2009-11-13 05:15:16 PST
(In reply to comment #79)
> (From update of attachment 402931 [details] [diff] [review])
> Did this go in to mozilla-1.9.2? I don't see a checkin comment
Comment #73?
Comment 81 Bob Clary [:bc:] 2009-11-18 20:50:19 PST
Timothy Nikkel (:tn) tnikkel@gmail.com thinks this is responsible for the crash in bug 523895 comment 7 / bug 523895 comment 8.
Comment 82 Al Billings [:abillings] 2010-02-18 16:23:58 PST
I just tested this in the nightly Seamonkey 1.1.19pre build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.24pre) Gecko/20100218 SeaMonkey/1.1.19pre). This isn't fixed. When I insert the iframe with flash source, it plays in the editor and it plays if I show remote content after I send it to myself.

So, someone should figure out why the fix doesn't work on 1.8.1.
Comment 83 neil@parkwaycc.co.uk 2010-03-03 16:13:27 PST
I don't actually see the content policy getting called on the 1.8 branch.

That is to say, I put a breakpoint on nsWebBrowserContentPolicy::ShouldLoad but it was never called with a TYPE_OBJECT content type.
Comment 84 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-03 20:11:04 PST
On 1.8.1 <object> loading works completely differently than in 1.9.

That said, the relevant codepath is nsPluginHostImpl::InstantiateEmbeddedPlugin, which does do a content policy check.  Why are you not seeing that call?
Comment 85 neil@parkwaycc.co.uk 2010-03-04 01:16:24 PST
(In reply to comment #84)
> That said, the relevant codepath is
> nsPluginHostImpl::InstantiateEmbeddedPlugin, which does do a content policy
> check.  Why are you not seeing that call?
My breakpoint on nsPluginHostImpl::InstantiateEmbeddedPlugin does not get hit.
Comment 86 Robert Kaiser (not working on stability any more) 2010-03-04 04:48:33 PST
Note that we already did cut builds for SeaMonkey 1.1.19 and are planning to EOL the whole 1.x series with that one. We also will not claim that 1.1.19 fixes all known security bugs, just that it is better than 1.1.18 - all security issues are only fixed on 2.x builds.
Due to all that, I'm not sure how high the value of trying to fix this on 1.8.1.x is right now.
Comment 87 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-04 07:17:17 PST
> My breakpoint on nsPluginHostImpl::InstantiateEmbeddedPlugin does not get hit.

Oh, this is a full-page plugin, isn't it?  Right.

Looking over this code again, I don't understand why your earlier patch without the addition of my original patch in this bug fixed anything.  It would block an attempt by the nsObjectLoadingContent to instantiate from LoadObject, but that's not what sets up the plugin.

I think you really do need my original patch in this bug to fix the issue on 1.8, and as far as I can see to fix it on anything else as well.
Comment 88 neil@parkwaycc.co.uk 2010-03-04 07:25:17 PST
(In reply to comment #87)
> Looking over this code again, I don't understand why your earlier patch without
> the addition of my original patch in this bug fixed anything.  It would block
> an attempt by the nsObjectLoadingContent to instantiate from LoadObject, but
> that's not what sets up the plugin.
Then what does? (I guess I could breakpoint trunk and reverse engineer...)

> I think you really do need my original patch in this bug to fix the issue on
> 1.8, and as far as I can see to fix it on anything else as well.
Not needed on 1.9+, because the bug there was that ShouldLoad was called at a point where NS_CP_GetDocShellFromContext failed.
Comment 89 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-04 07:44:48 PST
> Then what does? 

nsPluginStreamListener::SetupPlugin

> ShouldLoad was called at a point where NS_CP_GetDocShellFromContext failed

Yes, but that ShouldLoad call comes before code that actually does nothing in the full-page plugin case, usually. So I don't see why the patch that was checked in helped anything.
Comment 90 neil@parkwaycc.co.uk 2010-03-04 08:36:29 PST
(In reply to comment #84)
> That said, the relevant codepath is nsPluginHostImpl::InstantiateEmbeddedPlugin
Actually it isn't, it's nsPluginHostImpl::InstantiateFullPagePlugin, which doesn't do any content policy checks at all :-(
Comment 91 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-04 08:59:54 PST
Right; my initial patch in this bug added a check on the codepath that leads to that code.
Comment 92 neil@parkwaycc.co.uk 2010-03-04 09:06:35 PST
I'm failing to see the relevance, since that's adding a ShouldProcess call that nobody (e.g. nsWebBrowserContentPolicy::ShouldProcess) cares about.
Comment 93 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-04 09:12:57 PST
Well, the point is that it should be caring about it, if that's the content policy that you rely on to stop the plug-in instantiation.  The load that actually loads the plug-in does not go through ShouldLoad with TYPE_OBJECT for full-page plug-ins.
Comment 94 neil@parkwaycc.co.uk 2010-03-04 09:21:43 PST
(In reply to comment #93)
> Well, the point is that it should be caring about it, if that's the content
> policy that you rely on to stop the plug-in instantiation.  The load that
> actually loads the plug-in does not go through ShouldLoad with TYPE_OBJECT for
> full-page plug-ins.
I guess trunk works differently then - nsObjectLoadingContent::LoadObject makes the ShouldLoad(TYPE_OBJECT, ...) check.
Comment 95 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-04 09:31:52 PST
Yes, but for full-page plug-ins it doesn't actually do the load!  So the results of that check are simply ignored.
Comment 96 neil@parkwaycc.co.uk 2010-03-04 09:39:55 PST
I don't know what you mean by ignored; the result of NS_CheckContentLoadPolicy is used to determine whether to call HandleBeingBlockedByContentPolicy.
Comment 97 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-03-04 09:55:55 PST
Oh, I see.  So we keep doing the load but never end up with an object frame and so never instantiate.  OK.  That explains why it works (Totally by accident!) in 1.9 and above.
Comment 98 neil@parkwaycc.co.uk 2010-03-04 10:04:34 PST
In that case I'd better remove the branch fix keyword.

Note You need to log in before you can comment on or make changes to this bug.