Bug 371976 (CVE-2009-3385)

execution of flash in seamonkey mail

RESOLVED FIXED

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed-seamonkey2.0, fixed1.9.0.15, verified1.9.1})

Trunk
fixed-seamonkey2.0, fixed1.9.0.15, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.0.15 +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

(Whiteboard: [sg:vector-critical] mail worms, what fun!)

Attachments

(5 attachments, 3 obsolete attachments)

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.
thunderbird seems safe, probably because doesn't recognize that flash is installed.

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

Comment 2

11 years ago
Thunderbird turns off plugins by default.
(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.
Assignee: mail → neil
Component: MailNews: Notification → MailNews: Main Mail Window
Flags: blocking-seamonkey1.5a?
Flags: blocking-seamonkey1.1.2?
Flags: blocking-seamonkey1.0.9?
(Assignee)

Comment 4

11 years ago
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
Severity: normal → critical
Flags: blocking-seamonkey1.5a? → blocking-seamonkey1.5a+
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.
(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


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...
when the flash is <embed>ed it doesn't execute, so bz's suggestion seems nice
mean when it is embeded either in mail or in an iframe
Created attachment 256996 [details] [diff] [review]
Does this help?
(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)?
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.  ;)
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.
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

10 years ago
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).
I'm not working on the content policy end; I don't really know that code.  If someone does, they should step up.
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.
Whiteboard: [sg:low]
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.

Lowering severity based on comment 17.
Severity: critical → normal
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.
(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?
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
is this exploitable via the quicktime qtnext bug?

Updated

10 years ago
Flags: blocking-seamonkey1.1.6?
Flags: blocking-seamonkey1.1.2?
Flags: blocking-seamonkey1.0.9?
Flags: blocking-seamonkey1.0.10?

Updated

10 years ago
Flags: blocking-seamonkey1.0.10?
[sg:investigate] because may be directly exploitable via plugin exploit
Whiteboard: [sg:low] → [sg:low] [sg:investigate]

Comment 25

9 years ago
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?
>I wonder if this is still an issue

yes it is

Comment 27

9 years ago
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?
Flags: wanted-seamonkey2+
Flags: blocking-seamonkey2?
Flags: blocking-seamonkey2.0a2?
Flags: blocking-seamonkey2.0a1-
Flags: blocking-seamonkey2.0a1+
Flags: blocking-seamonkey1.1.8?
Flags: blocking-seamonkey1.1.13?
>Has someone made sure that Thunderbird is not affected as well, by real tesing?

see comment #1
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.
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:///">
same situation on macosx 10.4 ppc. in addition quicktime is executed.
OS: Linux → All
Whiteboard: [sg:low] [sg:investigate] → [sg:investigate]
per mw22 test windoze is affected => ALL

Updated

9 years ago
OS: All → Linux

Updated

9 years ago
OS: Linux → All
Hardware: PC → All
just for your information Bug 392929 proposes something very close to enabling plugins in thunderbird on purpose.

Comment 34

9 years ago
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.
Flags: blocking-seamonkey2.0a2?
Flags: blocking-seamonkey2.0a2-
Flags: blocking-seamonkey1.1.13?
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

9 years ago
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.
(Assignee)

Comment 37

8 years ago
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.
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...
(Assignee)

Comment 39

8 years ago
(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!

Updated

8 years ago
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
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)
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.
(Assignee)

Comment 42

8 years ago
(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.
(Assignee)

Comment 43

8 years ago
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.
Attachment #402655 - Flags: review?(jonas)

Comment 44

8 years ago
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.
blocking1.9.1: --- → ?
Considering the patches are both in core code, what component does this bug really belong in?
QA Contact: message-display
Probably Core:DOM.
Component: MailNews: Message Display → DOM
Flags: wanted-seamonkey2.0+
Flags: blocking1.9.2?
Flags: blocking-seamonkey2.0a2-
Flags: blocking-seamonkey2.0a1-
Flags: blocking-seamonkey2.0+
Product: SeaMonkey → Core
QA Contact: message-display → general
status1.9.1: --- → ?
Flags: in-testsuite?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.15?
Can you diff this with -Pu8 please?
(Assignee)

Comment 48

8 years ago
Created attachment 402697 [details] [diff] [review]
-pU8
Attachment #402655 - Attachment is obsolete: true
Attachment #402697 - Flags: review?(jonas)
Attachment #402655 - Flags: review?(jonas)
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.
blocking1.9.1: ? → .4+
status1.9.1: ? → wanted
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15+
Whiteboard: [sg:investigate] → [sg:vector-critical] mail worms, what fun!
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 on attachment 402697 [details] [diff] [review]
-pU8

Smaug has graciously offered to help!
Attachment #402697 - Flags: review?(jonas) → review?(Olli.Pettay)
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)
(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?
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)?

Updated

8 years ago
Attachment #402697 - Flags: review?(Olli.Pettay) → review+
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.)
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?)
(Assignee)

Comment 57

8 years ago
Created attachment 402931 [details] [diff] [review]
With additional assertion
Attachment #402931 - Flags: superreview?(jonas)
Attachment #402931 - Flags: review+
Attachment #402931 - Flags: superreview?(jonas) → superreview+
(Assignee)

