251297 - files in /tmp world readable (email attachments, helper app docs)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Daniel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Thunderbird 0.7.1

I was comparing treatment of attachments opened directly from emails on
different platforms. I discovered that Linux builds save attachments in /tmp
with world readable rights. This doesn't seem like a good thing. Couldn't
someone else logged onto the same machine read your attachments? It seems like
the user profile is a better place for an attachment cache. Or at the very least
the files could be saved with more restrictive permissions.

Sorry if there's something I'm missing and this bug report is invalid.

Reproducible: Always
Steps to Reproduce:
1. Find an email with an attachment
2 [review]. Open the attachment from within the email by double clicking on it


Actual Results:  
The attachment was saved to /tmp with world readable rights

Expected Results:  
Save it in a more secure location (user profile directory) and/or with more
secure rights (only owner readable).

Comment 1

[Daniel Veditz [:dveditz]]

Also mentioned in bug 258578 comment 5 -- confirming.

This is not a good thing, fix for release. Mozilla mail surely does the same,
should I file another bug or can this one cover both?

Comment 2

[Scott MacGregor]

dveditz, currently the uriloader opens attachments (and external brower clicks
likd PDF files, etc) using the following permission bits in the temp directory:

mTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);

is 0600 the right value to use or should we be using something different?

See:

http://lxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1335

for where the permissions get set on this temporary file.

Comment 3

[Scott MacGregor]

http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsUTF8ConverterService.cpp#118

shows that the utf8 converter is indeed returning an unescaped url even though
we passed in an escaped one.

Also, Dan is there a reason why this is listed as a security senstive bug? This
seems like a normal bug fix to me....just curious.

Comment 4

[Daniel Veditz [:dveditz]]

Was comment 3 intended for bug 243504 instead? That one is not security
sensitive. World-readable files containing potentially sensitive mail is a
security issue.

Comment 5

[Scott MacGregor]

heh yup that comment was for the other bug :) sorry about that. 

what do you think about comment #2? What permissions should the uriloader be
setting on the temp files it creates for attachments and for browser external
helper apps?

Comment 6

[Daniel Veditz [:dveditz]]

Regarding comment 2: 0600 seems like it would do the right thing, but I just
confirmed on brendan's machine that temp attachments *are* coming up world
readable. Either that call's not the one actually creating the attachment or
something below it has gone wonky. Someone's going to have to step through it in
a debugger.

Comment 7

[Scott MacGregor]

When I step through this on windows I see the temp file getting created in the
temp directory with a call to CreateUnique with a value of 0600.

The resulting file permissions:

-rwx------+

which to me says: read, write, execute for the owner of the file.

I don't see any UNIX specific code around this file creating code in the
uriloader. Very strange. 

Comment 8

[Daniel Veditz [:dveditz]]

*** Bug 264372 has been marked as a duplicate of this bug. ***

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

-> file handling where this code is.

Comment 10

[Daniel Veditz [:dveditz]]

files were world readable: bug 107088
Then they weren't readable enough: bug 124307
now they're too readable: <you are here>

As dbaron says in bug 264372 comment 5 MoveFile() isn't the right place to call
FixFilePermissions, it should be up in ExecuteDesiredAction

but watch out for regressing part of bug 213921. I don't know what other actions
might fall into that "else", make sure to call FixFilePermissions --only-- if
the action is saveToDisk.

Comment 11

[Daniel Veditz [:dveditz]]

Attachment: Only change permissions if saving the file

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163020 [details] [diff] [review]
Only change permissions if saving the file

Looks fine to me if biesi's OK with it.

Comment 13

[chris hofmann]

Comment on attachment 163020 [details] [diff] [review]
Only change permissions if saving the file

a=chofmann for the branches

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 163020 [details] [diff] [review]
Only change permissions if saving the file

r=biesi; a bit more context would've been nice

thanks for making this patch.

Comment 15

[Daniel Veditz [:dveditz]]

Checked in to aviary and 1.7 branches. Trunk is waiting on smoketest blockers

Comment 16

[sairuh (rarely reading bugmail)]

vrfy'd fixed with firefox 2004102409-0.9+ and tbird 2004102305-0.8 on linux fc2.
tested files and attachment opened via helper apps (respectively).

Comment 17

[sairuh (rarely reading bugmail)]

forgot to add: while firefox and thunderbird were running, the files opened in
/tmp only had 600 privs. and once I had quit firefox/tbird, the files were
removed (as expected).

Comment 18

[Daniel Veditz [:dveditz]]

Fixed on trunk

Comment 19

[Daniel Veditz [:dveditz]]

Posted on public security lists, removing confidential flag.

Comment 20

[Daniel Veditz [:dveditz]]

adding secunia advisory information to catch duplicate filings: SA12956
http://secunia.com/advisories/12956/

Comment 21

[Juha-Matti Laurio]

(In reply to comment #20)
> adding secunia advisory information to catch duplicate filings: SA12956
> http://secunia.com/advisories/12956/

In this advisory Firefox 1.0 is now marked as patched.
"Solution:

Firefox:
Update to version 1.0."

Comment 22

[Jo Hermans]

*** Bug 292069 has been marked as a duplicate of this bug. ***