Last Comment Bug 411572 - Unnamed attachments reveal full local paths when forwarded inline or edited as new
: Unnamed attachments reveal full local paths when forwarded inline or edited a...
Status: RESOLVED FIXED
: fixed1.8.1.13, privacy
Product: MailNews Core
Classification: Components
Component: Attachments (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9beta4
Assigned To: rsx11m
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-09 14:25 PST by rsx11m
Modified: 2008-07-31 04:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (3.08 KB, patch)
2008-01-09 14:31 PST, rsx11m
standard8: review+
mozilla: superreview+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review

Description rsx11m 2008-01-09 14:25:25 PST
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 2008-01-09 14:31:41 PST
Created attachment 296221 [details] [diff] [review]
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: gavin@gavinsharp.com] 2008-01-09 20:58:03 PST
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 2008-01-10 03:43:35 PST
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 2008-02-07 08:59:24 PST
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 2008-02-07 13:03:40 PST
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 2008-02-21 16:16:25 PST
Mark, how is the review of this patch doing on your end?
Comment 7 Mark Banner (:standard8, limited time in Dec) 2008-02-26 10:50:11 PST
Comment on attachment 296221 [details] [diff] [review]
Proposed fix

Sorry for the delay in reviewing this. r=me.
Comment 8 rsx11m 2008-02-26 11:08:25 PST
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] (use needinfo?) 2008-02-26 16:57:16 PST
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 2008-02-26 17:29:44 PST
> 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] (use needinfo?) 2008-02-26 17:32:25 PST
(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 2008-02-26 18:08:21 PST
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 (:dmose) 2008-02-28 16:43:30 PST
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] 2008-02-29 16:12:55 PST
Comment on attachment 296221 [details] [diff] [review]
Proposed fix

approved for 1.8.1.13, a=dveditz,dmose for release-drivers
Comment 15 rsx11m 2008-02-29 17:44:23 PST
Thanks to both of you - checkin for branch then, please.
Comment 16 Magnus Melin 2008-03-01 03:32:17 PST
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

Note You need to log in before you can comment on or make changes to this bug.