Closed Bug 346984 Opened 14 years ago Closed 14 years ago
xbl and event handlers cause trouble in mailnews and editor
1.05 KB, text/plain
1.15 KB, text/xml
2.89 KB, patch
|Details | Diff | Splinter Review|
3.39 KB, patch
|Details | Diff | Splinter Review|
3.40 KB, patch
|Details | Diff | Splinter Review|
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)
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?
(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
OK. So perhaps we should just add said check to CallEventHandler....
(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.
(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
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.
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.
Er... apparently dveditz owns this component? ccing people who might know about the compose code.
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.
Depends on: 329755
Flags: blocking188.8.131.52? → blocking184.108.40.206+
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.
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!
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.
(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.
This comes back to chrome vs non-chrome XBL. We should allow chrome XBL in editor. We should not allow arbitrary XBL.
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.
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?
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?
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.
Assignee: dveditz → general
Component: Security → XBL
Product: Thunderbird → Core
QA Contact: thunderbird → ian
Version: 1.5 → Trunk
Looking at CVS blame, the log entry for the checkin that added the "look at the js stack" thing is: 3.121 <firstname.lastname@example.org> 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.
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...
(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).
(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.
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?
by default, you're right - if the sender is in your pab, we won't block remote content like images.
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.
(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.
*** Bug 349342 has been marked as a duplicate of this bug. ***
the dupe of this for thunderbird is Bug 349342
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...
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.
Filed bug 349465 on changing the cx in existing event listeners and bug 349467 on the nsEventListenerManager changes.
This does fix the bug.... I hope it doesn't break anything. :(
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...
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
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.
> 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 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.
Attachment #234737 - Flags: review?(mrbkap) → review+
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
Hmm... it might help, maybe... you want to test? If not, I'll try to apply it and build it tonight.
Comment on attachment 234737 [details] [diff] [review] Super-hacky patch sr=jst whether we need this or not.
Attachment #234737 - Flags: superreview?(jst) → superreview+
Whiteboard: need reviews (mrbkap, jst) → [sg:critical?] need approval request
> 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.
Depends on: 343037
Bug 343037 appears inappropriate for 220.127.116.11 given that it likely breaks themes. Would the "super-hacky patch" be OK enough for 1.8.0.x?
Comment on attachment 234737 [details] [diff] [review] Super-hacky patch Yeah, let's do this for the stable branch.
Attachment #234737 - Flags: approval18.104.22.168?
I checked this in on trunk to get some bake-time. We should back it out once bug 343037 lands on trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 234737 [details] [diff] [review] Super-hacky patch a=dveditz for 1.8.0 branch
Attachment #234737 - Flags: approval22.214.171.124? → approval126.96.36.199+
Checked in on 1.8.0 branch for 188.8.131.52.
G30rgi: Can you possibly verify this fix with the latest Thunderbird 184.108.40.206 candidate builds? Thanks!
(In reply to comment #55) > G30rgi: Can you possibly verify this fix with the latest Thunderbird 220.127.116.11 > candidate builds? Thanks! > according to tests it is fixed in thunderbird version 18.104.22.168 (20060907)
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 on attachment 235353 [details] [diff] [review] 1.8 branch version of patch if we need it Yeah, I guess we should.
Attachment #235353 - Flags: approval1.8.1?
Comment on attachment 235353 [details] [diff] [review] 1.8 branch version of patch if we need it a=schrep for drivers.
Attachment #235353 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [sg:critical?] need approval request → [checkin needed(1.8 branch)][sg:critical?]
From the summary: "...trouble in mailnews and editor" In Thunderbird 22.214.171.124 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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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-126.96.36.199.en-US.linux-i686.tar.gz dveditz, what exact build were you testing?
I tested both the 188.8.131.52 windows release build as well as the Thunderbird 184.108.40.206 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.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Checked in on 1.8 branch. I missed that approval somehow...
Whiteboard: [checkin needed(1.8 branch)][sg:critical?] → [sg:critical?]
Whiteboard: [sg:critical?] → [sg:high]
(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.
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.
At what point can we open up this bug?
You need to log in before you can comment on or make changes to this bug.