453943 - Review JavaScript in mail/rss policies

Status: RESOLVED DUPLICATE of bug 453928

(Thunderbird :: Security, defect)

Description

[David Ascher (:davida)]

At this point, it strikes me that allowing JS in mail is a bad idea that we should actively stop, rather than leave as an attractive nuisance (http://en.wikipedia.org/wiki/Attractive_nuisance_doctrine).

It's exposing users to risk whenever there is a Firefox release that ships before the corresponding Thunderbird release, and no other modern mail client supports it, so the validity of the use cases feel marginal at best.

I think we should move beyond making it hard to enable, and make JS in mail not available at all without an extension.  

Notes:

 a) the code enabling JS by looking at the javascript.allow.mailnews should be removed

 b) on first-run after upgrades, we should detect if the javascript.allow.mailnews pref was set, and 
   - remove it (only to avoid confusion -- it wouldn't work anyway)
   - gently tell users why we did it

Possibly:

 c) we could setup a new pref specifically for RSS content, also default off.

 d) we could setup a new pref specifically for news content, also default off.

My inclination is to do c) but not d), but I'd like input from security folks on either of those is reasonable.

I'll mark as blocking-tb3 and targeting for b2 because I think we need a final decision before then.

(See bug 453928 for one new issue with allowing JS)

Comment 1

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

This winds great if we can pull it off. I'd even say that we should not have a pref for feeds as that sounds pretty much as bad as in mail.

Making it possible to let extensions enable based on sender might address corporate cases

Comment 2

[Phil Ringnalda (:philor)]

"The feed case" is actually two cases:

the "display the feed <description>/<content>" one, where we just create a text/html mail with the content stuffed in it, which is slightly worse than mail since loading remote images is enabled, so in current nightlies we'll directly expose your profile path to anyone who can get into a feed you subscribe to, and 

the unchecked "[ ] Show the article summary instead of loading the web page" one, where we just use the item <link> as the src for an iframe, where we should only be vulnerable to something that makes http same-origin with mailbox. I feel moderately good about that, but short of giving the script security manager a whole lot more knowledge about Tb's internals, that would mean twiddling the APP_TYPE_ on the root docshell as we load and unload one of those messages (hmm, messages which can be copied/moved to any other mailbox), at which point I start getting nervous again.

Comment 3

[Mark Banner (:standard8)]

Adding Neil to the cc list - I know Scott was working on something in bug 374577 related to this pref and I'm not sure if it would help here.

Comment 4

[David Ascher (:davida)]

We might want to deal with this earlier.

Comment 5

[Joe Sabash [:JoeS1]]

(In reply to comment #0)
> At this point, it strikes me that allowing JS in mail is a bad idea that we
> should actively stop, rather than leave as an attractive nuisance
> (http://en.wikipedia.org/wiki/Attractive_nuisance_doctrine).

Many users consider the ability to use script in (primarily newsgroups) an asset. So when you say "it strikes me" that would really apply directly to the folks who actively use JS
I think you should seek the input of that group of users..hence my comments here.

> It's exposing users to risk whenever there is a Firefox release that ships
> before the corresponding Thunderbird release, and no other modern mail client
> supports it, so the validity of the use cases feel marginal at best.
> 
> I think we should move beyond making it hard to enable, and make JS in mail not
> available at all without an extension.  
> 
> Notes:
> 
>  a) the code enabling JS by looking at the javascript.allow.mailnews should be
> removed

A similar thing was done when the plugin XPI's were removed from branch builds.
This resulted in many users going to SeaMonkey, or even OE, that wanted that capability.(that happened because prefs were ignore in linux)
 
>  b) on first-run after upgrades, we should detect if the
> javascript.allow.mailnews pref was set, and 
>    - remove it (only to avoid confusion -- it wouldn't work anyway)
>    - gently tell users why we did it

For folks who want JS capability, there would be no "gentle" way to do this.
I would consider this arbitrary and inconsiderate of my usage of TB.

> Possibly:
> 
>  c) we could setup a new pref specifically for RSS content, also default off.
> 
>  d) we could setup a new pref specifically for news content, also default off.
> 
> My inclination is to do c) but not d), but I'd like input from security folks
> on either of those is reasonable.

All of the above should be optional, and accessible by pref (and not hidden)

> I'll mark as blocking-tb3 and targeting for b2 because I think we need a final
> decision before then.
> 
> (See bug 453928 for one new issue with allowing JS)

There are more users than you know, that would like to use (or enjoy the results of) JS in mailnews. The problem is selectively enabling JS

