1361733 - [mac] Don't allow writes to all of /var/private in debug builds

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Alex Gaynor [:Alex_Gaynor]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1361304#c1 describes what we're trying to remove here, and what the blocker is.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

This permission was needed for the memory bloat logging -- specifically for
logging intentionally crashing processes. Now we restrict ourselves to only
allowing writes to the location needed for this logging, rather than all of
/var/private.

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

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

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

Comment 3

[Haik Aftandilian [:haik]]

The particular error the /private/var DEBUG write-access rule was added for is the leakcheck test which is run for (all?) DEBUG tests.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1284588#c10

02:41:44  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | tab process: missing output line for total leaks!
02:41:44     INFO -  TEST-INFO | leakcheck | missing output line from log file /var/folders/f6/kjqp0l7n7cb307nv1hq0lrf400000w/T/tmpgFyenB.mozrunner/runtests_leaks_tab_pid1975.log

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

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

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Try results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef3b88535c44ed701d8d8748c44345e1d417544&group_state=expanded

Comment 6

[Haik Aftandilian [:haik]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

https://reviewboard.mozilla.org/r/135834/#review139386

::: commit-message-82c2d:1
(Diff revision 3)
> +Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /var/private

Nit: /private/var instead of /var/private.

::: commit-message-82c2d:4
(Diff revision 3)
> +Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /var/private
> +
> +This permission was needed for the memory bloat logging -- specifically for
> +logging intentionally crashing processes. Now we restrict ourselves to only

And for the leaktest right?

::: dom/ipc/ContentChild.cpp:1286
(Diff revision 3)
>  
> +// This function is only used in an |#ifdef DEBUG| path.
> +#ifdef DEBUG
> +// Given a path to a file, return the directory which contains it.
> +static nsAutoCString
> +GetDirectoryPath(char *path) {

Can the path argument be const? Should be 'aPath' per coding guidelines.

::: layout/tools/reftest/runreftest.py:388
(Diff revision 3)
>                  return None
>              browserEnv[v[:ix]] = v[ix + 1:]
>  
>          # Enable leaks detection to its own log file.
>          self.leakLogFile = os.path.join(profileDir, "runreftest_leaks.log")
> -        browserEnv["XPCOM_MEM_BLOAT_LOG"] = self.leakLogFile
> +        browserEnv["XPCOM_MEM_BLOAT_LOG"] = os.path.realpath(self.leakLogFile)

Rather than depending on XPCOM_MEM_BLOAT_LOG not being a symlinked path, I think it would be better to make use of nsIFile::Normalize() in GetDirectoryPath().

With bug 1361304, I'm adding (allow file-read-metadata ..) rules for /private/var so the ::Normalize call should be OK.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135834/diff/3-4/

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

https://reviewboard.mozilla.org/r/135834/#review139386

> And for the leaktest right?

Yes, indeed. Fixed.

Comment 9

[Haik Aftandilian [:haik]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

https://reviewboard.mozilla.org/r/135834/#review139684

I think this will have to land after 1361304 because it depends on resolving paths that link to within /private/var, but if tests are green no need to wait.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135834/diff/4-5/

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8864171 [details]
Bug 1361733 - In debug builds, do not allow content sandbox to write to all of /private/var

https://reviewboard.mozilla.org/r/135834/#review140168

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45cc3ebc0446
In debug builds, do not allow content sandbox to write to all of /private/var r=haik

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45cc3ebc0446