271062 - /tmp file permissions of nscpmsg.txt (world/group readable)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Alexander Sack]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041111 Firefox/1.0 (Debian package 1.0-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041111 Firefox/1.0 (Debian package 1.0-2)

when performing a copy with high latency, one can observe that there is a file
created called /tmp/nscpmsg.txt that is world readable. This should be only
readable by the user itself (IMHO).

For detailed infos take a look at the debian bug report provided by the url!

Reproducible: Always
Steps to Reproduce:

Comment 1

[Alexander Sack]

Attachment: here the patch that is verified to fix this for debian.

dunno if this is the way to go, but it has been verified by the initial bug
reporter, so I guess it really works!

Comment 2

[David :Bienvenu]

I'd rather do this in the underlying file spec code...maybe change
nsFileSpecImpl::OpenStreamForWriting to call 

NS_NewTypicalOutputFileStream(
    nsISupports** aResult,
    const nsFileSpec& inFile
    PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE),
    0700 (octal)

and restore the nspr mode and access rights to NS_NewTypicalOutputFileStream...

we might have to add access rights to OpenStreamForWriting. Since mailnews is
the only remaining client of this code, that's not too bad. I'm not sure why the
code was changed to use 0666 instead of 0700 in general.

Comment 3

[Alexander Sack]

thought about changing filespec code first too, but didn't suggested it, because
it is a frozen interface ?is it really?. 

So I thought changing the interface of OpenStreamForWriting or adding a second
method that allows specifying the permissions was not a choice! 

So you want me to add permission flags to the OpenStreamForWriting method? Or go
the way to make files produced by OpenStreamForWriting by default 0700?

Comment 4

[David :Bienvenu]

the interface is deprecated, but not frozen, afaik.

I'd add permission flags to the OpenStreamForWriting method. Changing the
default access mode to 0700 is a bit scary though I'd consider it - and why 0700
instead of 0600?

Comment 5

[Alexander Sack]

> the interface is deprecated, but not frozen, afaik.

so maybe it would even be a better idea to start right today moving away from
that interface? Since we will touch the code anyway, we could start switching to
a non-deprecated pattern. What is the preferred file mechnism to do this?

 
> I'd add permission flags to the OpenStreamForWriting method. Changing the
> default access mode to 0700 is a bit scary though I'd consider it - and why 0700
> instead of 0600?

0700 is of course bullshit.

I will go on and add the permissions flags if you don't want the migration to
the new file mechanism to start now. Just let me know!

Comment 6

[David :Bienvenu]

if you're talking about the trunk as opposed to the 1.0 branch, then it would be
a better idea to move away from that interface. But for the branch, that's too
risky.

There is a third way - avoid OpenStreamForWriting and call NS_NewIOFileStream
directly and then get an nsIOutputStream from that:

    mPath->GetFileSpec(&fileSpec);
    rv = NS_NewIOFileStream(getter_AddRefs(supports), fileSpec, PR_WRONLY |
PR_CREATE_FILE, 00600);
    NS_ENSURE_SUCCESS(rv, rv);
    supports->QueryInterface(NS_GET_IID(nsIOutputStream), (void **) outputStream);

put the output stream in the copy state and call write directly on it...that
will ease the transition to using the non-deprecated interfaces, since we'll
need to do end up with an outputstream ultimately anyway.

Or better you can do this:

    nsCOMPtr<nsILocalFile> localFile;
    nsCOMPtr<nsIOutputStream> outputStream;
    NS_FileSpecToIFile(mFileSpec, getter_AddRefs(localFile));
    rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), localFile,
-1, 00600);

and put the output stream in the copy state, as above.

that's all clear as mud, I'm sure :-(

Comment 7

[Alexander Sack]

(In reply to comment #6)

>
> that's all clear as mud, I'm sure :-(
> 

OK, thx, I think I think I understood! Will test it this weekend!

Comment 8

[Alexander Sack]

Attachment: the new patch for the nsImapMailFolder.cpp/.h

tried my best to implement your enhancements. I tried using imap, copying a bit
around... and it appears to work. Offline is also tested and works too for me!

Comment 9

[Alexander Sack]

Attachment: the new patch for the nsImapMailFolder.cpp/.h and offlineSync

tried my best to implement your enhancements. I tried using imap, copying a bit
around... and it appears to work. Offline is also tested and works too for me!

Comment 10

[Alexander Sack]

Comment on attachment 167336 [details] [diff] [review]
the new patch for the nsImapMailFolder.cpp/.h and offlineSync

please review it. If you want further improvements, let me know! This patch is
against the branch (e.g. thunderbird 0.9 / not HEAD). If it does not apply
cleanly on the BRANCHHEAD, let me know!

Comment 11

[David :Bienvenu]

Comment on attachment 167336 [details] [diff] [review]
the new patch for the nsImapMailFolder.cpp/.h and offlineSync

this looks in general OK - a few nits - fix the comment here:
nsCOMPtr<nsIOutputStream> m_outputStream; // outputStram to tmp file for
copying

to be outputStream.

the prevailing comment style (and my strong preference :-) ) is 

if (a)
{
  foo();
}

not K&R, so could you fix that?

other than that, it looks good. I don't think this is going to make 1.0, but we
can get it into the trunk and on the train for 1.1

Comment 12

[Alexander Sack]

Attachment: new version with improved formatting

Comment 13

[Alexander Sack]

Comment on attachment 167478 [details] [diff] [review]
new version with improved formatting

please review the reformatted patch.

Comment 14

[Alexander Sack]

David, what are the plans for this bug now? Maybe this one can now be tackled
for real in HEAD?

Comment 15

[David :Bienvenu]

sorry, yes, dropped the ball on this. I'll try to pick it up, and see if the
patch still applies on the trunk.

Comment 16

[Alexander Sack]

Once you told:

>if you're talking about the trunk as opposed to the 1.0 branch, then it would be
>a better idea to move away from that interface. But for the branch, that's too
>risky.

Let me know if we should consider to implement the RIGHT solution. Maybe outline
it to me and I will do my best to implement it :). 

Anyway, if you have problems with the patch, let me know. I can adjust it.

Comment 17

[Reed Loden [:reed]]

asac, this patch is three years old... could you please make sure it will still work on trunk? Thanks.

David, could you review this like you mentioned you would in 2005? :)

Comment 18

[David :Bienvenu]

I don't think this patch would apply against the trunk because of the work to get rid of nsIFileSpec. Sorry to have dropped the ball on this. I looked through the code - the code in nsImapOfflineSync does the right thing now (permissions of 0600) and it looks like nsImapMailFolder does as well. There's still an 0700 instead of 0600, but that's not world/group readable.

Comment 19

[David :Bienvenu]

I'm going to mark this fixed, by the nsIFileSpec removal patch, bug 33451.

Comment 20

[David :Bienvenu]

Comment on attachment 166660 [details] [diff] [review]
here the patch that is verified to fix this for debian.

clearing request since the bug is fixed, I believe.

Comment 21

[David :Bienvenu]

Comment on attachment 167478 [details] [diff] [review]
new version with improved formatting

missed this one - also obsolete, and the underlying bug is fixed.

Comment 22

[David :Bienvenu]

Comment on attachment 167336 [details] [diff] [review]
the new patch for the nsImapMailFolder.cpp/.h and offlineSync

clearing obsolete request