CAPS can be used to selectively allow JS for a specific news domain, but it is difficult to set up and beyond the scope of the average user. I have posted samples of how to do this.

Basically, there is no way to completely bullet-proof any software.
Maybe there is a danger in smileys, should we eliminate them too.

The answer, IMHO is educating the user, and allowing *them* to choose their level of comfort by prefs that warn, then actually do what they say.
There should be a separate pref for each protocol:
Mail, News, rss feeds, and local folders (folders created by the user)
BTW You can configure OE to run JS, Jscript, or vbscript quite easily.

Please don't "through out the baby with the bathwater."

Comment 6

[David Ascher (:davida)]

Joe, maybe you and I can have a side conversation so you can explain the use cases of JS in content.  I expect it'll be a longish conversation, and we can summarize it on this bug after the fact.  In general, I'm the last guy to want to take away dynamic features, and I'm far from a security wonk.  I have, however, seen the brake on development that half-supported features bring, and sometimes it's better for the overall health of the project to remove a half-completed feature, so it can be redone better in a different, more appropriate context.  I half-suspect that the use cases that are important to you can be better handled (more completely, more securely) using a different approach than the current one.  But I'd like to understand your POV better, hence the suggestion of a "sidebar".

Comment 7

[David :Bienvenu]

We could just make this line always return false, for b1:

http://mxr.mozilla.org/comm-central/source/mozilla/caps/src/nsScriptSecurityManager.cpp#1711

and then explore the patch in bug 374577. Does that sound like a reasonable plan?

Comment 8

[Joe Sabash [:JoeS1]]

I wasn't aware of any "firedrill" regarding JS in mailnews.
So investigating the options in bug 374577 first, should be the first step. IMO

I'm in favor of a popup notification for *all* security violations, not just script. OE does something like this in regard to iframes, scripts and remote sourced images.

Basically TB should tell me, in some way, when it can't/won't do what the content is requesting.

"Permission denied" in the error console is not sufficient notification that TB has decided for me, the security level that I should use.

I think that was what Scott was heading for in bug 374577
Allowing the App to determine security measures, and not the core platform.

Comment 9

[Joe Sabash [:JoeS1]]

xref bug 429817 on where security issues should be determined.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> xref bug 429817 on where security issues should be determined.

Bug 429817 is completely unrelated to this.

Bug 453928 is mainly why we're deciding to turn this off for beta 1 (and we probably made a mistake doing these comments here rather than there).

At the moment (due to the changes referenced in bug 453928), anyone who enables javascript in mailnews (in 3.x builds) is now at much greater risk for attack, as beta 1 code freeze is on Tuesday, we're simply not going to have time to do a proper security review and fix. That is why we have decided, at least temporarily, to force javascript to be turned off in mailnews.

We will relnote the change, and at the same time, I expect it we will get some feedback on how much, and where users actually use this.

Post b1 we can then focus on doing a correct fix - whatever that is determined to be.

Comment 11

[David :Bienvenu]

Attachment: temporarily disable js for mailnews

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #7)
> We could just make this line always return false, for b1 [...] and then
> explore the patch in bug 374577. Does that sound like a reasonable plan?

Always returning false will make it impossible to control mailnews JS at the
docshell level as proposed in bug 374577. Don't get me wrong, I would love to
get app-specific checks out of CAPS (though wouldn't mind a generic, extendable
one), but your proposal seems at cross-purposes with what looks to be the
eventual plan. It would make more sense to remove the mail check entirely and
rely solely on the global JS pref until tbird hooks prefs up with the docshell
controls.

(In reply to comment #1)
> I'd even say that we should not have a pref for feeds as that sounds pretty
> much as bad as in mail.

I totally agree for feeds viewed as feeds, but in Thunderbird lots of people
don't view the feed content. For whatever reason (feed only has summaries, they
want to read the comments too) they actually load the web content for the link
instead of the content from the feed. Then you run into blogs that display
terribly if you don't have JS or whatever reason you might feel the need for JS
in a web page as opposed to a true feed and people start demanding JS be turned
on.

(In reply to comment #5)
> Many users consider the ability to use script in (primarily newsgroups) an
> asset.

What users, what groups? Traditional Usenet banned HTML posts of any kind, so
no script there. Lots of people read through Google Groups, no scripts there...

There's one or two multimedia test groups that I know about (primarily
inherited from ancient Netscape groups), but that's not "many" users. I don't
feel the need to cater to people experimenting with unintended side effects of
one particular feed reader. It was fun while it lasted but it is in no way a
mainstream use-case.

> This resulted in many users going to SeaMonkey, or even OE, that wanted that
> capability.(that happened because prefs were ignore in linux)

If we remove the pref check from caps that won't leave Seamonkey many options.
Unlike Thunderbird they can't use the global pref because they have a browser
part that needs to keep working. They can probably do something like bug
374577, but I'd feed more confident they knew they needed to if there were a
seamonkey version of that bug.

> There are more users than you know, that would like to use (or enjoy the
> results of) JS in mailnews. The problem is selectively enabling JS

So you say. Is there a body of js-enhanced email or news postings you could
point us at?

> Maybe there is a danger in smileys, should we eliminate them too.

You joke, but in fact smileys were a major vector for downloaded trojans. If we
hadn't added smileys maybe people wouldn't have wanted advanced graphical
animated smileys enough to download packages from strangers.

> BTW You can configure OE to run JS, Jscript, or vbscript quite easily.

You _can_, but do enough people actually change the default to make it a
setting we need to interoperate with?

This has turned into a discussion that should move to a newsgroup
(mozilla.dev.apps.thunderbird? I don't see a discussion group for core mailnews
but maybe I missed it.)

Comment 13

[David :Bienvenu]

(In reply to comment #12)
> (In reply to comment #7)
> > We could just make this line always return false, for b1 [...] and then
> > explore the patch in bug 374577. Does that sound like a reasonable plan?
> 
> Always returning false will make it impossible to control mailnews JS at the
> docshell level as proposed in bug 374577. Don't get me wrong, I would love to
> get app-specific checks out of CAPS (though wouldn't mind a generic, extendable
> one), but your proposal seems at cross-purposes with what looks to be the
> eventual plan. It would make more sense to remove the mail check entirely and
> rely solely on the global JS pref until tbird hooks prefs up with the docshell
> controls.
> 
Since this is temporary, we were looking for the safest, easiest fix. If we rely on the global js pref, then we'd be in the same boat we're in today, for the beta - users could turn js on in mail, unless I'm missing something. Our intent is to turn that ability off for beta 1.

Comment 14

[Robert Kaiser]

Could we have a type flag/string on the <browser> that gets assigned to the docshell somehow and and checks a javascript.enabled.[typestring] pref to determine if it (and any subshells) should run JS. That way, the app (in this case Thunderbird) could assign "mailnews" or "feed" or something like those dynamically as types on the message view and have different settings of allowing JS for those.
We could use the same principle for plugins and perhaps could get bug 371976 fixed on the same basis.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 339454 [details] [diff] [review]
temporarily disable js for mailnews

sr=dmose.  We should probably relnote this, since it's going to break one broadly useful case that dveditz noted of people looking at content pointed to be feeds.

Comment 16

[Joe Sabash [:JoeS1]]

> 
> (In reply to comment #5)
> > Many users consider the ability to use script in (primarily newsgroups) an
> > asset.
> 
> What users, what groups? Traditional Usenet banned HTML posts of any kind, so
> no script there. Lots of people read through Google Groups, no scripts there...
> 
> There's one or two multimedia test groups that I know about (primarily
> inherited from ancient Netscape groups), but that's not "many" users. I don't
> feel the need to cater to people experimenting with unintended side effects of
> one particular feed reader. It was fun while it lasted but it is in no way a
> mainstream use-case.

Most of the posts have "aged off" now either through retention rates, or actual user aging.
Here are some OE users having "fun"
news://news.annexcafe.com:119/script.writing.beginners
There are a few left here (before the usage of the group was curtailed.)
news://news.mozilla.org:119/mozilla.test.multimedia

Mostly folks used JS to enhance posts for the same reason it is used on the Web.

The difficulty in composing(editor bugs) JS in news, was just too daunting.
For instance, how would like to compose a script without the use of "andif" or "greater than" operators. They are double escaped after composing.
I re-wrote quite a few scripts for templates eliminating those operators.

> > This resulted in many users going to SeaMonkey, or even OE, that wanted that
> > capability.(that happened because prefs were ignore in linux)
> 
> 
> > There are more users than you know, that would like to use (or enjoy the
> > results of) JS in mailnews. The problem is selectively enabling JS
> 
> So you say. Is there a body of js-enhanced email or news postings you could
> point us at?

Not any more, but I know quite a few users that would both enjoy, and learn from the experience.

> > Maybe there is a danger in smileys, should we eliminate them too.
> 
> You joke, but in fact smileys were a major vector for downloaded trojans. If we
> hadn't added smileys maybe people wouldn't have wanted advanced graphical
> animated smileys enough to download packages from strangers.

That's a matter of user education. But your right that those folks liked what they saw, and wanted more.

We need a newsgroup for folks interested in graphics, and other media enhancements in mailnews to educate them. (that's another bug) 

> This has turned into a discussion that should move to a newsgroup
> (mozilla.dev.apps.thunderbird? I don't see a discussion group for core mailnews
> but maybe I missed it.)

Agreed.

Comment 17

[David :Bienvenu]

checked into mozilla-central - changeset:   19455:38988e401f12

Comment 18

[David :Bienvenu]

I'm going to move this to b2, for the remaining issue of seeing if we can figure out a less restrictive solution to the caps problems.

Comment 19

[Joe Sabash [:JoeS1]]

The checkin does *not* seem to "work" for me.
I don't have any CAPS site policies installed at this time.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080920112042 Shredder/3.0b1pre ID:20080920112042
The above hourly should have included the checkin.

Comment 20

[Joe Sabash [:JoeS1]]

Attachment: js test eml

Dutifully, if not painfully posted by a nightly tester.

Comment 21

[Mark Banner (:standard8)]

Moving back to b1 blocker - I can confirm Joe's findings. If javascript.allow.mailnews is set to true, then javascript is still allowed to run.

(Tested with start page as http://acid3.acidtests.org/ which shows JS is disabled if javascript.allow.mailnews is set to false and also with Joe's test message.

Comment 22

[Joe Sabash [:JoeS1]]

I'd like to make a suggestion here:
Why use a code change, when JS can be blocked in all-thunderbird.js
(which is not accessible in about:config)
CAPS policy prefs seem to trump all.
Adding the following to all-thunderbird seems to do the job.
pref("capability.policy.default.javascript.enabled", "noAccess");

Comment 23

[Joe Sabash [:JoeS1]]

One thing that I did not anticipate using the above method:
The marquee tag fails to function.
I've known all along that without JS enabled, the "behavior" attribute was ignored. But now even default marquees do not function.

This might have happened with any method of disabling JS

Marquee is written in XBL, the question is:
Do we have any other XBL that will be affected.

Comment 24

[David :Bienvenu]

Attachment: proposed fix, v2 - checked in

I think this should work - the problem before was that the caps code was checking if the mailnews js pref was the same as the global pref before getting to the code that I changed before, so if js was globally set to true...

Comment 25

[Joe Sabash [:JoeS1]]

Doh ! A CAPS solution to a problem where CAPS might be ignored.
Next time I'll engage the brain, before opening mouth.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 339664 [details] [diff] [review]
proposed fix, v2 - checked in

sr=dmose

Comment 27

[David :Bienvenu]

Comment on attachment 339454 [details] [diff] [review]
temporarily disable js for mailnews

this was backed out in subsequent checkin/patch.

Comment 28

[David :Bienvenu]

moving to b2

Comment 29

[bugzilla]

I'm new to bugzilla, and say a few things.

A:"I think we should move beyond making it hard to enable, and make JS in mail not available at all without an extension."

Hard to enable for the user, or hard to enable for the developer?

B:Please don't remove the pref as this makes it harder for backwards compatibility with previous Thunderbird installs for extensions that use the pref to manage JS quickly and easily.

Comment 30

[Joe Sabash [:JoeS1]]

(In reply to comment #29)

> B:Please don't remove the pref as this makes it harder for backwards
> compatibility with previous Thunderbird installs for extensions that use the
> pref to manage JS quickly and easily.

Care to reveal just what those extensions would be.
If they rely on CAPS , that means of enabling might not be there much longer.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #29)
> I'm new to bugzilla, and say a few things.
> 
> A:"I think we should move beyond making it hard to enable, and make JS in mail
> not available at all without an extension."
> 
> Hard to enable for the user, or hard to enable for the developer?

For the user.  One of the motivations for the extension model is to allow for different policy decisions than the default ones in core.

> B:Please don't remove the pref as this makes it harder for backwards
> compatibility with previous Thunderbird installs for extensions that use the
> pref to manage JS quickly and easily.

We're definitely motivated to try and make things continue to be workable for extension developers.

Watch this space and bug 374577 to see what proposals come down the pike, and let us know whether they are likely to be practical for ThunderBrowse.

Comment 32

[David :Bienvenu]

taking off b1 blocker list, since the work will be done in bug 453928 - putting in b2, though it should ultimately just get marked fixed as the result of bug 453928

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

This is really the same thing as bug 453928.  Having two bugs open for the same thing adds confusion, and doesn't benefit us in any interesting way.  Marking it as a DUP and removing the blocking status.