191817 - Privileged script insertion in attachment content-type header

Status: VERIFIED FIXED

(MailNews Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

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)]

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]

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)]

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]

Attachment: 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]

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]

The exploit works remotely at least from imap servers.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [e-mail] instead)]

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 [e-mail] instead)]

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 [e-mail] instead)]

Attachment: modified patch, Georgi's original patch cleaned up

Comment 11

[(not reading, please use [e-mail] instead)]

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 [e-mail] instead)]

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]]

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 [e-mail] instead)]

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 [e-mail] instead)]

Attachment: 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]

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]]

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 [e-mail] instead)]

> Seth, thanks for the patch cleaning :)

no problem.  turns out, neil ([e-mail]) 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]

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)]

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)]

Attachment: Patch for 1.0.x branch

Comment 22

[Mitchell Stoltz (not reading bugmail)]

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 [e-mail] instead)]

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)]

I tested all of those things; everything looks fine.

Comment 25

[bmartin]

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]

The change seems to create a regression bug 193142.

Comment 27

[(not reading, please use [e-mail] instead)]

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]]

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 [e-mail] instead)]

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]]

Is anything further needed here for the 1.3branch and release?

Comment 31

[(not reading, please use [e-mail] instead)]

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]]

Comment on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

branch dead