Closed Bug 1405088 Opened 5 years ago Closed 5 years ago

[mac] remove remaining file-write permissions

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

Unspecified
macOS
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

References

(Blocks 2 open bugs)

Details

(Whiteboard: sb+)

Attachments

(2 files)

Right now we use file-write permissions on macOS for two things in the sandbox:

- printing
- NPAPI _tests_ (but not actual code!)

Once we fix these two we should drop all file-write.
Grumble, well, that code certainly looks like it does; assuming that runs in content!

Do we not have any tests for this?
Priority: -- → P3
Whiteboard: sb+
Depends on: 1407693
Depends on: 1414834
Blocks: 1393259
No longer blocks: 1393259
Assignee: nobody → agaynor
Priority: P3 → P1
Comment on attachment 8950660 [details]
Bug 1405088 - Part 1 - remove file-write permissions from macOS content temporary directory;

https://reviewboard.mozilla.org/r/219910/#review225714

This is great. Nice work getting this and the dependencies all in!

::: security/sandbox/test/browser_content_sandbox_fs.js:228
(Diff revision 1)
>  // Test if the content process can create a temp file, should pass. Also test
>  // that the content process cannot create symlinks or delete files.

Please update the comment to make it clear Mac blocks creating a temp file and that's expected because all file write is blocked.

::: security/sandbox/test/browser_content_sandbox_syscalls.js:197
(Diff revision 1)
>      // open a file for writing in the content temp dir, this should work
>      // and the open handler in the content process closes the file for us

Update comment in the same way.
Attachment #8950660 - Flags: review?(haftandilian) → review+
Comment on attachment 8950661 [details]
Bug 1405088 - Part 2 - Don't even setup the temp directory in content processes on macOS now that it is unused;

https://reviewboard.mozilla.org/r/219912/#review225720

It looks like we'll still call LoadContentProcessTempDir() in the parent and setup the temp dir pref "security.sandbox.content.tempDirSuffix". It would be nice if that didn't exist on Mac. Any reason we can't drop that on Mac now?
Comment on attachment 8950661 [details]
Bug 1405088 - Part 2 - Don't even setup the temp directory in content processes on macOS now that it is unused;

https://reviewboard.mozilla.org/r/219912/#review225720

This looks like it'll be slightly more involved than just removing the code on macOS; what do you think about doing this in a follow up patch?
Comment on attachment 8950661 [details]
Bug 1405088 - Part 2 - Don't even setup the temp directory in content processes on macOS now that it is unused;

https://reviewboard.mozilla.org/r/219912/#review225720

That sounds fine to me. Getting a bug filed would let us prioritize it accordingly. You don't have to be the person that fixes it.
Comment on attachment 8950661 [details]
Bug 1405088 - Part 2 - Don't even setup the temp directory in content processes on macOS now that it is unused;

https://reviewboard.mozilla.org/r/219912/#review225734
Attachment #8950661 - Flags: review?(haftandilian) → review+
Thanks!
Keywords: checkin-needed
Blocks: 1437977
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d46b232c926
Part 1 - remove file-write permissions from macOS content temporary directory; r=haik
https://hg.mozilla.org/integration/autoland/rev/4cecfca0155b
Part 2 - Don't even setup the temp directory in content processes on macOS now that it is unused; r=haik
https://hg.mozilla.org/mozilla-central/rev/6d46b232c926
https://hg.mozilla.org/mozilla-central/rev/4cecfca0155b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.