Closed Bug 1403260 Opened 2 years ago Closed 2 years ago

[Mac] Remove access to print server from content process sandbox

Categories

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

57 Branch
Unspecified
macOS
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: haik, Assigned: haik)

References

(Blocks 1 open bug)

Details

(Whiteboard: sb+)

Attachments

(1 file)

This bug is filed to remove access to the CUPS print daemon from the Mac content sandbox. That's the rule referencing /private/var/run/cupsd:

   ; bug 1324610
   (allow network-outbound file-read*
     (literal "/private/var/run/cupsd"))

We can't remove this without some refactoring of the Mac printing code. (See bug 1324610 for details.) Bug 1328975 exists for the refactoring.
Assignee: nobody → haftandilian
Depends on: 1328975
Priority: -- → P1
Summary: [Mac] Removing access to print server from content process sandbox → [Mac] Remove access to print server from content process sandbox
Whiteboard: sb+
Comment on attachment 8920384 [details]
Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

https://reviewboard.mozilla.org/r/191352/#review196768

::: widget/cocoa/nsPrintOptionsX.mm:55
(Diff revision 1)
> +  // If we are in the child process, we don't need to populate
> +  // nsPrintSettingsX completely. Apart from the document title,
> +  // the parent discards this data (bug 1328975). Furthermore,
> +  // reading some of the printer/printing settings from the OS
> +  // causes a connection to the printer to be made which is blocked
> +  // by sandboxing and results in hangs.
> +  if (!XRE_IsParentProcess()) {
> +    return NS_OK;
> +  }
> +

Could we split this into two functions, `SerializeForContent` and `SerializeForParent` or something, instead of this `if` statement?
Comment on attachment 8920384 [details]
Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

https://reviewboard.mozilla.org/r/191352/#review196768

> Could we split this into two functions, `SerializeForContent` and `SerializeForParent` or something, instead of this `if` statement?

Yes, sure. I'll add private methods to nsPrintSettingsX for that.
The fix removes the code in content that uses the native API's for print settings that triggers a connection to the print, allowing us to remove access to the print server. See also bug 1328975.
No longer depends on: 1328975
See Also: → 1328975
Attachment #8920384 - Flags: review?(mconley)
Comment on attachment 8920384 [details]
Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

https://reviewboard.mozilla.org/r/191352/#review197822

Looks good! Thanks so much!
Attachment #8920384 - Flags: review?(mconley) → review+
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ccc0e72f2152
[Mac] Remove access to print server from content process sandbox r=mconley
The backout was due to test browser_printpreview.js failing. That test triggers the print preview UI which involves an nsPrintSettingsX being serialized to the parent and then back to the child. Print preview setup reads the paper size out of the nsPrintSettings, but the patch removed the code to serialize the paper size when the child sends the nsPrintSettings to the parent resulting in it being lost. Adding serialization of the paper size back to the child process serialization code fixed the problem.

In the child process, we end up in nsPrintEngine::DoCommonPrint() which calls nsDeviceContext::InitForPrinting() and then nsDeviceContextSpecProxy::MakePrintTarget() which calls mPrintSettings->GetEffectivePageSize() on an nsPrintSettings object sent from child to parent and then back to the child. If the page dimensions are not set, an error is returned leading to the test failing.
Attachment #8920384 - Flags: review+ → review?(mconley)
Comment on attachment 8920384 [details]
Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

https://reviewboard.mozilla.org/r/191352/#review200638

Looks good, thanks!
Attachment #8920384 - Flags: review?(mconley) → review+
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49def60bda3d
[Mac] Remove access to print server from content process sandbox. r=mconley
https://hg.mozilla.org/mozilla-central/rev/49def60bda3d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1421957
You need to log in before you can comment on or make changes to this bug.