346984 - xbl and event handlers cause trouble in mailnews and editor

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

xbl and event handlers cause trouble in mailnews and editor

i am not sure how many bugs are in this report, so help split them if needed.

when an image is included via xbl in an email message, its javascript event
handlers (onload,onmouseover) fire in both preview pane and editor (when
forward inline or reply).

so this allows:

1. javascript exploits in email
2. when forwarding or replying, it is possible to steal user's files via
dhtml - img.src='file:///etc/passwd';document.body.appendChild(img);
3. privacy implications because of the image fetching.

affected are thunderbird 1.5.0.5, 2.0a1 and seamonkey trunk at least.

local mailbox with info and xbl file to be attached.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: local folder

Comment 2

[georgi - hopefully not receiving bugspam]

Attachment: xbl file

Comment 3

[georgi - hopefully not receiving bugspam]

for thunderbird Bug 329755
make javascript.enabled=false in thunderbird
stops the exploits (both for 1.5.0.5 and 20a1)

Comment 4

[georgi - hopefully not receiving bugspam]

brendan, i am not flaming on this: can you use your management skills to speed Bug 329755 (it seems to save the birds from this bug) 

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the basic problem seems to me to be that CompileEventHandler doesn't do any checks at all and CallEventHandler just checks nsJSContext::mScriptsEnabled.  If I understand what that means correctly, event handlers typically run in places where scripts shouldn't be running...  I'm not sure you even need XBL for this testcase, can't you just use an image in the original e-mail (assuming images in mail are enabled)?

Is there a reason we're not checking CanExecuteScripts in CallEventHandler and/or CompileEventHandler?

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Is there a reason we're not checking CanExecuteScripts in CallEventHandler
> and/or CompileEventHandler?

No good reason I know of for CallEventHandler.  We probably shouldn't be testing anything in CompileEventHandler, since compiling is separated in order to share immutable function scripts among windows, so the context on which a handler is compiled will be some early "brutal sharing" context, not a window context in which scripts might be disabled.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  So perhaps we should just add said check to CallEventHandler....

Comment 8

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> If I understand what that means correctly, event handlers typically run in
> places where scripts shouldn't be running...  I'm not sure you even need XBL
> for this testcase, can't you just use an image in the original e-mail (assuming
> images in mail are enabled)?
> 

in mailnews xbl seems needed for this abuse. pure html event handlers (div,input) don't work for me in mailnews - iirc someone killed this bug looong ago.

but if you open an imap uri in seamonkey *browser*, then html event handlers work, though for some reason i can't abuse this for stealing imap email - get sec errors.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> OK.  So perhaps we should just add said check to CallEventHandler....

Agreed -- let's fix this, ASAP.  Dan, I bet this fell on you by default -- can you own it, or find an owner?

/be

Comment 10

[georgi - hopefully not receiving bugspam]

Bug 330443 involves xbl and has privacy implications.

this probably means that at least message content policy is broken.

a potential solution is to *disallow all xbl in mailnews and editor* - i may try to implement this if there is consesus.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er... so I checked again, and we do call nsScriptSecurityManager::CheckFunctionAccess which calls nsScriptSecurityManager::CanExecuteScripts.

Then we have, for the mailcompose window:

1)  Script is enabled on the scriptcontext.
2)  Script is enabled on the docshell.
3)  The toplevel docshell is not APP_TYPE_MAIL (!).

So we allow script execution (item #3 means we do not apply the mail-specific JS-enabled pref).  I'm not sure why other event handlers don't execute, but I'm not sure I care.

In any case, the real bug, as far as I'm concerned, is that this is not an APP_TYPE_MAIL window.  Over to the mail folks.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er... apparently dveditz owns this component?  ccing people who might know about the compose code.

Comment 13

[Daniel Veditz [:dveditz]]

Let's see if we can get this in. Not likely to be as simple as flipping the default pref (bug 329755) because people who *want* js in mail ("malware hosts") won't have the UI to flip it.

Comment 14

[David :Bienvenu]

I believe the app type is EDITOR in this situation - we set it explicitly in several places, e.g.,

http://lxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#501

One potential reason for this is to allow images in compose window, even though they're blocked in mail messages. See https://bugzilla.mozilla.org/show_bug.cgi?id=206793 (though I'm not sure the patch in there is checked in)