Updated

8 years ago
Attachment #402931 - Flags: approval1.9.2?
Attachment #402931 - Flags: approval1.9.1.4?
Attachment #402931 - Flags: approval1.9.0.15?
Attachment #402931 - Flags: approval1.8.1.next?
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 58

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

Let's try this again now that I'm awake.
Attachment #402931 - Flags: approval1.9.2?
Attachment #402931 - Flags: approval1.9.1.4?
Attachment #402931 - Flags: approval1.9.0.15?
Attachment #402931 - Flags: approval1.8.1.next?
(Assignee)

Comment 59

8 years ago
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.
Attachment #402931 - Flags: approval1.9.2?
Attachment #402931 - Flags: approval1.9.1.4?
Attachment #402931 - Flags: approval1.9.0.15?
Attachment #402931 - Flags: approval1.8.1.next?
note that as per the description editor displays the flash when replying too so it isn't just the messagepane
(Assignee)

Comment 61

8 years ago
(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.
>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.
(Assignee)

Comment 63

8 years ago
(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 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
Attachment #402931 - Flags: approval1.9.1.4?
Attachment #402931 - Flags: approval1.9.1.4+
Attachment #402931 - Flags: approval1.9.0.15?
Attachment #402931 - Flags: approval1.9.0.15+
Attachment #402931 - Flags: approval1.8.1.next?
Attachment #402931 - Flags: approval1.8.1.next+
(Assignee)

Comment 65

8 years ago
Pushed changeset bb94e4ce52a4 to releases/mozilla-1.9.1

Fix checked in to CVS HEAD and MOZILLA_1_8_BRANCH.
status1.9.1: wanted → .4-fixed
Keywords: fixed-seamonkey2.0, fixed1.8.1.24, fixed1.9.0.15
(Assignee)

Comment 66

8 years ago
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]
Keywords: fixed-seamonkey2.0, fixed1.8.1.24, fixed1.9.0.15
Version: unspecified → Trunk
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
(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 :) ).
status1.9.1: .4-fixed → ---
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
(Assignee)

Comment 69

8 years ago
Created attachment 403444 [details] [diff] [review]
1.9.1 branch patch
Attachment #403444 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 70

8 years ago
Created attachment 403446 [details] [diff] [review]
Fixed 1.9.1 branch patch

Compiling isn't enough!
Attachment #403444 - Attachment is obsolete: true
Attachment #403446 - Flags: review?(Olli.Pettay)
Attachment #403444 - Flags: review?(Olli.Pettay)

Updated

8 years ago
Attachment #403446 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 71

8 years ago
Created attachment 403447 [details] [diff] [review]
With extra goodness ;-)
Attachment #403446 - Attachment is obsolete: true
Attachment #403447 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 72

8 years ago
Created attachment 403448 [details] [diff] [review]
1.8.1 branch patch

Not a significant merge but requesting review for completeness.
Attachment #403448 - Flags: review?(Olli.Pettay)

Updated

8 years ago
Attachment #403447 - Flags: review?(Olli.Pettay) → review+

Updated

8 years ago
Attachment #403448 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 73

8 years ago
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.
status1.9.1: --- → .4-fixed
status1.9.2: --- → beta1-fixed
Keywords: fixed-seamonkey2.0, fixed1.8.1.24, fixed1.9.0.15
btw, the extension "flashblock" seems to stop this for firefox in <iframe src="flash.swf" and opening flash from locationbar
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next+
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?
(Assignee)

Comment 76

8 years ago
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.
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.
Keywords: verified1.9.1
> Should this file even be getting retrieved at all?

i need to "show remote content" to retrieve the file.
Alias: CVE-2009-3385
Attachment #402931 - Flags: approval1.9.2?
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)
(Assignee)

Comment 80

8 years ago
(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

8 years ago
Timothy Nikkel (:tn) tnikkel@gmail.com thinks this is responsible for the crash in bug 523895 comment 7 / bug 523895 comment 8.

Updated

8 years ago
Blocks: 523895
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.
(Assignee)

Comment 83

7 years ago
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.
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?
(Assignee)

Comment 85

7 years ago
(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

7 years ago
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.
> 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.
(Assignee)

Comment 88

7 years ago
(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.
> 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.
(Assignee)

Comment 90

7 years ago
(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 :-(
Right; my initial patch in this bug added a check on the codepath that leads to that code.
(Assignee)

Comment 92

7 years ago
I'm failing to see the relevance, since that's adding a ShouldProcess call that nobody (e.g. nsWebBrowserContentPolicy::ShouldProcess) cares about.
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.
(Assignee)

Comment 94

7 years ago
(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.
Yes, but for full-page plug-ins it doesn't actually do the load!  So the results of that check are simply ignored.
(Assignee)

Comment 96

7 years ago
I don't know what you mean by ignored; the result of NS_CheckContentLoadPolicy is used to determine whether to call HandleBeingBlockedByContentPolicy.
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.
(Assignee)

Comment 98

7 years ago
In that case I'd better remove the branch fix keyword.
Keywords: fixed1.8.1.24
Group: core-security
You need to log in before you can comment on or make changes to this bug.