Thunderbird writes attachments to /tmp readable to everyone

[Thomas Mayer]

When I open an attachment of an email in Thunderbird it gets written to disk with permission 644, so it is readable by everyone on the system.

How to repeat: Open an E-Mail, Open an Attachment (e.g. google.png)

$ cd /tmp; ls -lh
-rw-r--r-- 1 theuser thegroup 2,4K Dez 11 10:39 google.png

Instead, Thunderbird should write the file with permissions 600. Plus, to avoid conflicts between users, the file should be written into a directory per user, e.g. /tmp/theuser/google.png or another user specific temp directory.

[Peter Bieringer (pb-bieringer)]

User-Agent: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.0.10) Gecko/20070313 Fedora/1.5.0.10-5.fc6 Firefox/1.5.0.10 pango-text
Build Identifier: 1.5.0.10

On at least Fedora Core every attachment which was openend is saved in /tmp. On a multi user system this can lead to a filename disclosure and therefore to a privacy problem, think about e.g.

/tmp/loveletter-from-girlfriend-xy.doc

Reproducible: Always

Steps to Reproduce:
1. Open attachment "secret-agenda-from-company.ppt" from an e-mail

2. login as different user and list /tmp directory
$ ls -al /tmp/*.ppt
-rw------- 1 peter peter 248832 16. Apr 14:08 /tmp/secret-agenda-from-company.ppt

Actual Results:
File name is unexpectly disclosed to all other non-root users

Expected Results:
File would be stored into a subdirectory in tmp, e.g.
/tmp/peter-thunderbird/secret-agenda-from-company.ppt
and
/tmp/peter-thunderbird is created with permissions 700

[A S Alam (aalam-deactivatedaccount)]

yes, bug as mention is there in fedora

[Peter Bieringer (pb-bieringer)]

Looks like no one cares about it. But I found a workaround. Digging through search engines and strings * |grep -i temp I found, that TEMP is mentioned somewhere in in the binaries. A short test shows, that following would be very helpful at least on Linux:

# cat /etc/profile.d/usertemp.sh
if [ ! -d /tmp/temp-$USER ]; then
        mkdir -m 700 /tmp/temp-$USER
fi
export TEMP=/tmp/temp-$USER

This script creates (if not already existing) a subdirectory in the /tmp folder with proper permissions and also adjusts the TEMP environment variable.

Now every attachement opened with thunderbird would be stored in /tmp/temp-$USER, no other user (except root) can see anything of the file name.

[mlissner (mlissner-michaeljaylissner)]

It's been over a year since the last comment in this message. I hope this doesn't get folded into t-bird 3.0 as well.

I can confirm this on Ubuntu Jaunty...

[mlissner (mlissner-michaeljaylissner)]

Confirming that this is still present in TB 3.0b4pre

Can we please fix this? It's not that complicated, as indicated above.

[Standard8 (standard8)]

Whilst I can see that this is an issue for a few users, I wouldn't block shipping the big upgrade of Thunderbird 3 on it - especially as it has been present since Thunderbird 1.5 at least.

[Kshriram18 (kshriram18)]

Now, how do we include that script into the binary file that modifies that deals with the /tmp directory?

[Kshriram18 (kshriram18)]

I am trying to find the file in the thunderbird 3 repo which deals with storing attachments. For anyone reading this bug, please feel free to submit a patch or propose alternative solutions.

[Mkmelin+mozilla (mkmelin+mozilla)]

Should be around here http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp

[Gosc-tb-project (gosc-tb-project)]

Created attachment 412493
Patch for Bug 377630.

This patch saves attachments in /tmp/thunderbird-$USER/ by default instead of /tmp.

[Benjamin Smedberg (Mozilla) [:bs] (benjamin-smedbergs)]

Comment on attachment 412493
Patch for Bug 377630.

This patch is incorrect for a bunch of different reasons:

* you hardcode thunderbird- into generic platform code which is shared with all mozilla apps.

* What happens if the directory doesn't exist? Do all the users of this code properly create the directory before attempting to stick files in it?

* the nsCAutoString initializer should probably be = NS_LITERAL_CSTRING("/tmp/")

I'm surprised that the path ends with a slash: that sounds unnecessarily complicated because the slash will be stripped by NS_NewNativeLocalFile anyway.

This must have tests.

[Gosc-tb-project (gosc-tb-project)]

Created attachment 413939
Patch.

[Benjamin Smedberg (Mozilla) [:bs] (benjamin-smedbergs)]

Comment on attachment 413939
Patch.

Still no tests, and no answer to the question about actually creating the directory (and what to do if it already exists).

[Vipul (vipulgolchha-gmail)]

Is the above patch correcting the right file? Because bsmedberg already said, the patch is making changes to generic code in xpcom folder.

If not can anybody please point to some location where the code may be found.

[Vipul (vipulgolchha-gmail)]

Created attachment 424393
patch creates the folder with "mozilla-$USER" in /tmp to store the temporary files for the user

[mic1234 (may-gup)]

Created attachment 424590
Patch

This patch is basically the same as posted above , just that it fixes the '0755' folder permission to '0700'.

[Ludovic-mozilla (ludovic-mozilla)]

(In reply to comment #16)
> Created an attachment (id=424590) [details]
> Patch
>
> This patch is basically the same as posted above , just that it fixes the
> '0755' folder permission to '0700'.

You mixed up review flags :-)

[mic1234 (may-gup)]

The patch above doesn't solve the case when there is another directory in the tmp folder, which is of the same name as desired. We can address this problem in two ways :

1. To create a new directory in /tmp every time the application starts up and delete it at shutdown.

2. Assign a user a directory, and create one for him if it doesn't already exists.
   And save this information somewhere.

 I think the second option would be better , as the first one leaves orphan directories in the /tmp folder in case of improper shutdowns.

Now if anybody could point to me some place where i could get to know, that how to save this information, it would be really helpful.

[Vipul (vipulgolchha-gmail)]

why donot we create a folder mozilla- thunderbird in tmp
then in this folder we can have thunderbird-$user folder(700) for each user.
no need of deleting the folders at all .

just store the temp attachment in respective folders and delete them at every exit

[Vipul (vipulgolchha-gmail)]

Created attachment 426850
It fixes the issue of already existing directory.please comment

[Vipul (vipulgolchha-gmail)]

Created attachment 428218
It fixes the issue of already existing directory.please comment

Final patch with Test Case

[Om-brahmana (om-brahmana)]

(In reply to comment #21)
> Created an attachment (id=428218) [details]
> It fixes the issue of already existing directory.please comment
>
> Final patch with Test Case

bsmedberg will do the full review. Here are some of my observations.

This patch only takes care of UNIX and BEOS. What about other operating systems? Though the bug is filed against Linux, I believe we want the behavior to be consistent across operating systems.

>+ PRBool wr = PR_FALSE, ex = PR_FALSE;

Use more descriptive variable names. Example : http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#2017

>+ wr = (per == 0700)?PR_TRUE:PR_FALSE;

Use mnemonics instead of hard-coding the numerical value : http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prio.h#652

Also, when you submit a new patch, mark your older versions of the same patch as obsolete.

[Vipul (vipulgolchha-gmail)]

Created attachment 428479
Patch;Please Comment

improvement :variable name,mnemonics

[Timeless-bemail (timeless-bemail)]

Comment on attachment 428479
Patch;Please Comment

>+ PRBool isWritable = PR_FALSE, isExecutable = PR_FALSE;

please include spaces around (=):
>+ PRInt16 i=1;

this isn't a particularly good variable name:
>+ PRUint32 per;

>+ if (!user) {

indentation following this line is confused, it should be 4-4, but for some reason you're doing something else:
>+ user = PR_GetEnv("USERNAME");
>+ if (!user || !*user) {
>+ user = PR_GetEnv("USER");
>+ if (!user || !*user) {
>+ user = PR_GetEnv("LOGNAME");

if this fails, things are strange.

>+ }
>+ }
>+ }

>+ nsDependentCString path(tPath);
>+ nsDependentCString tem;

we don't typically use nsDependent*String for string operations:
>+ path += "mozilla-";

instead, you can use ns*(Auto)String
>+ path += user;

>+ rv = NS_NewNativeLocalFile(path,PR_TRUE,aFile);
>+ while (!isExecutable || !isWritable) {

You didn't check isSymLink:
>+ ((nsILocalFile *)*aFile)->Exists(&isExecutable);
>+ if (isExecutable) {

you should check the return value of xpcom method calls. perhaps the function you're calling failed:
>+ ((nsIFile *)*aFile)->GetPermissions(&per);

again, your indentation is strange:
>+ isWritable = (per == PR_IRWXU)?PR_TRUE:PR_FALSE;
>+ if (!isWritable) {

Use .Truncate() instead, SetIsVoid is for very very rare cases:
>+ tem.SetIsVoid(PR_TRUE);

We have a nsIFile.createUnique, you might want to use that instead:
>+ tem.AppendInt(i,10);

generally you should have spaces after commas:
>+ rv = NS_NewNativeLocalFile(tem,PR_TRUE,aFile);

You can't do this, you need to use do_QueryInterface:
>+ ((nsILocalFile *)*aFile)->

What happens if the file system is FAT/NTFS or something similarly exotic?

[mic1234 (may-gup)]

>We have a nsIFile.createUnique, you might want to use that instead:
>>+ tem.AppendInt(i,10);

Yes thats right, but here we require to create unique if the condition says so. And reuse the unique if possible. The latter feature missing in CreateUnique()

>What happens if the file system is FAT/NTFS or something similarly exotic?

Could you please explain, I didn't get your point. The functions used are all platform independent. And the directory structure is still the same in FAT/NTFS as in EXT3/4 .

[mic1234 (may-gup)]

Created attachment 432570
Work in Progress

Issues Remaining :

>+ while (!isExecutable || !isWritable) {
>You didn't check isSymLink:

The Symlink is anyway being resolved automatically and thus GetPermissions() is returning the target permissions only. Do I still have to check for Symbolic Link ?

>What happens if the file system is FAT/NTFS or something similarly exotic?

Still Working..

[Dougt-a (dougt-a)]

Comment on attachment 432570
Work in Progress

I'd like to see this be address by the caller. XPCOM is giving out the temp directory, not a subdirectory of the temp directory.

tbird should probably just use a unique name instead of using the real file name to avoid this privacy leak.

[Vipul (vipulgolchha-gmail)]

Created attachment 441256
Patch with new approach

temp files have name cryptic names

[Dougt-a (dougt-a)]

Comment on attachment 441256
Patch with new approach

this is not correct. see my last comment. please make a tbird specific change.

[Bzbarsky (bzbarsky)]

tbird specific change is not good enough. Firefox has the same exact issue; all it takes is to open attachments from your webmail account.

We do NOT want to obfuscate the filename. We've done that in the past, and users hate it. They want to find their files.

The right fix really is the restricted parent directory fix. If your temp dir (or otherwise download dir) is on a file system that does not support restricting permissions, you already lose because the attacker doesn't have to try to deal with filenames at all: just grab all the file data and analyze at leisure.

[Vipul (vipulgolchha-gmail)]

Created attachment 442962
Patch with directory fix

patch with test case.
create user restricted directory to store the temporary files.

[Vipul (vipulgolchha-gmail)]

Created attachment 443575
Patch

patch fixes the issue by creating user specific directory with 700 permission only in the case user opts for Openwith option ,else System temp is used for other purpose .

[Bzbarsky (bzbarsky)]

Comment on attachment 443575
Patch

I don't understand why this is still unix-only, or making up its own way to get the temp dir.

Let's get that sorted out before we worry about the fact that this leaks lFile, or the broken string it passes to SetLeafName, or the fact that it clearly doesn't compile and hence wasn't tested)?

[Vipul (vipulgolchha-gmail)]

Created attachment 445968
PATCH

patch fixes the issue by creating user specific directory with 700 permission
when download directory is requested.

[Standard8 (standard8)]

Moving to the core component - as per comment 30 this affects Firefox as well, so once this gets review it'll be better for it to be in core (as approval flags are available there).

[Bzbarsky (bzbarsky)]

Comment on attachment 445968
PATCH

>@@ -63,16 +63,17 @@
>+#include "prenv.h"

This is no longer needed, right?

>@@ -433,19 +434,67 @@

You may want to add "showfunc = true" to the [diff] section in your ~/.hgrc.

>+ PRUint32 permissions;
>+ rv = dir->GetPermissions(&permissions);
>+ if (permissions != PR_IRWXU) {

Right before that code, I suggest adding a comment like "Make sure that only the current user can read the filenames we end up creating".

You need to check rv there (and presumably assume the permissions are not good if it's a failure code).

>+ nsAutoString tpath;

You don't need this; more on that later.

>+ PRBool isPrivate = PR_FALSE, isExisting = PR_FALSE;

Move these declarations down to where you use them. Same for 'i' and 'lFile'. This isn't C. ;)

>+ nsILocalFile *lFile ;

nsCOMPtr<nsILocalFile>, please. That would help you avoid the lFile leaks you have all over here.

>+ user="mozillaUser";

Spaces around '=', please.

>+ nsCAutoString path;
>+ nsCAutoString tem;
>+ path.AssignWithConversion(tpath);

You just lost any non-ASCII chars in the temp dir name. That's bad.

>+ path += "/mozilla-";
>+ path += user;
>+ rv = NS_NewNativeLocalFile(path, PR_TRUE, &lFile);

You should probably just clone dir, create an nsAutoString with the concatenation of "/mozilla-" and user in it, then append that autostring to the clone. That'll work correctly with non-ASCII paths, etc. And use create() on the resulting nsIFile to create the file.

Also, do you need to worry about usernames containing path separators here?

>+ while (NS_SUCCEEDED(rv) && (!isExisting || !isPrivate)) {

I'd really prefer to replace this by something that just reuses createUnique.... but I guess reusing the existing dir if one is there is nice. So let's leave it as-is.

>+ rv = lFile->Exists(&isExisting);

This rv is never checked by anyone. If it doesn't matter, don't assign it; otherwise check it. I think you need to check it...

>+ rv = lFile->GetPermissions(&permissions);

And check that too.

>+ isPrivate = (permissions == PR_IRWXU)?PR_TRUE:PR_FALSE;

  isPrivate = (permissions == PR_IWXU);

No need for the ?: stuff.

>+ tem.Truncate(0);
>+ tem += path;

This is the same as |tem = path|, but again you want to be using append() on nsIFiles here....

I'm really sorry about the lag. :( This patch is looking much better, though!

[Kshriram18 (kshriram18)]

Created attachment 728732
Patch for filename disclosure in /tmp

WIP
Based on last patch.
At the moment, test fails @ assertion on line 20:
do_check_true(isExists && isWritable);

[Kshriram18 (kshriram18)]

Created attachment 728734
Patch for filename disclosure in /tmp

This patch includes test file in addition to content in last patch.

[Ludovic-mozilla (ludovic-mozilla)]

(In reply to Shriram (irc: Mavericks) from comment #38)
> Created attachment 728734
> Patch for filename disclosure in /tmp
>
> This patch includes test file in addition to content in last patch.

Any reasons you didn't request reviews on those patches ?

[Kshriram18 (kshriram18)]

(In reply to Ludovic Hirlimann [:Usul] from comment #39)
> (In reply to Shriram (irc: Mavericks) from comment #38)
> > Created attachment 728734
> > Patch for filename disclosure in /tmp
> >
> > This patch includes test file in addition to content in last patch.
>
> Any reasons you didn't request reviews on those patches ?

The last patch's adds the test file only to the previous patch.
I thought to post an updated patch and request review after fixing the test failure mentioned in comment #37

[Enrico-tagliavini (enrico-tagliavini)]

There is also another security problem related to this bug: if you open an encrypted file with kgpg (just an example, other application can do the same, even gpg -d with a graphical ask-pass), it decrypt the file, saving another one in the same folder of the original one (/tmp) but with permission as per user umask, usually 0022, so world readable.

[Plst (plst)]

This over 7 year old bug is security related and still valid in Thunderbird 31.3. So why is the patch not approved? On home computers this is not a big issue but in companies with multi-user setup is really is, so this needs to be fixed fast.

[Bzbarsky (bzbarsky)]

The patch is not approved because the patch author doesn't think it's ready. See comment 40, which I assume you _did_ read before commenting?

[Plst (plst)]

Yes, I have read that comment. But it is two years old, so the question still remains the same: Why is it not fixed yet? If the author doesn't have the time to finish it, maybe someone else could help out? Also someone else than the author of the patch is assigned to this bug and therefore responsible for it. This was just a reminder that the bug is still valid, still a big security issue for professional users and that it hopefully won't be open for another 7 years. Still appreciating what the devs do in their free time.

[Plst (plst)]

vipul, which is assigned to this bug, was last active 2010, so please remove him from this bug and change the status to NEW.

[Bzbarsky (bzbarsky)]

I don't think you should pay so much attention to the "assignee" field or the status. Both are often bogus.

[Thomas Mayer (thomas303)]

As the discussion about this was going on for 8 years in the mozilla community, I suggest to at least set permissions right in the distros.

For the moment, there is only one path (which is /tmp) and there is only the original name used. That said, concurrent users could overwrite their temporary files to each other. Setting permissions right would avoid that in addition to solving the security problem. And it's still better than allowing users to overwrite files of other users to avoid error messages. Plus, privacy is an issue here as users can read private files of other users.

On single user systems, there might not be a noticable change to users. So, what should it break? It's still not a perfect concept but a big improvement in terms of security. The rest can be done later in a nice fashion.

After setting permissions right in the distros you can still wait another 8 years and see which solution mozilla community came up with. Possible we see an importance change to 'high' in between (say 4 years or so).

[Thomas Mayer (thomas303)]

I was wrong. Not overwrite, just read. Which makes it even less probable to break things.

[VON (aghsistemas)]

Bug continues, all users of thunderbird use /tmp as 755 so everybody can read attachments that one user has opened. Is there any straight solution ? It´s a great fail of security.

[Thomas Mayer (thomas303)]

Using Thunderbird 38.8.0 in Ubuntu 16.04, when I open a pdf I now get a

-r-------- 1 thomas thomas 19K Jun 16 18:28 filename.pdf

So nobody can read the file, which is 95% of the security fix. The remaining 5% would be to not expose the file name to other users.

That's exactly how it is done for Mozilla Firefox 47.0/Ubuntu 16.04:

Firefox now uses a directory which is only accessible by the user:

drwx------ 1 thomas thomas 1,9K Jun 16 18:08 mozilla_thomas0

Thereby, using Firefox, the file names of temporary files in the directory are no longer exposed to other users. Would be great to have the same behaviour in Thunderbird as well.