We could try setting the app type to MAIL and see what happens. We could also implement disabling js in editor apps like we do for mail apps, or add a separate pref for that and set it ito false n mailnews.js, and leave it on for real editor apps.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I think disabling JS in general for editor apps is a good idea, actually.  You wouldn't want scripts running while the editor is editing stuff!

Comment 16

[Scott MacGregor]

David, I don't think change the app type is going to get you very far. I think that's just going to cause us to go through nsMsgContentPolicy which is not setup to properly cope with compose windows. I've been trying go fix this same problem in Bug 330443 but it is pretty hard. I doubt it's something we'd take in a point release unless a much simpler solution appears than what I've been working on.

Comment 17

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #15)

> I think disabling JS in general for editor apps is a good idea, actually.  You
> wouldn't want scripts running while the editor is editing stuff!

Well.... Who knows ? Why couldn't a binding retrieve data from a database and
dynamically insert it into an editor ?

If I agree 100% that xbl should be disabled in emails messages because this
is a security threat, I clearly see how xbl can be used in a markup editor like
Nvu ; adding bindings to the edited html elements is an extension for the editor
itself, while xpis can only extend the chrome (right, right, you can always
upgrade the core but that's not a game I am willing to play).

To give you a concrete example, the first implementation of image resizing
in the editor I did was using XBL. It was extremely fast and reliable.
It was beautiful, really.

Comment 18

[Boris Zbarsky [:bzbarsky]]

This comes back to chrome vs non-chrome XBL.  We should allow chrome XBL in editor.  We should not allow arbitrary XBL.

Comment 19

[David :Bienvenu]

it's not clear to me that "javascript.enabled" overrides "javascript.allow.mailnews" - this code seems to check if they're different, and if it's a mail window, use the mail pref:

http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#1629

If I add a "javascript.allow.editor" and make it mirror the behavior of "javascript.allow.mailnews" for windows of type APP_TYPE_EDITOR, willthat work for folks? As I said before, TB can set that pref to false, and nVu can do what it wants (not sure about SM - it might want to allow it for composer but not for mail compose...but at least it's under user control, and we can default to the safe choice)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> If I add a "javascript.allow.editor" and make it mirror

This is what I think, yes.

Comment 21

[David :Bienvenu]

I tried this on the mac - as near as I can tell, the docshell we're looking at is for the hidden window, which is of type unknown (this is in the message preview pane, handling a mouse over event, so the problem seems much worse). Is that the docshell/window we'd expect?

I'll try this on windows, and look at the compose window as well.

Comment 22

[Boris Zbarsky [:bzbarsky]]

In the message preview pane the handler's not called; when I last checked the docshell there was the message preview docshell, which has the right app type...

I'm not sure what's going on with Mac, though, given the way menus work there.  Does the JS execute on Mac in the message preview?

Comment 23

[David :Bienvenu]

yes, the js executes on both the mac and windows. I was able to verify that it was the hidden window on the mac; it was a bit harder to match up the doc shell pointer values on windows to see if it was the hidden window, but when I set the hidden window's app type to MAIL, the js stopped executing, both in the preview pane, and the compose window, on windows.

Just so I understand, in this case, it shouldn't be the hidden window's docshell that we get from the nsIScriptGlobalObject referred to by js context, right?

Comment 24

[Boris Zbarsky [:bzbarsky]]

OK, I can reproduce the preview pane issue -- I had to tell seamonkey to load the remote content.

And yes, the script is running on the hidden window, which is mucho wrong.  Here's what happens:

1)  We parse the XBL.
2)  We clone the <img> node when installing XBL anonymous content.
3)  Cloning sets the on* attributes on the new node.
4)  Setting on* attributes calls into
    nsEventListenerManager::AddScriptEventListener
5)  This looks for an nsIScriptGlobalObject to use.
6)  The document's script global is still null, since the owner doc at this
    point is the XBL document.  So we fall back to the "get JS context from the
    stack, or safe JS context" thing.  Luckily, there's no chrome on the stack,
    so we get the hidden window context (safe context).
