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

VERIFIED FIXED in mozilla0.9.3

Status

MailNews Core
Composition
P1
critical
VERIFIED FIXED
17 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: Jean-Francois Ducarroz)

Tracking

Trunk
mozilla0.9.3
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] security)

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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.

Comment 1

17 years ago
cc-ing related people. 

Comment 2

17 years ago
Jean-Francois, can you look into this?

Updated

17 years ago
Whiteboard: critical for 0.9.2

Comment 3

17 years ago
security related.  Mark PDT+ for now to get on PDT radar.
Whiteboard: critical for 0.9.2 → [PDT+]critical for 0.9.2
(Reporter)

Comment 4

17 years ago
Cc cmanske since I think this is his area.

Updated

17 years ago
Whiteboard: [PDT+]critical for 0.9.2 → [PDT+]
(Assignee)

Comment 5

17 years ago
Accepting and looking at it...
Status: NEW → ASSIGNED
(Assignee)

Comment 6

17 years ago
I have a fix but that cause a little regression: parts are not send anymore...
(Assignee)

Comment 7

17 years ago
Created attachment 40638 [details] [diff] [review]
Proposed fix, V1

Comment 8

17 years ago
Please tell me that was the wrong patch. I have no idea what the patch is trying 
to do.
(Assignee)

Comment 9

17 years ago
Can somebody in editor team review this patch. Thanks
(Assignee)

Comment 10

17 years ago
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...

Comment 11

17 years ago
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?
(Assignee)

Comment 12

17 years ago
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.

Comment 13

17 years ago
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)?
(Assignee)

Comment 14

17 years ago
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.

Comment 15

17 years ago
*** Bug 88079 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

17 years ago
Created attachment 40666 [details] [diff] [review]
Proposed fix, V2
(Assignee)

Comment 17

17 years ago
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?
(Assignee)

Comment 18

17 years ago
Also, I fixed couple potential crashes I found while debugging this patch.

Comment 19

17 years ago
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?

Comment 20

17 years ago
Are the crashes that you fixed the cause of bug 88079?
(Assignee)

Comment 21

17 years ago
That's possible, I need to check...
(Assignee)

Comment 22

17 years ago
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.
(Assignee)

Comment 23

17 years ago
This patch does not fix bug 88079
Whiteboard: [PDT+] → [PDT+]Have fix
(Assignee)

Comment 24

17 years ago
Created attachment 40703 [details] [diff] [review]
proposed fix, v3
(Assignee)

Comment 25

17 years ago
in patch v3, I have addressed sfraser concerns and I have also found some 
missing { in the if/else if chain.
(Reporter)

Comment 26

17 years ago
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.
(Assignee)

Comment 27

17 years ago
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.
(Assignee)

Comment 29

17 years ago
Created attachment 41067 [details] [diff] [review]
Proposed fix, v4
(Assignee)

Comment 30

17 years ago
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. 
(Assignee)

Comment 31

17 years ago
oops! ignore the changes in nsMsgCompUtils.cpp, it's for another bug...
(Assignee)

Updated

17 years ago
Priority: -- → P1
Target Milestone: --- → mozilla0.9.3
(Assignee)

Comment 32

17 years ago
varada, jess, can you review it?

Comment 33

17 years ago
+    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.

Comment 34

17 years ago
+  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.
(Assignee)

Comment 35

17 years ago
Created attachment 41318 [details] [diff] [review]
Proposed fix, v5
(Assignee)

Comment 36

17 years ago
I have addressed all simon's comment. Good catch about msgService, I forget to 
call ReleaseMessageServiceFromURI().

Comment 37

17 years ago
You guys need to write a stack-based class that does GetMessageServiceFromURI()/
ReleaseMessageServiceFromURI().

sr=sfraser on the patch.

Comment 38

17 years ago
r=varada
(Assignee)

Comment 39

17 years ago
Fix checked in the trunk and the 0.9.2 branch
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+]Have fix → [PDT+]

Comment 40

17 years ago
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

Comment 41

17 years ago
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.  

Comment 42

17 years ago
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?
(Reporter)

Updated

14 years ago
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.