Closed Bug 377630 Opened 17 years ago Closed 9 years ago

Filename disclosure in /tmp - e.g. when saving attachments

Categories

(Core :: Security, defect)

x86
Linux
defect
Not set
normal

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
yes, bug as mention is there in fedora
Assignee: mscott → dveditz
Component: General → Security
QA Contact: general → thunderbird
Keywords: privacy
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.

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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-thunderbird3+
Assignee: dveditz → nobody
Confirming that this is still present in TB 3.0b4pre

Can we please fix this? It's not that complicated, as indicated above.
Flags: blocking-thunderbird3?
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
Now, how do we include that script into the binary file that modifies that deals with the /tmp directory?
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.
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.
Attached patch Patch for Bug 377630. (obsolete) — Splinter Review
This patch saves attachments in /tmp/thunderbird-$USER/ by default instead of /tmp.
Attachment #412493 - Flags: review?(benjamin)
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-
Attached patch Patch. (obsolete) — Splinter Review
Attachment #412493 - Attachment is obsolete: true
Attachment #413939 - Flags: review?(benjamin)
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-
Assignee: nobody → may.gup
Assignee: may.gup → vipulgolchha
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
Attached patch Patch (obsolete) — Splinter Review
This patch is basically the same as posted above , just that it fixes the '0755' folder permission to '0700'.
Attachment #424590 - Flags: review-
(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 :-)
Attachment #424590 - Flags: review-
Attachment #424590 - Flags: review?(benjamin)
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.
Attachment #424590 - Flags: review?(benjamin)
Attachment #424590 - Flags: review?(benjamin)
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
Attachment #424393 - Attachment is obsolete: true
Attachment #426850 - Flags: review+
Attachment #424393 - Flags: review?(benjamin)
Attachment #426850 - Flags: review+ → review?
Attachment #426850 - Flags: review? → review?(benjamin)
Final patch with Test Case
Attachment #426850 - Attachment is obsolete: true
Attachment #428218 - Flags: review?(benjamin)
Attachment #426850 - Flags: review?(benjamin)
(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.
Attachment #424590 - Attachment is obsolete: true
Attachment #424590 - Flags: review?(benjamin)
Attached patch Patch;Please Comment (obsolete) — Splinter Review
improvement :variable name,mnemonics
Attachment #428218 - Attachment is obsolete: true
Attachment #428479 - Flags: review?(benjamin)
Attachment #428218 - Flags: review?(benjamin)
Attachment #413939 - Attachment is obsolete: true
Attachment #428479 - Flags: review?(benjamin) → review-
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?
>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 .
Attached patch Work in Progress (obsolete) — Splinter Review
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
Attachment #432570 - Flags: review?(dougt)
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-
Attached patch Patch with new approach (obsolete) — Splinter Review
temp files have name cryptic names
Attachment #441256 - Flags: review?
Attachment #432570 - Attachment is obsolete: true
Attachment #441256 - Flags: review? → review?(dougt)
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-
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.
Attached patch Patch with directory fix (obsolete) — Splinter Review
patch with test case.
create user restricted directory to store the temporary files.
Attachment #441256 - Attachment is obsolete: true
Attachment #442962 - Flags: review?(bzbarsky)
Attached patch Patch (obsolete) — Splinter Review
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 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-
Attached patch PATCH (obsolete) — Splinter Review
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)
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 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-
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
This patch includes test file in addition to content in last patch.
Attachment #728732 - Attachment is obsolete: true
(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 ?
(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
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.
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.
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?
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.
vipul, which is assigned to this bug, was last active 2010, so please remove him from this bug and change the status to NEW.
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
Attached patch 37763.patch (obsolete) — Splinter Review
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
Attachment #8654113 - Flags: review?(bzbarsky)
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-
Attached patch 37763.patch (obsolete) — Splinter Review
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)
I forget!

I built and run xpcshell-test on Try Server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f3049d28eff
Hmm.  What about mac?

What doesn't work on Windows?  Are the permissions ignored, or does the directory creation fail?
Flags: needinfo?(aidin)
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 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+
Attached patch 37763.patchSplinter Review
Thank you for the review (:
Attachment #8654113 - Attachment is obsolete: true
Attachment #8657450 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b046fd76907f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1233434
Depends on: 1336730
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: