Closed Bug 1385816 Opened 7 years ago Closed 7 years ago

nsFileStreamBase::DoOpen() should call directory creation with 0755 permission instead of 0644.

Categories

(Core :: Networking: File, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ishikawa, Assigned: ishikawa)

References

Details

Attachments

(1 file, 1 obsolete file)

|mkdir()| should not be called without X-bit (directory search
permission) unless the intention of creating such a directory is very
clear about it, say, for testing special directory as in a creation
for anonymous ftp upload, etc: others can write into it but not list
other file entries unless you know the exact pathname for other
uploaded files, for example.

I found the issue while investigating a problematic symptom in
Bug 1384461 - C-C TB: Unreliable test: mailnews/jsaccount/test/unit/test_jaMsgFolder.js (it repeats pass/fail/pass/fail if executed individually)

Please see especially the comment about |mkdir ()| called with 0644 in Bug 1384461 comment 1, 
https://bugzilla.mozilla.org/show_bug.cgi?id=1384461#c1

I found by running strace under linux that |mkdir| is called with permission 0644 and thus there is no X-bit set for its own creator (not to mention group and other users).
This is bad, I think since subsequently no one can list the content of the directory.
If the created directory is related to one's user profile, you can see there is a problem.

BTW, The test in question in bug 1384461, i.e., C-C TB's
mailnews/jsaccount/test/unit/test_jaMsgFolder.js needs to be fixed
since it calls directory creation routine without setting X-bit
explicitly. However, even after I fixed *THAT* (I will address THAT
fix in Bug 1384461), I still noticed |mkdir| being called with no
owner X-bit when some profile-related file(s) were created during that test.

The originating call with 0644 in |mkdir()| came from the function

   nsFileStreamBase::DoOpen()

The use of 0644 in this function seems to add to the problem I noticed in bug 1384461.
This number ought to be changed to 0755.

I will upload a patch.

The directory creation feature in |DoOpen()| has been introduced in
Bug 277367 - nsIFileOutputStream.init(file_in_adirectory_that_does_not_exist, MODE_WRONLY /* 0x2*/ | MODE_CREATE /* 0x8*/ | MODE_TRUNCATE /* 0x20*/, 0644, 0) should either create the directory or throw something more sensible than NS_ERROR_FILE_NOT_FOUND

So I will set the See also field to include bug 277367.
I am uploading a patch that takes care of the change of 0644 into 0755.

The patch also includes the detection of the call to |do_mkdir|,
(which is an interace to |mkdir| under UNIX(linux and Mac OSX(?))
without owner's X-bit inside #ifdef DEBUG/#endif.

I believe if we can check problematic calls to |mkdir| during linux
testing on mozilla test infrastructure, we should be covered rather well even under
Windows(???).

I am not sure about Windows operation, however.  If someone is very
concerned with a possible issue under Windows, a separate
investigation may be ncessary.  I say this because I found emulation
of 0755 and other directory permission bits was less than ideal a few
years ago under Cygwin.  Even if I ran |chmod -w a-directory|. I could
still |echo something > a-directory| without invoking an error.
Assignee: nobody → ishikawa
Attachment #8891939 - Flags: review?(bzbarsky)
Blocks: 1384461
(In reply to ISHIKAWA, Chiaki from comment #0)

> Please see especially the comment about |mkdir ()| called with 0644 in Bug
> 1384461 comment 1, 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1384461#c1

That should have read "mkdir() called with 0600" instead.
The call with 0644 was found AFTER I fixed the bug in the test itself, 
mailnews/jsaccount/test/unit/test_jaMsgFolder.js, and the test proceeded further.
(In reply to ISHIKAWA, Chiaki from comment #1)

> I am not sure about Windows operation, however.  If someone is very
> concerned with a possible issue under Windows, a separate
> investigation may be ncessary.  I say this because I found emulation
> of 0755 and other directory permission bits was less than ideal a few
> years ago under Cygwin.  Even if I ran |chmod -w a-directory|. I could
> still |echo something > a-directory| without invoking an error.

My memory is hazy here. Myabe it was the case where |chmod -w a-directory| was issued, but I could still save into that directory from C-C TB or something like that.
Built-in linux emulcation environment to windows (Ubuntu) may change the situation for the better.
Comment on attachment 8891939 [details] [diff] [review]
Creating a directory with 0755 permission instead of 0644

The change to nsFileStreams.cpp looks ok.

The change to nsLocalFileUnix.cpp is just noise that shouldn't be there; please take that out.

r=me on the nsFileStreams bit.
Attachment #8891939 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8891939 [details] [diff] [review]
> Creating a directory with 0755 permission instead of 0644
> 
> The change to nsFileStreams.cpp looks ok.
> 
> The change to nsLocalFileUnix.cpp is just noise that shouldn't be there;
> please take that out.
> 
> r=me on the nsFileStreams bit.

Thank you for the review.

I took out the patch for nsLocalFileUnix.cpp, and put r=bz at the end of the description of the patch.

I will put checkin-needed keyword.
Attachment #8891939 - Attachment is obsolete: true
Attachment #8892098 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/274298ae4f23
nsFileStreamBase::DoOpen() should call directory creation with 0755 permission instead of 0644. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/274298ae4f23
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.