Closed Bug 88124 Opened 23 years ago Closed 23 years ago

replying to message containing "<img src=file>" attaches file

Categories

(MailNews Core :: Composition, defect, P1)

x86
Windows NT
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: jruderman, Assigned: bugzilla)

Details

(Whiteboard: [PDT+] security)

Attachments

(5 files)

An attacker can steal a copy of a file from a Mozilla mail user's hard drive if
- the attacker knows the name and location of the file on the user's hard drive, and
- the attacker can get the victim to reply to a message as HTML or as HTML+plain.

When a user replies to a message containing a literal <img src="file:///...">,
Mozilla attaches that file to the message (if the file exists).

If the file doesn't exist, Mozilla leaves the progress window up but never
finishes sending the message.  The user can cancel out of the Sending Messages
progress window.  If the src given is a directory, then while the progress
window is up, I get an alert "Unable to open the temporary file %.200s.  Check
your 'Temporary Directory' setting and try again" and the message is not sent.

If the user chooses to send the message as plain text only, the attacker does
not recieve a copy of the file.  (Bug 88110 causes the message to be sent as
multipart/related rather than text/plain in this case, but that's not a security
risk.)  I wasn't able to figure out a way for the attacker to make the dialog
not appear or change the default from "plain text only", but I'm not very
familiar with this part of Mozilla mail.


Here's what I pasted into judge (a Netscape mail server) on port 25 using telnet:

-- begin paste --
mail from:jesse
rcpt to:jesse
data
Subject: foop
Content-Type: text/html

foo <img src="file:///C:/WINNT/Profiles/Administrator/Desktop/nss.txt" alt=bar> baz

.

-- end paste --

When I replied to this message, a copy of nss.txt was attached.
cc-ing related people. 
Jean-Francois, can you look into this?
Whiteboard: critical for 0.9.2
security related.  Mark PDT+ for now to get on PDT radar.
Whiteboard: critical for 0.9.2 → [PDT+]critical for 0.9.2
Cc cmanske since I think this is his area.
Whiteboard: [PDT+]critical for 0.9.2 → [PDT+]
Accepting and looking at it...
Status: NEW → ASSIGNED
I have a fix but that cause a little regression: parts are not send anymore...
Attached patch Proposed fix, V1Splinter Review
Please tell me that was the wrong patch. I have no idea what the patch is trying 
to do.
Can somebody in editor team review this patch. Thanks
The patch is the correct one. This is the function that build a list of embedded 
objects for mail editor. Mail compose will use this list to determine the parts 
that need to be attached to the message. I have modified it to skip over 
blockquote node of type "cite" which will prevent use to reattach any object 
received.

Oops, I should block only object with file URL! Let me change it...
OK, as far as I can see, the patch changes nsHTMLEditor::GetEmbeddedObjects() so 
that it does not return objects inside of a <blockquote>. I don't think this is 
the correct fix for the bug. nsHTMLEditor::GetEmbeddedObjects() is not mail 
specific, and is used elsewhere (e.g. by the code that saves a page with images 
to disk), and will be used by publishing.

If you want to persue this approach, you should, I think, not change this method, 
but have the caller filter out elements on the result based on whether they are 
nested inside of a blockquote.

Also, does this really address the problem? Is it always the case that 
"dangerous" images will be inside the blockquote?
ok, I should maybe address the problem right after the quoting (and before the 
user start typing). I can parse the nodes (I can use 
nsHTMLEditor::GetEmbeddedObjects)and either kill any local file url or set an 
attribute on the node that I can check during the send to avoid to attach the 
part.
It's possible for users to edit the quoted text, and add new images in there. 
Your current fix will stop these images being sent. Doing the work at quoting 
time would be better, but why not filter file:// URLs out of the text that gets 
quoted, before inserting into the editor (i.e. before quoting)?
In fact, if I totally suppress the file url, I wonder if I will kill some 
legitimate cases (imagine a file url on a corporate file server). Therefore, I 
think the best solution would be to just add an attribute to the node that we 
can filter during the send. The recipient will get the message, with the URL but 
without a copy of the file.
*** Bug 88079 has been marked as a duplicate of this bug. ***
Attached patch Proposed fix, V2Splinter Review
Here is the deal. Right after inserting the original message body into editor, I 
retreive the embedded objects (nsHTMLEditor::GetEmbeddedObjects) and check if 
they are using a file url. If it's the case, I add an attribute to the node. 
During the send, I look if the embedded object as this new attribute set and if 
it's, I just ignore the part. The original link is not affected.

Varada, can you review?
Also, I fixed couple potential crashes I found while debugging this patch.
Coding style comments:
+    nsString objURL;

Better to use nsAutoString.

+        domElement->SetAttribute(NS_ConvertASCIItoUCS2("DoNotSend"), 
NS_ConvertASCIItoUCS2("true"));

NS_LITERAL_STRING ?

Also, I think we should use a -moz_ attribute name, so it's obvious that this is 
some internal thing.

+        if (NS_SUCCEEDED(domElement->
HasAttribute(NS_ConvertASCIItoUCS2("DoNotSend"), &hasAttribute)))

NS_LITERAL_STRING

Does this patch do the right thing with <file://> URLs in signatures/vCards?
Are the crashes that you fixed the cause of bug 88079?
That's possible, I need to check...
I'll address simon remarks.