7)  We store this context in our listener.
8)  Later on, we check whether we can execute the listener and pass the context
    to the security check.  The security manager uses this to get to the
    docshell.
9)  It finds the hidden window, which is not of TYPE_MAIL and has scripts
    enabled.

I tried applying the patch from bug 347523 and commenting out the failing security check in ImportNode, and that fixed the bug.  The event handlers now had the right JSContext, and the mail preview is TYPE_MAIL, while the compose window's docshell has script disabled.

So I think we should do the following, short-term:

A) Introduce an nsIDocument (_MOZILLA_1_8_BRANCH on branches) API to do
   ImportNode but without the security check.  Alternately, push a chrome
   subject principal in XBL before importing, but I'm not sure we can do that,
   and doing it bothers me in general.
B) Use this for XBL.
C) Add some "XXX sXBL/XBL2" comments in AddScriptEventListener.
D) If possible, remove the code in
   nsEventListenerManager::AddScriptEventListener that fishes for a script
   context.  Having random code looking at the JSContext stack and giving
   things principals based on that stack just because it's in a document that
   has no scripting environment bothers me...  We might need to deal on trunk
   with AdoptNode on a node coming from such a document and event handlers
   being broken on it, but that should be fixable.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Looking at CVS blame, the log entry for the checkin that added the "look at the js stack" thing is:

3.121 <[e-mail]> 2000-08-08 14:30
massive landing of joki changes.
Relevant nsbeta3+ bugs 43309, 44503, 2634, 2504,5981, 24698, 25758, 33577,
36062, 36217, 41191, 41491, 42356, 42829, 43016
r=saari (joki code). also been tested by heikki and bryner

I'm guessing the js stack thing was added to fix bug 33577, which is a non-issue now because createContextualFragment creates nodes with the right ownerDocument.  So I stand by removing it, at least on trunk.

Comment 26

[David :Bienvenu]

Thx for figuring this out! Who's the right person to do the short term work for 1.5.0.x?

Out of curiousity, why does the compose window's docshell have script disabled? It's not of type APP_MAIL...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Another option is to change the cx and such on the event handlers of a node when adopting it (whether through adoptNode or the auto-adopt we do right now).  We probably want that for adoptNode, in any case, even if we fix XBL using an importNode hack.

I could do the work once we decide which approach makes us happier, I guess...  Peter, Johnny, Jonas, what do you think?

> Out of curiousity, why does the compose window's docshell have script
> disabled?

Probably because of:

  /editor/composer/src/nsEditingSession.cpp, line 164 -- 
    rv = docShell->SetAllowJavascript(PR_FALSE);

Comment 28

[georgi - hopefully not receiving bugspam]

