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

Status: RESOLVED FIXED

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

Description

[Haik Aftandilian [:haik]]

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.

Comment 1

[Haik Aftandilian [:haik]]

Attachment: Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

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

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

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 3

[Haik Aftandilian [:haik]]

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.

Comment 4

[Haik Aftandilian [:haik]]

Comment on attachment 8920384 [details]
Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

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

Comment 5

[Haik Aftandilian [:haik]]

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.

Comment 6

[Mike Conley (: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!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ccc0e72f2152
[Mac] Remove access to print server from content process sandbox r=mconley

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/485a03afaa23 - apparently you'll have to do something about https://treeherder.mozilla.org/logviewer.html#?job_id=139749059&repo=autoland&lineNumber=2431 before you do that.

Comment 9

[Haik Aftandilian [:haik]]

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.

Comment 10

[Haik Aftandilian [:haik]]

Comment on attachment 8920384 [details]
Bug 1403260 - [Mac] Remove access to print server from content process sandbox.

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

Comment 11

[Mike Conley (: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!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49def60bda3d
[Mac] Remove access to print server from content process sandbox. r=mconley

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/49def60bda3d