371976 - (CVE-2009-3385) execution of flash in seamonkey mail

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[georgi - hopefully not receiving bugspam]

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]

thunderbird seems safe, probably because doesn't recognize that flash is installed.

bug in the description: </html> should be </iframe>

Comment 2

[David :Bienvenu]

Thunderbird turns off plugins by default.

Comment 3

[georgi - hopefully not receiving bugspam]

(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

[[e-mail]]

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 [:bzbarsky]]

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]

(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 [:bzbarsky]]

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]

when the flash is <embed>ed it doesn't execute, so bz's suggestion seems nice

Comment 9

[georgi - hopefully not receiving bugspam]

mean when it is embeded either in mail or in an iframe

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Does this help?

Comment 11

[georgi - hopefully not receiving bugspam]

(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 [:bzbarsky]]

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 [:bzbarsky]]

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]

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]

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 [:bzbarsky]]

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]]

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]

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 [e-mail] [e-mail]]]

Lowering severity based on comment 17.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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]

(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]

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]

is this exploitable via the quicktime qtnext bug?

Comment 24

[georgi - hopefully not receiving bugspam]

[sg:investigate] because may be directly exploitable via plugin exploit

Comment 25

[Robert Kaiser]

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]

>I wonder if this is still an issue

yes it is

Comment 27

[Robert Kaiser]

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]

>Has someone made sure that Thunderbird is not affected as well, by real tesing?

see comment #1

Comment 29

[georgi - hopefully not receiving bugspam]

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]

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]

same situation on macosx 10.4 ppc. in addition quicktime is executed.

Comment 32

[georgi - hopefully not receiving bugspam]

per mw22 test windoze is affected => ALL

Comment 33

[georgi - hopefully not receiving bugspam]

just for your information Bug 392929 proposes something very close to enabling plugins in thunderbird on purpose.

Comment 34

[Robert Kaiser]

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 [:bzbarsky]]

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]

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

[[e-mail]]

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)]

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

[[e-mail]]

(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) No longer reading bugmail consistently]

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 [:bzbarsky]]

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

[[e-mail]]

(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

[[e-mail]]

Attachment: 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]

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]]

Considering the patches are both in core code, what component does this bug really belong in?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Probably Core:DOM.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can you diff this with -Pu8 please?

Comment 48

[[e-mail]]

Attachment: -pU8

Comment 49

[Daniel Veditz [:dveditz]]

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 [:bzbarsky]]

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) No longer reading bugmail consistently]

Comment on attachment 402697 [details] [diff] [review]
-pU8

Smaug has graciously offered to help!

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

(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][[e-mail]]]

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][[e-mail]]]

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]]

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

[[e-mail]]

Attachment: With additional assertion

Comment 58

[[e-mail]]

Comment on attachment 402931 [details] [diff] [review]
With additional assertion

Let's try this again now that I'm awake.

Comment 59

[[e-mail]]

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]

note that as per the description editor displays the flash when replying too so it isn't just the messagepane

Comment 61

[[e-mail]]

(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]

>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

[[e-mail]]

(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]]

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

[[e-mail]]

Pushed changeset bb94e4ce52a4 to releases/mozilla-1.9.1

Fix checked in to CVS HEAD and MOZILLA_1_8_BRANCH.

Comment 66

[[e-mail]]

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]]

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]]

(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

[[e-mail]]

Attachment: 1.9.1 branch patch

Comment 70

[[e-mail]]

Attachment: Fixed 1.9.1 branch patch

Compiling isn't enough!

Comment 71

[[e-mail]]

Attachment: With extra goodness ;-)

Comment 72

[[e-mail]]

Attachment: 1.8.1 branch patch

Not a significant merge but requesting review for completeness.

Comment 73

[[e-mail]]

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]

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 - ex-MoCo]]

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

[[e-mail]]

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 - ex-MoCo]]

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]

> 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]]

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

[[e-mail]]

(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] (inactive)]

Timothy Nikkel (:tn) [e-mail] thinks this is responsible for the crash in bug 523895 comment 7 / bug 523895 comment 8.

Comment 82

[Al Billings [:abillings - ex-MoCo]]

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

[[e-mail]]

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 [:bzbarsky]]

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

[[e-mail]]

(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]

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 [:bzbarsky]]

> 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

[[e-mail]]

(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 [:bzbarsky]]

> 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

[[e-mail]]

(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 [:bzbarsky]]

Right; my initial patch in this bug added a check on the codepath that leads to that code.

Comment 92

[[e-mail]]

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 [:bzbarsky]]

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

[[e-mail]]

(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 [:bzbarsky]]

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

[[e-mail]]

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 [:bzbarsky]]

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

[[e-mail]]

In that case I'd better remove the branch fix keyword.