1405088 - [mac] remove remaining file-write permissions

Status: RESOLVED FIXED

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

Description

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Isn't this also needed for crash metadata files? https://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/toolkit/crashreporter/nsExceptionHandler.cpp#1379

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Grumble, well, that code certainly looks like it does; assuming that runs in content!

Do we not have any tests for this?

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1405088 - Part 1 - remove file-write permissions from macOS content temporary directory;

With this change, the macOS content sandbox has no ability to create files
anywhere on disk (in release builds). If the content process needs a file to
write to, it needs to obtain a file descriptor from the parent process.

Review commit: https://reviewboard.mozilla.org/r/219910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219910/

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1405088 - Part 2 - Don't even setup the temp directory in content processes on macOS now that it is unused;

Review commit: https://reviewboard.mozilla.org/r/219912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219912/

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Green try runs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd599061373c27ab3b2783c3a0e8fde14e1e53e&group_state=expanded
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ebd41ff32208428d4f2c2b5de707712b5a27d0&group_state=expanded

Comment 6

[Haik Aftandilian [:haik]]

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.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8950660 [details]
Bug 1405088 - Part 1 - remove file-write permissions from macOS content temporary directory;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219910/diff/1-2/

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

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;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219912/diff/1-2/

Comment 9

[Haik Aftandilian [:haik]]

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 10

[Alex Gaynor [:Alex_Gaynor]]

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 11

[Haik Aftandilian [:haik]]

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 12

[Haik Aftandilian [:haik]]

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

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6d46b232c926
https://hg.mozilla.org/mozilla-central/rev/4cecfca0155b