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 Desktop Packages, which is subscribed to thunderbird in Ubuntu. https://bugs.launchpad.net/bugs/1401454 Title: Thunderbird writes attachments to /tmp readable to everyone Status in Mozilla Thunderbird Mail and News: In Progress Status in thunderbird package in Ubuntu: Confirmed Bug description: 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. To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/1401454/+subscriptions -- Mailing list: https://launchpad.net/~desktop-packages Post to : desktop-packages@lists.launchpad.net Unsubscribe : https://launchpad.net/~desktop-packages More help : https://help.launchpad.net/ListHelp
