User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.4) Gecko/20091106 SeaMonkey/2.0 Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.4) Gecko/20091106 SeaMonkey/2.0 Email attachments saved (or detached) using the context menu in the message view are saved with restrictive permissions (0600). Instead they should be 0666 & ~umask : that's the way they were until 2.0, and also the way they are if you "open" the attachment and then save it. Reproducible: Always Steps to Reproduce: 1. open MailNews, view an email with an attachment (I tested pdf, odt and doc) 2. right-click on the attachment and choose "Save as..." or "Detach..." Actual Results: The file permissions of the saved/detached attachment are 600. Expected Results: The file permissions should depend on the umask but potentially be more permissive than 600. For example my umask is 022, so I would expect 644. If you select "Open" after right-clicking on the attachment, a window pops up asking what app to use and also offering to save the file. If you do this, the file is saved with the correct permissions. Note that this seems similar to bug 124307 (https://bugzilla.mozilla.org/show_bug.cgi?id=124307) but it's different: the problem described in that bug is no longer present for me (ie saving a file in the browser). I'm also pretty sure that this bug didn't exist in 1.1.18 (and several earlier versions).

Pages from Browser are saved with 611 and attachments from MailNews as described even under SM2.1pre

SM is among the most important tools for managing my web project, WIMA http://icking-music-archive.org/ (an archive of free-of-charge classical music scores). WIMA contributors unable to upload files through WIMA's ftp service are allowed to send me their files as email attachments. Since I upgraded to SM 2* I need to 'chmod' the permission bits of these files. Unfortunately it often happens that I forget to do that - and get complaints from WIMA users.

I've also noticed this and was about to file a bug. Since we've reverted back to Thunderbird 2 for various reasons this does not affect us as a company anymore. However, me and some others have decided to stick with Thunderbird 3 and it's really a major annoyance.

On a similar note, when I tell Seamonkey to open the attachment directly, it saves the attachment to /tmp with 400 permissions, which means it's read-only. Is there any way I can change that? Open Office, for instance, won't let me make any changes to a file that's opened as read-only, which means I need to 1) Save the attachment 2 [review]) Start OpenOffice 3) Manually load the file from /tmp

I have found out that the code paths invoked by - Right-clicking on the attachment and "Save As", and - Double clicking by the Left mouse button on the attachment are different and thus may use totally different permission bits. See Bug 549719 Unify/Merge two code paths for saving an attachment in a message (different save directory in "double click/save file" vs "context menu/save as") Can people who have reported on this problem check to see if the difference is noticed on their OS platforms in the left-clicking vs right-clicking cases? I once suspected that failure to use umask may be to blame, but I now feel that the original permission setting is very different in the two code paths after all. The right-click + "save as" path seems to use very restrictive permission of 0600(!). If you are interested, please see two additional bugs: Bug 569807 - Right-clicking on an attachment and choose "Save AS" in TB3 and try to save to a write-protected directory shows error dialog TWICE Bug 569807 - Right-clicking on an attachment and choose "Save AS" in TB3 and try to save to a write-protected directory shows error dialog TWICE for some background information. If other people's reporting match with my observation, then I think we can home in the cause of the problem. TIA

(In reply to comment #6) > I have found out that the code paths invoked by > > - Right-clicking on the attachment and "Save As", and > - Double clicking by the Left mouse button on the attachment > > are different and thus may use totally different permission bits. See Bug > 549719 > Unify/Merge two code paths for saving an attachment in a message (different > save directory in "double click/save file" vs "context menu/save as") > > Can people who have reported on this problem check to see if the > difference is noticed on their OS platforms in the left-clicking vs > right-clicking cases? It does make difference in my OS environment, Ubuntu 10.04. The permissions for a file saved by double clicking are -rw-r--r-- > > I once suspected that failure to use umask may be to blame, but I now feel that > the original permission setting is very different in the two code paths after > all. The right-click + "save as" path seems to use very restrictive permission > of 0600(!). > > If you are interested, please see two additional bugs: Bug 569807 - > Right-clicking on an attachment and choose "Save AS" in TB3 and try to save to > a write-protected directory shows error dialog TWICE > > Bug 569807 - Right-clicking on an attachment and choose "Save AS" in TB3 and > try to save to a write-protected directory shows error dialog TWICE > for some background information. > > If other people's reporting match with my observation, then > I think we can home in the cause of the problem. > > TIA

Thank you for reporting. From the use of "06*" in the code, it seems that 00600 is hard-coded in more places in nsMessenger.cpp that handles attachment saving in v3 series. I suspect that old v2 hides the permission setting in separate utility function(s) (where umask is handled nicely, probably with fopen() that handles umask as we desire. But this is my pure guess.) So we probably wanted to set the permission 00600 used in nsMessagenger.cpp (see the following) to 00666 & ~usermask (after obtaining usermask = umask(022) and reverted it back, say.) running "grep -n 6 nsMessenger.cpp" and removed unwanted lines. old nsMessenger.cpp (v2) 3053: rv = NS_NewLocalFileOutputStream(getter_AddRefs(fileOutputStream), msgFile, -1, 00600); new nsMessenger.cpp (3.2a1pre comm-central) 740: rv = attachmentDestination->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); 902: rv = attachmentDestination->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); 1148: rv = tmpFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); 1842: MsgNewBufferedFileOutputStream(getter_AddRefs(m_outputStream), m_file, -1, 00600); 1926: rv = localFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); 2876: rv = mMsgFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); 2879: rv = MsgNewBufferedFileOutputStream(getter_AddRefs(mMsgFileStream), mMsgFile, -1, 00600); Any thoughts from someone in the know?

