freeze after imap msg w/attached web page (JS security)

VERIFIED FIXED in M18

Status

MailNews Core
Networking
P1
normal
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: Bienvenu, Assigned: Scott MacGregor)

Tracking

({crash})

Trunk
x86
Windows NT
crash

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-][rtm need info])

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
I don't know when this bug started, but it's in yesterday's release build, and
on the tip today - if you read the msg Chris Hoffman sent out with the ZDNet
article, then you can't read any other messages, imap or local. If you read a
local msg, you crash in nsMailboxProtocol::ReadMessageResponse because
m_channelListener is null. If you try to read an imap message, it loads but does
not display, again because in nsImapProtocol::BeginMessageDownLoad,
m_channelListener is null. I don't know what's causing this but it's easily
reproducible.
(Reporter)

Comment 1

17 years ago
I'll look into this a bit and see if I can figure out why the channel listener
is not set. I *don't* think it was your fix for the news article forwarding
problem, because yesterday's release build had the problem.
Keywords: crash, nsbeta3, regression
(Reporter)

Comment 2

17 years ago
OK, the problem is that nsMessenger's mDocShell is null after reading Chris's
message, and everything goes downhill after that. Not sure how mDocShell got to
be null, unless SetWindow got called. I'll look a little more.
(Reporter)

Comment 3

17 years ago
So, SetWindow gets called everytime we load a msg, and after loading the msg in
question, we can't get the msg pane from the passed in dom window. I wonder if
this could be related to the js changes for clearing out js state on web page
loades. I'll see if I can find that change and back it out.

Comment 4

17 years ago
i think this is a dup of bug 38398 which was reopened yesterday
(Reporter)

Comment 5

17 years ago
Nope, Brendan's change to dom\src\base\nsGlobalWindow.cpp wasn't it. No more clues.

Comment 6

17 years ago
anyways - several attachment bugs the past 24h, some likely related:

bug 48224
"Mail with Attachment icon is not showing in the thread view"
bug 48374
"Attachment not shown"
(Assignee)

Comment 7

17 years ago
So this is kind of cool.....here's why this page breaks us....
the body is loaded into an iframe that has a name attribute of "messagepane".
before we load a message, we find the right iframe to load it into by searching
in the DOM for an element with this name.

This page attached to this message contains some JS which does the following:
self.name = "parentWin"

effectively renaming our iframe from "messagePane" to "parentWin".

*doh*

So we never find the message pane again in future searches.....

clearing regression keyword. This isn't a regression.
Status: NEW → ASSIGNED
Keywords: regression
Target Milestone: --- → M18
Blocks: 40756

Comment 8

17 years ago
+, P1 per mail triage
Keywords: mail2
Priority: P3 → P1
Whiteboard: [nsbeta3+]
(Assignee)

Comment 9

17 years ago
cc'ing Mitch. Mitch, this is the bug I emailed you about last week. The web page
we are laying out in the message pane has JS which says self.name = "parentWin"
causing our "messagePane" docshell to get renamed.

Supposedly JS is disabled in the message pane. So this shouldn't have had the
right capaiblities to execute, right?
JS is not disabled in the message pane...messages can contain scripts, and the 
scripts will run. I wanted to disable JS in mail by default, and was told I can't 
do this, but even if it's disabled by default, when turned on it should obviously 
not be allowed to change the name of the frame it's in. There are probably other 
similar things which JS in mail should not be allowed to do; the sandbox needs to 
be tighter or we'll wind up with messages which can mail themselves to people in 
your addressbook and other fun behaviors. I'm not sure how best to deal with this 
but I'll give it a look.

Comment 11

17 years ago
cc: Cathy Zhang (QA) since this may have to do with mail security.

And js is not disabled by default in the message pane in commercial builds as 
per another bug report that Mitch is referring to.

Comment 12

17 years ago
to pdt: this is a P1 because someone can affect another user's system and make 
it so that s/he can't read any other message in that session.  Per Marketing and 
mail team's earlier decision, Netscape builds will have JS turned on by default 
in the message pane due to other reasons so we can't fix this by turning JS off.

Comment 13

17 years ago
PDT agrees P1
(Assignee)

Comment 14

17 years ago
Hey Mitch, it turns out my fix for 51442 fixes this bug as well because now the
running JS doesn't have the right capabilities for setting the name of it's
containing iframe.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 15

17 years ago
verifying it 

Comment 16

17 years ago
Waiting for the (attachment) webpage that caused this bug!

Comment 17

17 years ago
Re-opening bug.
Steps to reproduce problem
1- goto www.zdnet.ch (if you viewsource you will notice a self.name="parentWin");
2- send the page to yourself.
3- Open the page and then try clicking on any other message
4- since parentWin is the name of the pane now messagepane is lost and you cant
view messages.
5- you can still view messages in a separate window by clicking on the thread or
pressing Enter.
Status: RESOLVED → REOPENED
Keywords: rtm
Resolution: FIXED → ---

Comment 18

17 years ago
Does not work on release commercial builds 2000092808/2000092810
on all three platforms (mac,linux,win).
Although works on 
release mozilla build (2000092810) on win, linux and mac. 
 

Comment 19

17 years ago
rtm+, this renders mail unusable.  Why is only the mozilla build fixed?  Did we
forget to check this in on the branch or something?
Whiteboard: [nsbeta3+] → [nsbeta3+][rtm+]

