The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9beta4

Status

MailNews Core
Attachments
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

({fixed1.8.1.13, privacy})

Trunk
mozilla1.9beta4
fixed1.8.1.13, privacy

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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"
(Assignee)

Comment 1

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

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: privacy
Version: unspecified → Trunk
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.)
(Assignee)

Updated

9 years ago
Attachment #296221 - Flags: superreview?(bienvenu)
Attachment #296221 - Flags: review?(bugzilla)
(Assignee)

Comment 3

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

Updated

9 years ago
Keywords: regression

Updated

9 years ago
Keywords: regression

Comment 4

9 years ago
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?
Attachment #296221 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 5

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

Comment 6

9 years ago
Mark, how is the review of this patch doing on your end?
Comment on attachment 296221 [details] [diff] [review]
Proposed fix

Sorry for the delay in reviewing this. r=me.
Attachment #296221 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 8

9 years ago
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?
Keywords: checkin-needed
Assignee: nobody → rsx11m.pub
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Comment 10

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

Ok, I'll take that as a "no" for the branch then.
The path issue is probably benign enough...
(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.
(Assignee)

Comment 12

9 years ago
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.
Attachment #296221 - Flags: approval1.8.1.13?
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 on attachment 296221 [details] [diff] [review]
Proposed fix

approved for 1.8.1.13, a=dveditz,dmose for release-drivers
Attachment #296221 - Flags: approval1.8.1.13? → approval1.8.1.13+
(Assignee)

Comment 15

9 years ago
Thanks to both of you - checkin for branch then, please.
Keywords: checkin-needed

Comment 16

9 years ago
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
Keywords: checkin-needed → fixed1.8.1.13
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.