OK, I am uploading a patch to nsMessenger.cpp to fix the permission problem. That is, to give it a somewhat relaxed permission setting. After the patch, TB3.2a1pre restored the older behavior. It seems that umask setting is not ignored after all (I re-read open(2) manual and found that umask value is observed by open() and creat().) So basically the passing of 00600 value in some functions in nsMessenger.cpp was the culprit. I changed them to a symoblic macro ATTACHMENT_PERMISSION in all the places, and set it 00660. (Well, 00666 seemed a little too lax IMHO). But then again, download manager seems to use 0666 after all. If 00666 is deemed too lenient in terms of security, someone ought to raise an alert here. After the patch, the permission of saved attachment became as follows. My usual umask is 0022. So saving an attachment with the new code sets the permission to 00640. When I set umask to zero (0000), then the new code sets the permission to 00660. I show the permission set by the download manager invoked by LEFT-click, also. Obviously it uses the permission of 00666. -rw-r--r-- 1 ishikawa ishikawa 1294572 2010-06-04 01:29 save-with-left-click -rw-rw-rw- 1 ishikawa ishikawa 1294572 2010-06-04 01:33 save-with-left-click-with-0-umask -rw-r----- 1 ishikawa ishikawa 1294572 2010-06-04 01:29 save-with-right-click-patched -rw-rw---- 1 ishikawa ishikawa 1294572 2010-06-04 01:33 save-with-right-click-patched-with-0-umask I will appreciate people's comment about the particular choice of value 00660. Maybe after a week of hearing the pros and cons of 00660 vs 00666, etc., I will upload the final patch for review. But for now, for those who want to experiment, I am uploading the patch to try the above.

A proposal patch to change the permission of Right-context menu Save As to be more like the permission used by Download Manager invoked by Left-click. Despite what I said in my previous comment, I set review flag "?" to get the attention. Whose e-mail address should I put in here?

Comment on attachment 449036 [details] A proposal patch to change the permission of Right-context menu Save As Put bienvenu@nventure.com in the requestee field since he seems to have reviewed the latest changes to this file. TIA

thx for the patch; it looks reasonable. I would take out the comments for each use of ATTACHMENT_PERMISSION, and remove the FIXME/TODO part of the comment before ATTACHMENT_PERMISSION is defined.

Comment on attachment 449036 [details] A proposal patch to change the permission of Right-context menu Save As actually, why are you even changing these lines? + // TODO/FIXME: Is permission other than 00600 OK here for security reasons? + rv = tmpFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, ATTACHMENT_PERMISSION); they don't have to do with the original bug, and they're just temp files. I think leaving them at 0600 is better.

(In reply to comment #13) > (From update of attachment 449036 [details]) > actually, why are you even changing these lines? > ... > they don't have to do with the original bug, and they're just temp files. I > think leaving them at 0600 is better. Just as I thought. I could not follow the logic flow very well. (I am still learning the idioms of TB3 internal, for example, "Listener.") I figured that this could be a temp file and so may be 0600 was OK, but was not so sure. (It may be that a temp file is initially created for saving and simply renamed at the end of a save operation under certain situations, for example.) I will incorporate your comment and fix the point above and resubmit the patch. TIA

Incorporate the suggestion. In one place, I added a comment about using 0600 is better. TIA

Comment on attachment 449205 [details] [diff] [review] Improved patch to fix the permission issue of Right-context Save As I don't think you want this bit to change - @@ -2868,20 +2871,20 @@ // create an output stream on a temporary file. This stream will save the modified // message data to a file which we will later use to replace the existing message. // The file is removed in the destructor. rv = GetSpecialDirectoryWithFileName(NS_OS_TEMP_DIR, "nsmail.tmp", getter_AddRefs(mMsgFile)); NS_ENSURE_SUCCESS(rv,rv); - rv = mMsgFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); + rv = mMsgFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, ATTACHMENT_PERMISSION); since this is also a temp file.

Comment on attachment 449205 [details] [diff] [review] Improved patch to fix the permission issue of Right-context Save As minusing, but the rest looks good, thx for the new patch.

(In reply to comment #16) > (From update of attachment 449205 [details] [diff] [review]) > I don't think you want this bit to change - > > @@ -2868,20 +2871,20 @@ > > // create an output stream on a temporary file. This stream will save the > modified > // message data to a file which we will later use to replace the existing > message. > // The file is removed in the destructor. > rv = GetSpecialDirectoryWithFileName(NS_OS_TEMP_DIR, "nsmail.tmp", > getter_AddRefs(mMsgFile)); > NS_ENSURE_SUCCESS(rv,rv); > > - rv = mMsgFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600); > + rv = mMsgFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, > ATTACHMENT_PERMISSION); > > since this is also a temp file. Thank you. I should have looked at the context leading to this mMsgFile processing. Is the use of ATTACHMENT_PERMISSION in the expression (just a couple of lines below) OK? > rv = MsgNewBufferedFileOutputStream(getter_AddRefs(mMsgFileStream), mMsgFile, -1, ATTACHMENT_PERMISSION); I suspect that the file and a stream opened for it can have different permissions. Hmm... Anyway, if this is the case, the new patch I am uploading in a few minutes should be it, hopefully.

Incorporated all the comments so far, hopefully.

Thanks a lot for tackling this! Concerning the value of ATTACHMENT_PERMISSION, I think download manager is doing the right thing: it should be 0666, not 0660. Let the user choose. Every distribution comes with a default umask that at least kills the world-write permission, so if a user's umask does not do that, it must be a conscious decision. Why should the app decide otherwise? At the very least I suggest you use 0664. Personnally I only need the group-readable bit, but I'm sure some users in some environments will require world-read permissions. One question: does this patch also correct the issue when detaching attachments? (assuming "detach" is offered in TB, I'm not sure because I use seamonkey, where the bug is present both for "save" and "detach", hence my initial bug report).

(In reply to comment #20) > Thanks a lot for tackling this! > > Concerning the value of ATTACHMENT_PERMISSION, I think download manager is > doing the right thing: it should be 0666, not 0660. > Let the user choose. Every distribution comes with a default umask that at > least kills the world-write permission, so if a user's umask does not do that, > it must be a conscious decision. Why should the app decide otherwise? > At the very least I suggest you use 0664. Personnally I only need the > group-readable bit, but I'm sure some users in some environments will require > world-read permissions. > Thank you for the comment. I was thinking of waiting to hear such input. > One question: does this patch also correct the issue when detaching > attachments? (assuming "detach" is offered in TB, I'm not sure because I use > seamonkey, where the bug is present both for "save" and "detach", hence my > initial bug report). I believe so. While I am not familiar with detach operation, I checked that the detached files have the same permissions as the attached files. Also, the detaching to a write-protected directory raises the warning. However, currently it raises the error TWICE in succession about which I filed an bugzilla. Bug 569807 Right-clicking on an attachment and choose "Save AS" in TB3 and try to save to a write-protected directory shows error dialog TWICE Now I realize the same bug occurs for detaching operation as well.

Comment on attachment 449278 [details] [diff] [review] Another improved patch to fix the permission issue of Right-context Save As thx for the patch!

(In reply to comment #20) > Thanks a lot for tackling this! > > Concerning the value of ATTACHMENT_PERMISSION, I think download manager is > doing the right thing: it should be 0666, not 0660. > Let the user choose. ... > At the very least I suggest you use 0664. Personnally I only need the > group-readable bit, but I'm sure some users in some environments will require > world-read permissions. > Would we want to relax ATTACHMENT_PERMISSION to 0664 at the least? What do other people think? TIA

(In reply to comment #23) > Would we want to relax ATTACHMENT_PERMISSION to 0664 at the least? > What do other people think? I'm OK with that. It's easy to change now.

(In reply to comment #24) > (In reply to comment #23) > > Would we want to relax ATTACHMENT_PERMISSION to 0664 at the least? > > What do other people think? > > I'm OK with that. It's easy to change now. thanks!

I have modified the already approved patch so that it has a slightly more relaxed permission.

Now that the review is granted (review+), do I have to do something to get this patch merged into the repository? (It is always hard to try to figure out what to do for an occasional patch submitter. Some bugs seem to be handled automatically all the way to the merging while some needs me to set keywords to "check-in needed", etc. TIA

Comment on attachment 452256 [details] [diff] [review] New patch to fix the permission issue of Right-context Save As (relaxed permission slightly) First you need sr (I've requested that for you). Then, once you have sr, you need to add the checkin-neeeded keyword, so that someone will check it in for you)

(In reply to comment #28) > (From update of attachment 452256 [details] [diff] [review]) > First you need sr (I've requested that for you). Then, once you have sr, you > need to add the checkin-neeeded keyword, so that someone will check it in for > you) Thank you for taking care of the sr thing. Now that I see sr+ is given, I look at the web page of bugzilla, but I see no place where keyword can be changed. Maybe such change of keyword is possible only for the original reporter? (In this case, Nicolas Thierry-Mieg ?) I do see a keyword change field in some bugzilla entries which turned out to be reported by me for the first time. TIA

The "editbugs" permission is needed. See Preferences - Permissions for your settings. User permissions are managed by Gerv. Approval by project management is needed.

Checked in: http://hg.mozilla.org/comm-central/rev/cc9c99448ea7

I would love to see this patch included in seamonkey 2.0.next (and therefore also TB 3.0.next, if I understand correctly). I have adapted it slightly so that it applies cleanly on SM 2.0.5 source, and can confirm that the issue is fixed in this [SM 2.0.5 + adapted patch] build. Adapted patch is available [2]. I was told on a thread on mozilla.dev.apps.seamonkey [1] that I should set status-thunderbird3.0? on the bug, but I can't do that (no drop-down menu next to status-thunderbird3.0). Could someone with the knowledge and permissions please set the appropriate flag? Thanks [1] http://groups.google.com/group/mozilla.dev.apps.seamonkey/browse_thread/thread/2cb3d5a5a0d96299# [2] http://membres-timc.imag.fr/Nicolas.Thierry-Mieg/seamonkey_wrongSavedPerms_bz533976.diff

Comment on attachment 452256 [details] [diff] [review] New patch to fix the permission issue of Right-context Save As (relaxed permission slightly) 3.0.6 is a minimal release for us, so we'll consider this for 3.0.7.

Comment on attachment 452256 [details] [diff] [review] New patch to fix the permission issue of Right-context Save As (relaxed permission slightly) This is fine for the 1.9.2 branch, but needs a clean patch for the 1.9.1 branch.

Comment on attachment 452256 [details] [diff] [review] New patch to fix the permission issue of Right-context Save As (relaxed permission slightly) (needs new patch)

(In reply to comment #34) > Comment on attachment 452256 [details] [diff] [review] > New patch to fix the permission issue of Right-context Save As (relaxed > permission slightly) > > This is fine for the 1.9.2 branch, but needs a clean patch for the 1.9.1 > branch. I believe the patch I posted a link to in Comment 32 is "a clean patch for the 1.9.1 branch", though I'm not 100% on that because the version numbers and comm-central repo are confusing to me. I'm attaching that patch to this bz, hopefully I'm not doing things totally wrong.

Checked into 1.9.2 branch: http://hg.mozilla.org/releases/comm-1.9.2/rev/68f190a2818d

Comment on attachment 467834 [details] [diff] [review] Chiaki's patch, adapted for the 1.9.1 branch (I hope) Neil, can you check this over for the 1.9.1 branch?

(In reply to comment #39) > Comment on attachment 467834 [details] [diff] [review] > Chiaki's patch, adapted for the 1.9.1 branch (I hope) > > Neil, can you check this over for the 1.9.1 branch? Hi, sorry I could not follow up myself. I have been on a business trip outside my home country and could not follow the thread easily. I would love to see the patch back ported to the earlier releases. I now realize that comm-central, which I tinker with mostly, is only for the trunk, and is the basis for the future version. So I have started tinkering with porting the patches to the current working revisions only lately. I vote for merging the patch to the current versions. TIA.

Comment on attachment 467834 [details] [diff] [review] Chiaki's patch, adapted for the 1.9.1 branch (I hope) This looks right; the places that haven't been patched are in code that doesn't exist on the 1.9.1 branch. (I don't actually have a 1.9.1 Linux build to test.)

(In reply to comment #41) > Comment on attachment 467834 [details] [diff] [review] > Chiaki's patch, adapted for the 1.9.1 branch (I hope) > > This looks right; the places that haven't been patched are in code that doesn't > exist on the 1.9.1 branch. (I don't actually have a 1.9.1 Linux build to test.) Hi, I confirmed that the patched source compiles and works in comm-1.9.1 source tree under Linux. (The version shown in Help->About Thunderbird is 3.0.8pre.) TIA

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100825 Thunderbird/3.1.3