Privileged script insertion in attachment content-type header

VERIFIED FIXED in mozilla1.3final

Status

MailNews Core
Security
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.3final
x86
Windows 2000
Bug Flags:
blocking1.3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:mustfix] [fixed1.3])

Attachments

(3 attachments, 1 obsolete attachment)

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
(Reporter)

Comment 1

15 years ago
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.
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 3

15 years ago
That would be great. Georgi, if you can code the changes, I will get the
necessary reviews and approval.
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.
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.

The exploit works remotely at least from imap servers.
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...
Flags: blocking1.3+
Whiteboard: [sg:mustfix]
Attachment #113581 - Flags: superreview?(sspitzer)
Attachment #113581 - Flags: review?(sspitzer)
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.

ok, I've got a test message that reproduces the problem.

wow, great catch, Georgi!

taking from mstoltz, as I'll drive this in.
Assignee: mstoltz → sspitzer
Status: ASSIGNED → NEW
Created attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up
Attachment #113581 - Attachment is obsolete: true
Attachment #113581 - Flags: superreview?(sspitzer)
Attachment #113581 - Flags: review?(sspitzer)
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.
Status: NEW → ASSIGNED
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.
Attachment #113756 - Flags: superreview+
Attachment #113756 - Flags: review+
Attachment #113756 - Flags: approval1.3b?

Comment 13

15 years ago
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.
Attachment #113756 - Flags: approval1.3b? → approval1.3b+
fix checked in.

I'll attach a local folder with an evil message that can be used as a test case.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
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.
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 on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

nominating for the 1.0 branch
Attachment #113756 - Flags: approval1.0.x?
> 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

15 years ago
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.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 20

15 years ago
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.
(Reporter)

Comment 21

15 years ago
Created attachment 113846 [details] [diff] [review]
Patch for 1.0.x branch
(Reporter)

Comment 22

15 years ago
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)
Attachment #113846 - Flags: review?(sspitzer)
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.
Attachment #113846 - Flags: review?(sspitzer) → review+
(Reporter)

Comment 24

15 years ago
I tested all of those things; everything looks fine.

Comment 25

15 years ago
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.
Group: security

Comment 26

15 years ago
The change seems to create a regression bug 193142.
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;
 }

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

15 years ago
Is anything further needed here for the 1.3branch and release?
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.
Whiteboard: [sg:mustfix] → [sg:mustfix] [fixed in 1.3]
Target Milestone: --- → mozilla1.3final

Updated

15 years ago
Whiteboard: [sg:mustfix] [fixed in 1.3] → [sg:mustfix] [fixed1.3]

Updated

15 years ago
Blocks: 88314
Comment on attachment 113756 [details] [diff] [review]
modified patch, Georgi's original patch cleaned up

branch dead
Attachment #113756 - Flags: approval1.0.x?
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.