Comment 20

17 years ago
cleaning up: turning nsbeta3+ to nsbeta-: beta3 has passed.
Whiteboard: [nsbeta3+][rtm+] → [nsbeta3-][rtm+]
(Reporter)

Comment 21

17 years ago
we don't know why it occurs on netscape but not mozilla builds - a couple
possibilities are prefs differences between mozilla and netscape, or diffs
between debug and release builds. But the branch is not an issue since we
branched long after this bug was marked fixed.

Comment 22

17 years ago
JS in Mailnews is disabled by default in Mozilla, enabled in Netscape 6.

Comment 23

17 years ago
changing to [rtm need info]. We want to rtm+ this but need a patch and code
reviews. When those exist, please change back to [rtm+].
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]

Comment 24

17 years ago
If 51442 is fixed, how come that page can still rename its iframe??

Comment 25

17 years ago
What is bug 51442? I don't have permissions to view it :-(((.

Comment 26

17 years ago
*** Bug 55871 has been marked as a duplicate of this bug. ***

Comment 27

17 years ago
Have we lost the battle on this one?  Should I mark it rtm-?  I haven't seen a
progress update in the last few days and time is almost up.

Comment 28

17 years ago
I hope we haven't lost the battle. I repeatedly fall into the trap of loading
messages that won't go away, and the workaround - shut down and restart mail -
is very painful.
Give us one more day. Scott, any progress? I'll take a look today.
Looks like the message pane is still being allowed to change its own name. I've
reproduced this in Mozilla with mail JS activated. As Scott suggested, it wasn't
showing up in Mozilla because mail JS was off by default. Looks like we'll just
have to add a check somewhere that sats self.name is read-only if you're a
message pane. While we're at it, is there anything else the message pane
shouldn't be able to do?
I will try to have a fix for this tomorrow (Fri), can we get it into rtm?
(Assignee)

Comment 31

17 years ago
If you get me a patch, I'll try to get it through PDT =). 

Updated

17 years ago
QA Contact: lchiang → pmock

Comment 32

17 years ago
mstoltz, any luck with a patch?
(Assignee)

Comment 33

17 years ago
If you can get me a patch today I'll take it to PDT while they are meeting. Thanks! 
Not sure I'm going to get to this.
I was thinking about just preventing scripts in mail from setting self.name, but
is there a larger issue here? Are there any other, similar things whoich will
break mail?
(Assignee)

Comment 36

17 years ago
On the surface, self.name is the only one I can think of. Actually, should we
prevent it from bringing up dialogs or is that already blocked.

Comment 37

17 years ago
Changing summary to include the JS security concern.

Please attach the patch if you can.  The approval step is predicated on it existing.
Summary: after reading imap msg with attached web page, can't read any other messages → freeze after imap msg w/attached web page (JS security)
(Assignee)

Comment 38

17 years ago
I'd still like to get a fix for this Mitch, if you think it'll be easy. If you
don't have time to get to it, if you can point me in the right direction then
maybe I can figure it out. 
I'm working on a fix tonight. I think I can modify the security manager to
prevent mail messages from writing to window.name. 
I have a fix, testing to make sure it's safe...
Created attachment 17537 [details] [diff] [review]
Patch - allow per-scheme sec policies and add policy for mail
(Assignee)

Comment 42

17 years ago
Mitch does mOrigin come from a URI? If so, we can just call ->GetScheme() to get
the scheme instead of trying to parse the scheme out of the string ourselves.
mOrigin is a string originally read out of prefs; it can be either an URL like
http://warp or just a scheme, like http:. Basically all the operations are on
strings; it would add more complexity to build URLs and then parse out the
scheme, I think.
note to mscott:

following varada's steps to reproduce, I was able to reproduce this bug.

then I applied mstoltz's patch and rebuilt, and the problem is fixed.
(Assignee)

Comment 45

17 years ago
Mitch, can you get a reviewer from someone in your group to review your patch? I
can be the super reviewer for this.

thanks for trying this out Seth.

sr=mscott

It'd be great if we could get the review done today so we can take it to PDT.
Thanks again for fixing this for us!

Comment 46

17 years ago
sorry for the extra email. Removing mail2 keyword.
Keywords: mail2

Comment 47

17 years ago
Mitch, would it be possible to get another security review so we can try to get
this in this weekend before Monday's build?
(Assignee)

Comment 48

17 years ago
Mitch, I'd really like to get this checked in today but we need another reviewer
from soneone on your team that's familiar with this code. Who can we pester?
beard has reviewed this. Marking rtm+ to expedite pdt review.
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm+]

Comment 50

17 years ago
rtm++
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm++]
(Assignee)

Comment 51

17 years ago
checked into the branch and the tip. thanks again mitch.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Whiteboard: [nsbeta3-][rtm++] → [nsbeta3-][rtm need info]

Updated

17 years ago
Keywords: vtrunk

Comment 52

17 years ago
Verified as fixed on branch build of win32, linux, and macos using the following
builds:
 win32 commercial seamonkey build 2000-102409-mn6 installed on P500 Win98
 linux commercial seamonkey build 2000-102409-mn6 installed on P200 RedHat 6.2
 macos commercial seamonkey build 2000-102308-mn6 installed on G3/400 OS 9.04

Comment 53

17 years ago
Win32 (2001-07-10-05-0.9.2)
This bug is gone.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.