>Does this patch do the right thing with <file://> URLs in signatures/vCards?
This patch does not touch the user's signature or the user's vcard.
This patch does not fix bug 88079
Whiteboard: [PDT+] → [PDT+]Have fix
Attached patch proposed fix, v3Splinter Review
in patch v3, I have addressed sfraser concerns and I have also found some 
missing { in the if/else if chain.
I haven't tried the patch, but it looks like it only blocks file:/// URLs from
being attached.  I didn't think of this when I first reported the bug, but if I
mail myself a message with 
  <img src="http://bugzilla.mozilla.org/show_bug.cgi?id=88124"> 
and reply to it, the contents of this (netscape-confidential!) bug are attached
to the reply message.

The only things that should be attached are:
- attachments the user creates (including images the user inserts)
- attachments and images that came with the original message
- images and other objects embedded by (not linked to by!) the sender's
signature or vcard.
Right, this is blocking only file urls. I'll see if I can block more...
Why not just keep a list of images the user has explicitly attached, and then
attach those when sending, rather than scanning the message body in the first
place? 

I don't know if we automatically attach images from files which the user has
attached, if so, then just add stuff to the list recursively, and cull
duplicates at the end.

Do we send file urls when the mail is forwarded as an attachment? My solution
would take care of that, as well.
Attached patch Proposed fix, v4Splinter Review
the patch v4 will block any embedded object that wasn't a part of the original 
message. I have tested it for imap, pop3 and news. 
oops! ignore the changes in nsMsgCompUtils.cpp, it's for another bug...
Priority: -- → P1
Target Milestone: --- → mozilla0.9.3
varada, jess, can you review it?
+    rv = NS_NewURI(getter_AddRefs(uri), (const char *)nsAutoCString(objURL));

Implicit downsizing of a UCS2 string to ASCII is nasty. This would be better 
written using CopyUCS2toASCII(). More comments soon.
+  nsIMsgMessageService * msgService = nsnull;
+  rv = GetMessageServiceFromURI(mQuoteURI, &msgService);

Does this leak? Or is GetMessageServiceFromURI() "special"?

+        PRBool hasAttribute;
+        if (NS_SUCCEEDED(domElement->HasAttribute(NS_LITERAL_STRING("moz-do-not-
send"), &hasAttribute)))
+          if (hasAttribute)
+            continue;

This fails if somehow the element has moz-do-not-send="false". You should check 
the value of the attribute too.
Attached patch Proposed fix, v5Splinter Review
I have addressed all simon's comment. Good catch about msgService, I forget to 
call ReleaseMessageServiceFromURI().
You guys need to write a stack-based class that does GetMessageServiceFromURI()/
ReleaseMessageServiceFromURI().

sr=sfraser on the patch.
r=varada
Fix checked in the trunk and the 0.9.2 branch
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+]Have fix → [PDT+]
Jesse, can you see if this is fixed using the latest branch build since I am 
unable to reproduce the problem? If you prefer just state more detailed steps on 
how I can reproduce.

Assigning this bug to nbaca since sheelar will be out for a couple days.
QA Contact: sheelar → nbaca
Trunk build 2001-07-13-04: WinMe, Linux RH 6.2, Mac 9.04
verifiedtrunk.

I tried the following scenario:

a. In the Browser go to "bugzilla.mozilla.org"
b. Enter a bug number so you view the details in bugzilla (i.e. 88124)
c. Select File|Send Page and address it to yourself
d. In Mail, retrieve the message
   1. Select the message, select the Reply button and send the message back to  
  yourself.
   2. Select the message, select the Reply button, Insert an image and send the 
   message back to yourself (this is to check for regressions).
e. Using 4.x view the message source for d1 (with moz/6.x current builds, if the 
message is too large then you cannot view the entire page source, known bug)
f. Using 4.x view the message source for d2

Actual Results:
1. e: The message source does Not show the "mozilla-banner.gif" reference 
followed by hex characters.
2. f: The message has a reference to the gif file followed by hex characters as 
expected.

I hope this makes sense.  
Branch build: 07-13-06: Win
Branch build: 07-13-04: Linux RH 6.2
Branch build: 07-13-03: Mac 8.6
Verified Fixed.

Esther and Sheela performed the following scenario which nicely displays the 
problem. 


Using an older build such as PR1:
1. User1 telnets a message to User2 which includes the path and a file name on 
User2's system:

--begin paste--
mail from:esther
rcpt to:esther
data
Subject: foop
Content-Type: text/html

foo <img src="file:///C:/graphics/graphic1.jpg" alt=bar> baz

.

--end paste--

Note: User2 never sent the graphic file to User1. User1 just happens to know 
that this file exists on User2's system.

2. User2 receives the message
   a. User2 replies to the message
   b. User2 replies to the same message but also inserts a different graphic 
      file (graphic2.jpg)
3. User1 retrieves message 2a.
4. User1 retrieves message 2b.

Actual Results: 
3. User1 receives the message and now sees the graphic file (graphic1.jpg), bad.
4. User1 receives the message and sees both graphic files (graphic1.jpg and 
graphic2.jpg), bad.

With the build from today, 7/13, the problem no longer occurs:
Actual Results: 
3. User1 never receives the graphic file (graphic1.jpg), as expected.
4. User1 never receives graphic1.jpg but does receive graphic2.jpg, as expected.
Status: RESOLVED → VERIFIED
Removing Netscape-Confidential
Group: netscapeconfidential?
Whiteboard: [PDT+] → [PDT+] security
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: