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)
Core
Networking: File
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ishikawa, Assigned: ishikawa)
References
Details
Attachments
(1 file, 1 obsolete file)
1.03 KB,
patch
|
ishikawa
:
review+
|
Details | Diff | Splinter Review |
|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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
(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.
Assignee | ||
Comment 3•7 years ago
|
||
(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 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
(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+
Assignee | ||
Updated•7 years ago
|
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/274298ae4f23
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•