[Bug 1401454] Re: Thunderbird writes attachments to /tmp readable to everyone
Bug Watch Updater
1401454 at bugs.launchpad.net
Fri Dec 12 01:18:54 UTC 2014
Launchpad has imported 42 comments from the remote bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=377630.
If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.
------------------------------------------------------------------------
On 2007-04-16T12:12:56+00:00 Pb-bieringer wrote:
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/0
------------------------------------------------------------------------
On 2007-04-24T10:39:39+00:00 A S Alam wrote:
yes, bug as mention is there in fedora
Reply at: https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/1
------------------------------------------------------------------------
On 2008-03-20T18:59:02+00:00 Pb-bieringer wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/2
------------------------------------------------------------------------
On 2009-05-11T17:42:23+00:00 mlissner wrote:
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...
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/3
------------------------------------------------------------------------
On 2009-10-16T19:40:38+00:00 mlissner wrote:
Confirming that this is still present in TB 3.0b4pre
Can we please fix this? It's not that complicated, as indicated above.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/4
------------------------------------------------------------------------
On 2009-10-16T20:35:15+00:00 Standard8 wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/5
------------------------------------------------------------------------
On 2009-11-09T03:14:22+00:00 Kshriram18 wrote:
Now, how do we include that script into the binary file that modifies
that deals with the /tmp directory?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/6
------------------------------------------------------------------------
On 2009-11-09T03:16:51+00:00 Kshriram18 wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/7
------------------------------------------------------------------------
On 2009-11-09T03:17:17+00:00 Kshriram18 wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/8
------------------------------------------------------------------------
On 2009-11-09T19:56:32+00:00 Mkmelin+mozilla wrote:
Should be around here http://mxr.mozilla.org/comm-
central/source/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/9
------------------------------------------------------------------------
On 2009-11-15T21:46:01+00:00 Gosc-tb-project wrote:
Created attachment 412493
Patch for Bug 377630.
This patch saves attachments in /tmp/thunderbird-$USER/ by default
instead of /tmp.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/10
------------------------------------------------------------------------
On 2009-11-16T16:03:11+00:00 Benjamin Smedberg (Mozilla) [:bs] wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/11
------------------------------------------------------------------------
On 2009-11-22T18:03:33+00:00 Gosc-tb-project wrote:
Created attachment 413939
Patch.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/12
------------------------------------------------------------------------
On 2009-11-23T22:13:19+00:00 Benjamin Smedberg (Mozilla) [:bs] wrote:
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).
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/13
------------------------------------------------------------------------
On 2010-01-21T09:20:46+00:00 Vipul wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/14
------------------------------------------------------------------------
On 2010-01-30T10:48:45+00:00 Vipul wrote:
Created attachment 424393
patch creates the folder with "mozilla-$USER" in /tmp to store the temporary files for the user
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/15
------------------------------------------------------------------------
On 2010-02-01T15:07:42+00:00 mic1234 wrote:
Created attachment 424590
Patch
This patch is basically the same as posted above , just that it fixes
the '0755' folder permission to '0700'.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/16
------------------------------------------------------------------------
On 2010-02-01T17:07:40+00:00 Ludovic-mozilla wrote:
(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 :-)
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/17
------------------------------------------------------------------------
On 2010-02-03T11:28:33+00:00 mic1234 wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/18
------------------------------------------------------------------------
On 2010-02-05T04:25:16+00:00 Vipul wrote:
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/19
------------------------------------------------------------------------
On 2010-02-13T21:00:58+00:00 Vipul wrote:
Created attachment 426850
It fixes the issue of already existing directory.please comment
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/20
------------------------------------------------------------------------
On 2010-02-22T15:15:43+00:00 Vipul wrote:
Created attachment 428218
It fixes the issue of already existing directory.please comment
Final patch with Test Case
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/21
------------------------------------------------------------------------
On 2010-02-22T15:58:43+00:00 Om-brahmana wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/22
------------------------------------------------------------------------
On 2010-02-23T18:50:57+00:00 Vipul wrote:
Created attachment 428479
Patch;Please Comment
improvement :variable name,mnemonics
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/23
------------------------------------------------------------------------
On 2010-03-09T07:51:09+00:00 Timeless-bemail wrote:
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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/24
------------------------------------------------------------------------
On 2010-03-11T18:51:02+00:00 mic1234 wrote:
>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 .
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/25
------------------------------------------------------------------------
On 2010-03-15T16:21:53+00:00 mic1234 wrote:
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..
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/26
------------------------------------------------------------------------
On 2010-03-30T19:58:00+00:00 Dougt-a wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/27
------------------------------------------------------------------------
On 2010-04-24T08:40:38+00:00 Vipul wrote:
Created attachment 441256
Patch with new approach
temp files have name cryptic names
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/28
------------------------------------------------------------------------
On 2010-04-24T22:23:57+00:00 Dougt-a wrote:
Comment on attachment 441256
Patch with new approach
this is not correct. see my last comment. please make a tbird specific
change.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/29
------------------------------------------------------------------------
On 2010-05-01T04:25:36+00:00 Bzbarsky wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/30
------------------------------------------------------------------------
On 2010-05-02T03:16:38+00:00 Vipul wrote:
Created attachment 442962
Patch with directory fix
patch with test case.
create user restricted directory to store the temporary files.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/31
------------------------------------------------------------------------
On 2010-05-05T08:23:58+00:00 Vipul wrote:
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 .
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/32
------------------------------------------------------------------------
On 2010-05-17T18:39:29+00:00 Bzbarsky wrote:
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)?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/33
------------------------------------------------------------------------
On 2010-05-18T15:24:21+00:00 Vipul wrote:
Created attachment 445968
PATCH
patch fixes the issue by creating user specific directory with 700 permission
when download directory is requested.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/34
------------------------------------------------------------------------
On 2010-09-22T07:08:04+00:00 Standard8 wrote:
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).
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/35
------------------------------------------------------------------------
On 2010-10-16T05:23:32+00:00 Bzbarsky wrote:
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!
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/36
------------------------------------------------------------------------
On 2013-03-24T14:27:42+00:00 Kshriram18 wrote:
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);
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/37
------------------------------------------------------------------------
On 2013-03-24T14:48:14+00:00 Kshriram18 wrote:
Created attachment 728734
Patch for filename disclosure in /tmp
This patch includes test file in addition to content in last patch.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/38
------------------------------------------------------------------------
On 2013-03-28T14:25:24+00:00 Ludovic-mozilla wrote:
(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 ?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/39
------------------------------------------------------------------------
On 2013-03-28T14:43:15+00:00 Kshriram18 wrote:
(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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/40
------------------------------------------------------------------------
On 2013-04-02T21:12:36+00:00 Enrico-tagliavini wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1401454/comments/41
** Changed in: thunderbird
Status: Unknown => In Progress
** Changed in: thunderbird
Importance: Unknown => Medium
--
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla.
https://bugs.launchpad.net/bugs/1401454
Title:
Thunderbird writes attachments to /tmp readable to everyone
To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/1401454/+subscriptions
More information about the Ubuntu-mozillateam-bugs
mailing list