Last Comment Bug 191817 - Privileged script insertion in attachment content-type header
: Privileged script insertion in attachment content-type header
Status: VERIFIED FIXED
[sg:mustfix] [fixed1.3]
:
Product: MailNews Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla1.3final
Assigned To: (not reading, please use seth@sspitzer.org instead)
: John Unruh
Mentors:
Depends on:
Blocks: 88314
  Show dependency treegraph
 
Reported: 2003-02-03 16:51 PST by Mitchell Stoltz (not reading bugmail)
Modified: 2008-07-31 01:24 PDT (History)
5 users (show)
roc: blocking1.3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch, diff -uwN (4.57 KB, patch)
2003-02-05 05:18 PST, georgi - hopefully not receiving bugspam
no flags Details | Diff | Splinter Review
modified patch, Georgi's original patch cleaned up (6.18 KB, patch)
2003-02-06 17:24 PST, (not reading, please use seth@sspitzer.org instead)
sspitzer: review+
sspitzer: superreview+
asa: approval1.3b+
Details | Diff | Splinter Review
local folder that has an evil message (1.20 KB, text/plain)
2003-02-06 19:16 PST, (not reading, please use seth@sspitzer.org instead)
no flags Details
Patch for 1.0.x branch (6.05 KB, patch)
2003-02-07 15:20 PST, Mitchell Stoltz (not reading bugmail)
sspitzer: review+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2003-02-03 16:51:58 PST
From Georgi:

There is a serious problem in the mail client, which seems to be exploitable.
By choosing to save or open an attachment in specially crafted email, js can be
executed with system privilege.
Attached is Templates folder from my Local Folders - believe copying it in Local
Folders probably with another name will do.

Another way to reproduce is to create email with attachment then set the
content-type to:

Content-Type: t'+eval(unescape("alert%28Components.classes%29"))+'ext/html;

The problem is in 

./mailnews/base/resources/content/msgHdrViewOverlay.js" line 912 of 1139
which is later eval()ed. The single quote changes a string to code which is
eval()ed.

If I remember correctly, I have examined this code before and the bug
wasn't there, but I am not completely sure.

The fix seems simple - replacing ' with \' and probably checking for double
quotes also or better yet not use eval() in
this and in other cases. Shall check the above file for similar bugs.

The command which turned my attention to this on a linux box was:
find . -iname "*.js" | xargs grep -i 'eval('

Georgi Guninski
Comment 1 Mitchell Stoltz (not reading bugmail) 2003-02-03 17:09:30 PST
I have reproduced the bug. There are lots of evals in msgHdrViewOverlay.js, we
should probably check them all to make sure they're not eval-ing any raw data
from the message.
Comment 2 georgi - hopefully not receiving bugspam 2003-02-04 07:49:45 PST
IMHO this is blocking 1.3b.
Suggest that the evals in msgHdrViewOverlay.js be removed and the code is done
this way.
The arguments are passed in an array - e.g. thearray["contenttype"] = "string".
The eval() be replaced with something like this:
if(func=="openattachment") openAttachment(thearray) else if (func=="saveas")
saveAs(thearray).

The changes will be small and risk of regression is very low.

I believe I can code the changes in case there is chance someone approve them later.
Comment 3 Mitchell Stoltz (not reading bugmail) 2003-02-04 11:40:51 PST
That would be great. Georgi, if you can code the changes, I will get the
necessary reviews and approval.
Comment 4 georgi - hopefully not receiving bugspam 2003-02-05 05:18:44 PST
Created attachment 113581 [details] [diff] [review]
proposed patch, diff -uwN 

Fixes the js bug, tested on linux, everything seems to work. Will explain in
another comment.
Comment 5 georgi - hopefully not receiving bugspam 2003-02-05 05:29:29 PST
Removes all evals in msgHdrViewOverlay.js.
Most of the old calls still remain, but are commented with comments starting
with "!", such comments can be safely discarded.
eval()s are removed this way:
eval(func+arg {string}) is replaced by: dofunc(funcname{string},arg{object}).
Before, arg was string, now it is object.

