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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

Tracking

Trunk
mozilla55
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: sbmc2)

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1361304#c1 describes what we're trying to remove here, and what the blocker is.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Whiteboard: sbmc2
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → agaynor
(Assignee)

Updated

2 years ago
Attachment #8864171 - Flags: review?(haftandilian)
No longer blocks: 1361304

Comment 6

2 years ago
mozreview-review
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.
Attachment #8864171 - Flags: review?(haftandilian) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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.
Attachment #8864171 - Flags: review?(haftandilian) → review+
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
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
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45cc3ebc0446
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.