Closed
Bug 377630
Opened 17 years ago
Closed 9 years ago
Filename disclosure in /tmp - e.g. when saving attachments
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: pb, Assigned: aidin)
References
(Depends on 1 open bug)
Details
(Keywords: privacy, student-project)
Attachments
(1 file, 16 obsolete files)
4.36 KB,
patch
|
Details | Diff | Splinter Review |
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
Updated•17 years ago
|
Assignee: mscott → dveditz
Component: General → Security
QA Contact: general → thunderbird
Reporter | ||
Comment 2•16 years ago
|
||
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.
Comment 3•15 years ago
|
||
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...
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•15 years ago
|
Flags: wanted-thunderbird3+
Updated•15 years ago
|
Assignee: dveditz → nobody
Comment 4•15 years ago
|
||
Confirming that this is still present in TB 3.0b4pre Can we please fix this? It's not that complicated, as indicated above.
Updated•15 years ago
|
Flags: blocking-thunderbird3?
Comment 5•15 years ago
|
||
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.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
Keywords: student-project
Comment 6•15 years ago
|
||
Now, how do we include that script into the binary file that modifies that deals with the /tmp directory?
Comment 7•15 years ago
|
||
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.
Comment 8•15 years ago
|
||
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.
Comment 9•15 years ago
|
||
Should be around here http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp
Comment 10•15 years ago
|
||
This patch saves attachments in /tmp/thunderbird-$USER/ by default instead of /tmp.
Attachment #412493 -
Flags: review?(benjamin)
Comment 11•15 years ago
|
||
Comment on attachment 412493 [details] [diff] [review] 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.
Attachment #412493 -
Flags: review?(benjamin) → review-
Comment 12•15 years ago
|
||
Attachment #412493 -
Attachment is obsolete: true
Attachment #413939 -
Flags: review?(benjamin)
Comment 13•15 years ago
|
||
Comment on attachment 413939 [details] [diff] [review] Patch. Still no tests, and no answer to the question about actually creating the directory (and what to do if it already exists).
Attachment #413939 -
Flags: review?(benjamin) → review-
Updated•14 years ago
|
Assignee: nobody → may.gup
Updated•14 years ago
|
Assignee: may.gup → vipulgolchha
Comment 14•14 years ago
|
||
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.
Status: NEW → ASSIGNED
Comment 15•14 years ago
|
||
Attachment #424393 -
Flags: review?(benjamin)
Comment 16•14 years ago
|
||
This patch is basically the same as posted above , just that it fixes the '0755' folder permission to '0700'.
Attachment #424590 -
Flags: review-
Comment 17•14 years ago
|
||
(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 :-)
Updated•14 years ago
|
Attachment #424590 -
Flags: review-
Updated•14 years ago
|
Attachment #424590 -
Flags: review?(benjamin)
Comment 18•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #424590 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #424590 -
Flags: review?(benjamin)
Comment 19•14 years ago
|
||
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
Comment 20•14 years ago
|
||
Attachment #424393 -
Attachment is obsolete: true
Attachment #426850 -
Flags: review+
Attachment #424393 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #426850 -
Flags: review+ → review?
Updated•14 years ago
|
Attachment #426850 -
Flags: review? → review?(benjamin)
Comment 21•14 years ago
|
||
Final patch with Test Case
Attachment #426850 -
Attachment is obsolete: true
Attachment #428218 -
Flags: review?(benjamin)
Attachment #426850 -
Flags: review?(benjamin)
Comment 22•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #424590 -
Attachment is obsolete: true
Attachment #424590 -
Flags: review?(benjamin)
Comment 23•14 years ago
|
||
improvement :variable name,mnemonics
Attachment #428218 -
Attachment is obsolete: true
Attachment #428479 -
Flags: review?(benjamin)
Attachment #428218 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #413939 -
Attachment is obsolete: true
Attachment #428479 -
Flags: review?(benjamin) → review-
Comment 24•14 years ago
|
||
Comment on attachment 428479 [details] [diff] [review] 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?
Comment 25•14 years ago
|
||
>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 .
Comment 26•14 years ago
|
||
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..
Attachment #428479 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #432570 -
Flags: review?(dougt)
Comment 27•14 years ago
|
||
Comment on attachment 432570 [details] [diff] [review] 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.
Attachment #432570 -
Flags: review?(dougt) → review-
Updated•14 years ago
|
Attachment #432570 -
Attachment is obsolete: true
Attachment #441256 -
Flags: review? → review?(dougt)
Comment 29•14 years ago
|
||
Comment on attachment 441256 [details] [diff] [review] Patch with new approach this is not correct. see my last comment. please make a tbird specific change.
Attachment #441256 -
Flags: review?(dougt) → review-
Comment 30•14 years ago
|
||
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.
Comment 31•14 years ago
|
||
patch with test case. create user restricted directory to store the temporary files.
Attachment #441256 -
Attachment is obsolete: true
Attachment #442962 -
Flags: review?(bzbarsky)
Comment 32•14 years ago
|
||
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 .
Attachment #442962 -
Attachment is obsolete: true
Attachment #443575 -
Flags: review?(bzbarsky)
Attachment #442962 -
Flags: review?(bzbarsky)
Comment 33•14 years ago
|
||
Comment on attachment 443575 [details] [diff] [review] 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)?
Attachment #443575 -
Flags: review?(bzbarsky) → review-
Comment 34•14 years ago
|
||
patch fixes the issue by creating user specific directory with 700 permission when download directory is requested.
Attachment #443575 -
Attachment is obsolete: true
Attachment #445968 -
Flags: review?(bzbarsky)
Comment 35•14 years ago
|
||
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).
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Product: Thunderbird → Core
QA Contact: thunderbird → toolkit
Summary: Attachment filename disclosure in /tmp → Filename disclosure in /tmp - e.g. when saving attachments
Comment 36•14 years ago
|
||
Comment on attachment 445968 [details] [diff] [review] 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!
Attachment #445968 -
Flags: review?(bzbarsky) → review-
Comment 37•11 years ago
|
||
WIP Based on last patch. At the moment, test fails @ assertion on line 20: do_check_true(isExists && isWritable);
Attachment #445968 -
Attachment is obsolete: true
Comment 38•11 years ago
|
||
This patch includes test file in addition to content in last patch.
Attachment #728732 -
Attachment is obsolete: true
Comment 39•11 years ago
|
||
(In reply to Shriram (irc: Mavericks) from comment #38) > Created attachment 728734 [details] [diff] [review] > 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 ?
Comment 40•11 years ago
|
||
(In reply to Ludovic Hirlimann [:Usul] from comment #39) > (In reply to Shriram (irc: Mavericks) from comment #38) > > Created attachment 728734 [details] [diff] [review] > > 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
Comment 41•11 years ago
|
||
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.
See Also: → https://launchpad.net/bugs/1401454
Comment 42•9 years ago
|
||
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.
Comment 43•9 years ago
|
||
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?
Comment 44•9 years ago
|
||
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.
Comment 45•9 years ago
|
||
vipul, which is assigned to this bug, was last active 2010, so please remove him from this bug and change the status to NEW.
Comment 46•9 years ago
|
||
I don't think you should pay so much attention to the "assignee" field or the status. Both are often bogus.
Assignee: vipulgolchha → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 47•9 years ago
|
||
This patch is based on the previous work, but I cleaned up the code. I test it manually on a Gnu/Linux machine, and everything is fine. Just a note about the Unit Test: Previous patch was testing a wrong method. So I write my own test. The problem is, xpcshell have its own temporary directory, which is already have 0700 permission. So, the first if, "if (permissions != PR_IRWXU)", is always false, and test won't cover all the written code. Do you have any better idea for writing the Unit Test? Thanks (:
Assignee: nobody → aidin
Attachment #728734 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8654113 -
Flags: review?(bzbarsky)
Comment 48•9 years ago
|
||
Comment on attachment 8654113 [details] [diff] [review] 37763.patch >+ do_print(targetFile.parent.path); Is this needed as part of the test? Seems like extra noise for no good reason... >+ nsDependentString userDir; nsAutoString, please, since this is not in fact a dependent string. >+ bool isPathExists; Just pathExists? >+ while (PR_TRUE) { "true", please. PR_TRUE should not be used. >+ nsDependentString countedUserDir(userDir); nsAutoString. >+ if (!isPathExists) { Checking this and then doing the Create as a separate step is racy. This is why CreateUnique just tries to Create and sees whether it succeeded... In other words, you probably want to check for pathExists and if so for the correct permissions and if all that is true, use that path. Otherwise try to create and continue the loop if that fails with NS_ERROR_FILE_ALREADY_EXISTS. I'd like to see an updated version of this patch with that change made.
Attachment #8654113 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 49•9 years ago
|
||
Thanks for the review (: I fixed things you mentioned. I test my changes on a Windows machine, and found that it doesn't work! As far as I could found, creating a directory/file with restricted permissions only supports on Unix machines. So, I wrap the whole block in an #if statement.
Attachment #8657450 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 50•9 years ago
|
||
I forget! I built and run xpcshell-test on Try Server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f3049d28eff
Comment 51•9 years ago
|
||
Hmm. What about mac? What doesn't work on Windows? Are the permissions ignored, or does the directory creation fail?
Flags: needinfo?(aidin)
Assignee | ||
Comment 52•9 years ago
|
||
Mac have it's own #if block, with a different algorithm. I didn't examine that. (I don't know anything about Mac!) Yes, permissions ignored on Windows. I test it on Windows 7. But when I ran the unit test on Treeherder, it failed on other Windows too.
Flags: needinfo?(aidin)
Comment 53•9 years ago
|
||
Comment on attachment 8657450 [details] [diff] [review] 37763.patch It's worth adding a comment in the code about why the ifdef is there. >+ //Make sure that only the current user can read the filenames we end up creating Space after the "//" please. >+ userDir.AssignASCII("mozilla_"); AssignLiteral. >+ // If this path have the right permissions, use it. "has the right permissions" r=me with those nits picked. Thank you!
Attachment #8657450 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 54•9 years ago
|
||
Thank you for the review (:
Attachment #8654113 -
Attachment is obsolete: true
Attachment #8657450 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 55•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b046fd76907f
Keywords: checkin-needed
Comment 56•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b046fd76907f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•