Another change is replacing element.setAttribute(arg,string) to element.arg=object.

Also changed the argument of the oncommand generated handler from string to
object (not sure whether this is needed, worked without it also, but seems
dangerous, because it is hidden eval).

the diff context seems somewhat strange (function names are "shifted") in the
beginning, examine the produced output.

Comment 6 georgi - hopefully not receiving bugspam 2003-02-05 05:31:17 PST
The exploit works remotely at least from imap servers.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-02-06 14:57:52 PST
Blocks 1.3 for sure. I don't know that we'd hold 1.3beta for this, but we'll
check it in if it's ready in time.

The patch looks good to me but I'm no Javascript wizard. Let's get some review...
Comment 8 (not reading, please use seth@sspitzer.org instead) 2003-02-06 15:38:08 PST
I'll go look at the patch, test it, and review it.

I might attach a new patch, one with the ! comments removed.

I expect to have this done by 5 pm PST today so we can land today.

Comment 9 (not reading, please use seth@sspitzer.org instead) 2003-02-06 15:45:07 PST
ok, I've got a test message that reproduces the problem.

wow, great catch, Georgi!

taking from mstoltz, as I'll drive this in.
Comment 10 (not reading, please use seth@sspitzer.org instead) 2003-02-06 17:24:33 PST
Created attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up
Comment 11 (not reading, please use seth@sspitzer.org instead) 2003-02-06 17:34:38 PST
ok, I've tested this patch, made sure my tests went through all the modified
code, and made sure everything works.

with Georgi's fix, I now get the unknown content handler dialog on the evil message.

I'll seek approval to land this in 1.3 beta.

thanks again Georgi.
Comment 12 (not reading, please use seth@sspitzer.org instead) 2003-02-06 17:35:38 PST
Comment on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

since this is really Georgi's patch, with some minor cleanup, marking
r/sr=sspitzer

seeking approval from asa.
Comment 13 Asa Dotzler [:asa] 2003-02-06 18:15:49 PST
Comment on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

a=asa (on behalf of drivers) for checkin to 1.3beta.
Comment 14 (not reading, please use seth@sspitzer.org instead) 2003-02-06 18:38:01 PST
fix checked in.

I'll attach a local folder with an evil message that can be used as a test case.
Comment 15 (not reading, please use seth@sspitzer.org instead) 2003-02-06 19:16:50 PST
Created attachment 113760 [details]
local folder that has an evil message

for QA:

exit mozilla,
save this file under C:\Documents and Settings\Administrator\Something
Data\Mozilla\Profiles\default\xxxxxx.slt\Mail\Local Folders\bug191817
restart mozilla, then find the folder named "bug191817", select the one
message, and then double click on the attachment in the message.
Comment 16 georgi - hopefully not receiving bugspam 2003-02-07 09:47:59 PST
Seth, thanks for the patch cleaning :)
btw, I am not sure the oncommand handler ever gets executed.

I suggest cleaning as much of the eval()s in js as possible just as a preventive
measure.
Also when tested this should be backported to the branch.
Comment 17 Daniel Veditz [:dveditz] 2003-02-07 14:12:33 PST
Comment on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

nominating for the 1.0 branch
Comment 18 (not reading, please use seth@sspitzer.org instead) 2003-02-07 14:16:32 PST
> Seth, thanks for the patch cleaning :)

no problem.  turns out, neil (neil@parkwaycc.co.uk) has an even better way to
clean this up.  see bug #192318

> btw, I am not sure the oncommand handler ever gets executed.

it does, I tested it.  see the "File | Attachments" menu as well. 

> I suggest cleaning as much of the eval()s in js 
> as possible just as a preventive measure.

I've logged a bug on this, see bug #192317

> Also when tested this should be backported to the branch.

mstoltz, is this something you plan on landing to the mozilla 1.02 / netscape
7.0x branch?

