1419811 - [macOS][e10s] File listings no longer show icons for non-folders

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

This works for me in 53 and fails in 57, 58 and nightly.

STR:

1. open Firefox with e10s enabled
2. open file:///

ER:

file icons for individual files

AR:
only folders have icons.

I wouldn't be surprised if this is related to file processes and/or sandboxing changes which are breaking moz-icon: references to these icons...

Comment 1

[:Gijs (he/him)]

I can't reproduce this on Windows with 58...

Comment 2

[:Gijs (he/him)]

56 nightly was fine, at least. Still tracking down the regression window further.

Comment 3

[:Gijs (he/him)]

Summary in bug 1388360 looks suspiciously like what would cause this (and fits regression window - still confirming). Alex, is that possible?

Comment 4

[:Gijs (he/him)]

Confirmed that this is from bug 1388360.

Some options:

1) we could revert that bug.
2) we could only allow the access for the file: content process, not the other ones. This would break ftp: listings and web access, and I think that's a feature rather than a bug (it'd only break on macOS so we still need to fix bug 1222924).
3) we could leave it like this and provide plain file icons ourselves like we do for folders
4) we could come up with some way of passing the icon information through the parent

I think (4) isn't worth it and (1) might be a good idea for 58. I could live with (2), and kind of assume that file read access is "worse" than access to this service, but ymmv, and I dunno if it's easy to change sandbox restrictions based on content process type. (3) would be kind of sad, but another option we could take...

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Doing (2) would be equally easy as (1) -- if possible I'd be happier if we could do this (I assume FTP usage is very low).

On the longer time scale, we'd ultimately need to do (3) or (4), since we want to restrict content process's access to system services, since they're all potential sandbox escape vectors. So we should pick both a short term and a long term solution here (maybe we'll be lucky and they're the same).

Comment 6

[:Gijs (he/him)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> Doing (2) would be equally easy as (1) -- if possible I'd be happier if we
> could do this (I assume FTP usage is very low).

That's fine, I wasn't sure how hard (2) would be. This would fix the bug immediately, AIUI (and again, I don't think we care about ftp: listings here much - they've been on notice since the comments in 1222924 for Some Time).

> On the longer time scale, we'd ultimately need to do (3) or (4), since we
> want to restrict content process's access to system services, since they're
> all potential sandbox escape vectors. So we should pick both a short term
> and a long term solution here (maybe we'll be lucky and they're the same).

Would (4) actually significantly reduce exposure to such sandbox escape vectors? That is, the minimal information that goes to the system service is (I assume) the file extension, and what comes back is an icon. While we can relay all that through the parent, I'm struggling to see a scenario where that buys us much, unless the system service is crap at validating input and we're better. Which is a possibility, but it seems like a significant amount of work (again: I think - maybe there's existing infra we can piggyback on) for the case where the input is usually a 3-letter string, but funneling the output through to the content process is going to be a pain...

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

(2) -- sounds good, I'll upload a patch momentarily.

I'm not familiar with the precise details of this service, but the concern is generally that in addition to the "give me an icon for this extension" API method it may have other API methods such as "give me an icon for this bag of bytes" (e.g. libmagic) which is significantly more complex. We don't have control (or much visibility) into which methods a given services exposes, so our philosophy is to try minimize our trusted computing base as much as possible -- a single method is much smaller and easier to reason about than "all the methods this service will ever contain" :-)

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1419811 - allow file content processes to access the com.apple.iconservices service;

Directory listing for file URLs needs access to draw icons for files.

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

Comment 9

[:Gijs (he/him)]

Comment on attachment 8931025 [details]
Bug 1419811 - allow file content processes to access the com.apple.iconservices service;

https://reviewboard.mozilla.org/r/202122/#review207490

rs=me on the concept, I can't speak to the implementation. Thanks for picking this up so quickly!

Comment 10

[Haik Aftandilian [:haik]]

(In reply to :Gijs from comment #6)
> I'm struggling to see a scenario where
> that buys us much, unless the system service is crap at validating input and
> we're better.

That would be the benefit to remoting this API through the parent. i.e., we more tightly control the input to the icon service/API. There have definitely been bugs in the input validation for these Mac services and that's the motivation for removing as many of them as possible.

I think we should file a bug to investigate removing access to the API and proxying it through the parent unless the fix for bug 1222924 will make it this API unneeded.

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8931025 [details]
Bug 1419811 - allow file content processes to access the com.apple.iconservices service;

https://reviewboard.mozilla.org/r/202122/#review207496

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Haik: filed bug 1419839.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca944df78501
allow file content processes to access the com.apple.iconservices service; r=Gijs,haik

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ca944df78501

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8931025 [details]
Bug 1419811 - allow file content processes to access the com.apple.iconservices service;

Approval Request Comment
[Feature/Bug causing the regression]:

Caused by bug 1388360, which landed in 57.

[User impact if declined]:

On macOS, icons on file:// directory listings will be absent.

[Is this code covered by automated tests?]:

No.

[Has the fix been verified in Nightly?]:

Yes.

[Needs manual test from QE? If yes, steps to reproduce]:

No.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

Nope.

[Why is the change risky/not risky?]:

Straightforward change to the macOS sandbox policy.

[String changes made/needed]:

None.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8931025 [details]
Bug 1419811 - allow file content processes to access the com.apple.iconservices service;

Fix a regression that file listings don't show icons for non-folders. Beta58+.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/40a6d6c38072