411572 - Unnamed attachments reveal full local paths when forwarded inline or edited as new

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[rsx11m]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b3pre) Gecko/2008010802 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b3pre) Gecko/2008010802 SeaMonkey/2.0a1pre

Initially observed by Rimas Kudelis, bug 190298 comment #19.

When an attachment has no name, forwarding inline or editing as new results in the URI used as the attachment name. Following the discussion in bug 176016 comment #7, exposing relevant information on the profile or user/host-related name and file structures in an e-mail should be avoided.

The "file:" URI may potentially expose the user name and local file structure by providing the full path to the temporary file. Thus, the fix for bug 176016 should be extended to also include this URI type.


Reproducible: Always

Steps to Reproduce:

Forward inline or edit as new a message with unnamed attachment:
> Content-Type: image/png
> Content-Transfer-Encoding: base64
> Content-Disposition: inline

Actual Results:  

Full path to temporary file, here exposing user name in Windows XP:
> Content-Type: image/png;
>  name="file:///C:/DOCUME~1/(user)/LOCALS~1/TEMP/nsmail.png"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline;
>  filename="file:///C:/DOCUME~1/(user)/LOCALS~1/TEMP/nsmail.png"


Expected Results:  

Neutral attachment name similar to mailbox:
> Content-Type: image/png;
>  name="Attached Message Part"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline;
>  filename="Attached Message Part"

Comment 1

[rsx11m]

Attachment: Proposed fix

This patch hides the "file:" attachment name generated from the URI if no name is explicitly set. It has no effect on manually attached files, or files passed on the command line with the "-compose" option, as for those an explicit attachment name is generated. Tested with forwarding inline and editing as new, both for the original message and a forwarded message that was opened from a message/rfc822 attachment.

The other option would be to process the temporary file name through a basename function, similar to attachments passed on the command line. However, as the temporary file name doesn't contain any significant information, using the generic partAttachmentSafeName appears to be sufficient.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Are you going to ask for review on the patch?

(See http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree if you're not familiar with the Mozilla development process.)

Comment 3

[rsx11m]

Done - thanks for the reminder and the link. I just wanted to wait until this has been confirmed, before officially requesting a review.

Comment 4

[David :Bienvenu]

Comment on attachment 296221 [details] [diff] [review]
Proposed fix

ideally, wouldn't we use the leaf name of the file w/o the whole path?

Comment 5

[rsx11m]

Thanks David.

> wouldn't we use the leaf name of the file w/o the whole path?

That's what I meant with "temporary file name doesn't contain any significant information" in comment #1. Given that the file is temporary, its name "nsmail*" doesn't seem to make much sense. Thus, I figured treating it similarly as the other URI's gives it a more descriptive name.

Comment 6

[rsx11m]

Mark, how is the review of this patch doing on your end?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 296221 [details] [diff] [review]
Proposed fix

Sorry for the delay in reviewing this. r=me.

Comment 8

[rsx11m]

Thanks, marking for checkin then.

As this is a privacy issue, should this go into trunk only or is this also suitable for the branch?

Comment 9

[Reed Loden [:reed]]

Checking in mailnews/compose/resources/content/MsgComposeCommands.js;
/cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.411; previous revision: 1.410
done
Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.125; previous revision: 1.124
done

Comment 10

[rsx11m]

> Target Milestone | --- | mozilla1.9beta4

Ok, I'll take that as a "no" for the branch then.
The path issue is probably benign enough...

Comment 11

[Reed Loden [:reed]]

(In reply to comment #10)
> > Target Milestone | --- | mozilla1.9beta4
> 
> Ok, I'll take that as a "no" for the branch then.
> The path issue is probably benign enough...

You can still request approval. The TM has nothing to do with whether the patch can land on the branch or not. Make sure the patch applies on the branch and then request approval1.8.1.13.

Comment 12

[rsx11m]

Comment on attachment 296221 [details] [diff] [review]
Proposed fix

Ah, I see. I verified that the code of the AddAttachment() function is identical in trunk and branch versions, and I had tested it against the branch before posting.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

For defensive programming, I could imagine a lot more types of URI schemes in that blacklist.  Maybe gecko wants an isInternalProtocolScheme() function somewhere.  I could also imagine this being a whitelist rather than a blacklist, though it might break functionality that we care about.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 296221 [details] [diff] [review]
Proposed fix

approved for 1.8.1.13, a=dveditz,dmose for release-drivers

Comment 15

[rsx11m]

Thanks to both of you - checkin for branch then, please.

Comment 16

[Magnus Melin [:mkmelin]]

Checked in on the MOZILLA_1_8_BRANCH

Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.72.2.32; previous revision: 1.72.2.31
done
Checking in mailnews/compose/resources/content/MsgComposeCommands.js;
/cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.369.2.18; previous revision: 1.369.2.17
done