thanks again Georgi for finding this problem and coming up with a fix.  
I really appreciate it.
Comment 19 John Unruh 2003-02-07 14:24:41 PST
Tested on branch and the Feb 7 WinXP trunk. In the branch build I receive an
alert. In the trunk build I simply open an attachment.
Comment 20 Mitchell Stoltz (not reading bugmail) 2003-02-07 15:09:25 PST
The bug has not yet been fixed on the branch. The alert that says
"nsXPCComponents::Classes" or somesuch means that the exploit succeeded. I'm
preparing a branch patch.
Comment 21 Mitchell Stoltz (not reading bugmail) 2003-02-07 15:20:56 PST
Created attachment 113846 [details] [diff] [review]
Patch for 1.0.x branch
Comment 22 Mitchell Stoltz (not reading bugmail) 2003-02-07 15:21:57 PST
Comment on attachment 113846 [details] [diff] [review]
Patch for 1.0.x branch

Seth, can you take a look at this patch for the 1.0 branch and make sure I
ported it correclty? (I tested it and it works, but I want to be sure)
Comment 23 (not reading, please use seth@sspitzer.org instead) 2003-02-07 15:31:33 PST
Comment on attachment 113846 [details] [diff] [review]
Patch for 1.0.x branch

r=sspitzer.

as long as you test the attachment tree and the File | Attachment menu items,
and everything works fine, then I think we're safe.
Comment 24 Mitchell Stoltz (not reading bugmail) 2003-02-07 15:34:32 PST
I tested all of those things; everything looks fine.
Comment 25 bmartin 2003-02-12 15:02:38 PST
Marking Verified Fixed on the Branch 2002-02-10-09

Current expected behavior:

"Alert - [object nsXPCComponents_Classes]" dialog will not appear after
selecting the attachment test ( attach.txt specified in comment #15 ).

Double clicking the attach.txt file in the local folders "test" email, will
display the "Downloading attach.txt" dialog with options to Open or Save file to
disk.
Comment 26 nhottanscp 2003-02-17 09:57:34 PST
The change seems to create a regression bug 193142.
Comment 27 (not reading, please use seth@sspitzer.org instead) 2003-02-20 17:28:05 PST
yes, it did cause a regression.

I removed a necessary call to escape()

this needs to land in the trunk and on the 1.0 branch

Index: base/resources/content/msgHdrViewOverlay.js
===================================================================
RCS 
file: /cvsroot/mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js,v
retrieving revision 1.101
diff -u -w -r1.101 msgHdrViewOverlay.js
--- base/resources/content/msgHdrViewOverlay.js	7 Feb 2003 02:36:18 -0000
	1.101
+++ base/resources/content/msgHdrViewOverlay.js	21 Feb 2003 00:36:48 -0000
@@ -925,7 +925,7 @@
   var obj = new Object();
   obj.contentType = aAttachment.contentType;
   obj.url = aAttachment.url;
-  obj.displayName = aAttachment.displayName;
+  obj.displayName = escape(aAttachment.displayName);
   obj.messageUri = aAttachment.uri;
   return obj;
 }

Comment 28 Daniel Veditz [:dveditz] 2003-02-20 19:02:49 PST
NB: this fix was never checked into the 1.0 branch. There is no regression to
fix on the 1.0 branch until this lands.
Comment 29 (not reading, please use seth@sspitzer.org instead) 2003-02-21 09:08:58 PST
if http://bugzilla.mozilla.org/attachment.cgi?id=113846&action=view lands in
1.0x, we'd also want
http://bugzilla.mozilla.org/attachment.cgi?id=114584&action=view
Comment 30 Asa Dotzler [:asa] 2003-02-28 00:58:48 PST
Is anything further needed here for the 1.3branch and release?
Comment 31 (not reading, please use seth@sspitzer.org instead) 2003-03-02 08:52:12 PST
this was checked into the trunk before we branched for 1.3 final.

02/06 18:36 security fix was checked in to the trunk
02/20 supplimental fix was checked in to the trunk.
02/21 we branch for 1.3 final.

I've also double checked and the fix is on 1.3 final.
Comment 32 Daniel Veditz [:dveditz] 2004-04-08 18:39:03 PDT
Comment on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

branch dead

Note You need to log in before you can comment on or make changes to this bug.