(In reply to comment #13)
> Let's see if we can get this in. Not likely to be as simple as flipping the
> default pref (bug 329755) because people who *want* js in mail ("malware
> hosts") won't have the UI to flip it.
> 

i suspect that people that want to shoot themselves by enabling js in mail will cry when they get 0wn3d, so you at least can point them they should *not* enable dangerous preferences especially when there is deliberately no UI for shooting.

if they like shooting themselves so much, they can ask the sender for attaching the HTML/XML and then it will run with 'file:///' URI (as of 2006/08/19).

Comment 29

[georgi - hopefully not receiving bugspam]

(In reply to comment #24)
> OK, I can reproduce the preview pane issue -- I had to tell seamonkey to load
> the remote content.
> 

i am not sure this is entirely correct.

by default thunderbird loads remote content if the sender is the personal address book - tested this with new local linux account and installed the bird.
so if the sender is in the address book, xbl fires js.

and email spoofing is not rocket science.

Comment 30

[georgi - hopefully not receiving bugspam]

bz: in the previous comment i meant: if you add the sender of 'local folder' to the personal address book of thunderbird 1.5, does the js fire automatically in preview pane?

Comment 31

[David :Bienvenu]

by default, you're right - if the sender is in your pab, we won't block remote content like images.

Comment 32

[Boris Zbarsky [:bzbarsky]]

G30rgi, in case you missed it I wasn't even talking about thunderbird.  The reason I mentioned that at all is so that I would have recorded steps to reproduce when I tried to do so again.

Comment 33

[georgi - hopefully not receiving bugspam]

(In reply to comment #32)
> G30rgi, in case you missed it I wasn't even talking about thunderbird.  The
> reason I mentioned that at all is so that I would have recorded steps to
> reproduce when I tried to do so again.
> 

bz, i didn't mean to flame with you :)

should i file another bug for this default behaviour in thunderbird or change the Component?

with a trunk, a fox, a bird and a lot of branches, it is not very clear against what to file a bug.

Comment 34

[georgi - hopefully not receiving bugspam]

*** Bug 349342 has been marked as a duplicate of this bug. ***

Comment 35

[georgi - hopefully not receiving bugspam]

the dupe of this for thunderbird is Bug 349342

Comment 36

[Boris Zbarsky [:bzbarsky]]

OK, importNode is not even implemented on the branch... So we might as well do the cx changing, I guess.  :(  It's probably safer, and we want it on trunk too...

Comment 37

[Boris Zbarsky [:bzbarsky]]

So what we need is some new API on nsIJSEventListener and on nsIEventListenerManager to change contexts.... and a way to enumerate all the listeners for a node.  Most are not too bad, but eEventArrayType_Hash sucks... :(

I'll see if I can think of something hacky and simple, I guess, and hope that on trunk life becomes better.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Filed bug 349465 on changing the cx in existing event listeners and bug 349467 on the nsEventListenerManager changes.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Super-hacky patch

This does fix the bug....  I hope it doesn't break anything.  :(

Comment 40

[Boris Zbarsky [:bzbarsky]]

Note that I'll need to merge to branch, due to DOM_AGNOSTIC.  But I used nsIContent instead of nsINode to make that a little easier...

Comment 41

[georgi - hopefully not receiving bugspam]

i am not familiar with the code, just judging from comments, so probably this is nonsense:

as far i can understand, part of the problem is 'hidden window of type unknown'. unless such windows are very common, can the following work:

1. if the caller is chrome, allow js access
...
2. if the window is 'hidden window of type unknown' disallow js access.

in this scenario even if something breaks, it may be a sign of a problem.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> 2. if the window is 'hidden window of type unknown' disallow js access.

Sadly, given the current state of XBL, that would break the entire browser UI... And yes, it's a sign of a problem; see bug 349465.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 234737 [details] [diff] [review]
Super-hacky patch

Seems icky, but reasonable. The control flow in this function is a bit weird, but I guess I can just blame C++ not having a finally construct for that.

Comment 44

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

Does my patch in bug 343037 by any chance help here at all? It should make it impossible to do things like put onclick attributes on elements inside xbl bindings in mail

Comment 45

[Boris Zbarsky [:bzbarsky]]

Hmm... it might help, maybe... you want to test?  If not, I'll try to apply it and build it tonight.

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 234737 [details] [diff] [review]
Super-hacky patch

sr=jst whether we need this or not.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> Does my patch in bug 343037 by any chance help here at all?

Yes, it does.  Should we do that instead of this hack, then?  I think I would prefer it.

Comment 48

[Daniel Veditz [:dveditz]]

Bug 343037 appears inappropriate for 1.8.0.7 given that it likely breaks themes. Would the "super-hacky patch" be OK enough for 1.8.0.x?

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 234737 [details] [diff] [review]
Super-hacky patch

Yeah, let's do this for the stable branch.

Comment 50

[Boris Zbarsky [:bzbarsky]]

I checked this in on trunk to get some bake-time.  We should back it out once bug 343037 lands on trunk.

Comment 51

[Daniel Veditz [:dveditz]]

Comment on attachment 234737 [details] [diff] [review]
Super-hacky patch

a=dveditz for 1.8.0 branch

Comment 52

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch version of patch if we need it

Comment 53

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8.0 branch version of patch

Comment 54

[Boris Zbarsky [:bzbarsky]]

Checked in on 1.8.0 branch for 1.8.0.7.

Comment 55

[Jay Patel [:jay]]

G30rgi:  Can you possibly verify this fix with the latest Thunderbird 1.5.0.7 candidate builds?  Thanks!

Comment 56

[georgi - hopefully not receiving bugspam]

(In reply to comment #55)
> G30rgi:  Can you possibly verify this fix with the latest Thunderbird 1.5.0.7
> candidate builds?  Thanks!
> 

according to tests it is fixed in thunderbird version 1.5.0.7 (20060907)

Comment 57

[Daniel Veditz [:dveditz]]

I doubt bug 343037 will be landing in time for Tbird2.0 final, can we take this approach on the 1.8 branch also?

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 235353 [details] [diff] [review]
1.8 branch version of patch if we need it

Yeah, I guess we should.

Comment 59

[Mike Schroepfer]

Comment on attachment 235353 [details] [diff] [review]
1.8 branch version of patch if we need it

a=schrep for drivers.

Comment 60

[Daniel Veditz [:dveditz]]

From the summary: "...trouble in mailnews and editor"

In Thunderbird 1.5.0.7 this patch fixes the "editor" part--reply or forward inline--but not the "mailnews" part. If you load the XBL the scripts still run when you're viewing the message (in both three-pane and standalone message windows).

Comment 61

[Boris Zbarsky [:bzbarsky]]

Argh.  I'd tested in Seamonkey mailnews, and this patch does fix the bug there.  I thought the two were similar enough...

Pulling and building Thunderbird now.

Comment 62

[Boris Zbarsky [:bzbarsky]]

So... I just pulled thunderbird from the 1.8.0 branch and built it... and I don't see the bug in that build.

I also can't reproduce the problem in the build at ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2006-09-13-03-mozilla1.8.0/thunderbird-1.5.0.7.en-US.linux-i686.tar.gz

dveditz, what exact build were you testing?

Comment 63

[Daniel Veditz [:dveditz]]

I tested both the 1.5.0.5 windows release build as well as the Thunderbird 1.5.0.7 rc6 candidate build.  The reply/forward problem was fixed in .0.7 and the message window problem was seen in both.

I don't think Georgi wrote this bug based on windows testing (references /etc/passwd) and he was seeing it too.

Comment 64

[Daniel Veditz [:dveditz]]

So I am an idiot. My "clean" profile was not quite as virgin as I thought, at some point I allowed javascript in mail -- so of course I got javascript in mail.

This *is* fixed.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Checked in on 1.8 branch.  I missed that approval somehow...

Comment 66

[Daniel Veditz [:dveditz]]

Not sure how to rate this one. JavaScript in mail is an option people can actually choose to inflict on themselves, so how bad can it be? On the other hand  it makes people vulnerable to all the browser flaws they thought they were protected from by not having script in mail.

The most direct attack here is in the ability to "mail-tap" a reply or forward. Assuming a highly sensitive comment added to a forward the attack is a "high impact" information stealing one. But if there were a critical browser bug then this turns it into a critical mail bug as well.

stepping-stone bugs are always hard to rate :-(

Comment 67

[georgi - hopefully not receiving bugspam]

(In reply to comment #66)

> 
> The most direct attack here is in the ability to "mail-tap" a reply or forward.
> Assuming a highly sensitive comment added to a forward the attack is a "high
> impact" information stealing one. But if there were a critical browser bug then
> this turns it into a critical mail bug as well.

don't consider this critical.

but the above quote is not quite correct - hitting reply or forward inline allows stealing local files (/etc/passwd in the testcase) - reply to the testcase and examine the source of the message - content /etc/passwd is there.

Comment 68

[georgi - hopefully not receiving bugspam]

(In reply to comment #66)
> But if there were a critical browser bug then
> this turns it into a critical mail bug as well.

a little nit - critical *javascript based* browser bug - this doesn't affect non javascript bugs, though may make reliability of exploit higher with the help of javascript.

Comment 69

[georgi - hopefully not receiving bugspam]

couldn't make this work via mailto: urls - is this by design or am i missing something.

looks like mailto: does not invoke styles in editor at all.

Comment 70

[Boris Zbarsky [:bzbarsky]]

At what point can we open up this bug?