1201935 - MacOS sandbox breaks WebExtension tests

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Until we remote resource, chrome, and moz-extension URIs, it is not acceptable to prevent read access of files in the content process. It is not uncommon for add-on developers to locate their add-ons in places that are not part of the profile. The current sandbox breaks this.

Right now this is breaking WebExtension tests. I'm going to have to disable them on MacOS. However, we should fix the sandbox rules.

Comment 1

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be72de9ac6a

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3be72de9ac6a

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'll probably regret taking this bug, but it looks like a simple patch, and we could use a little more “bus factor” for Mac sandboxing.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I can't reproduce this locally (10.10.5), so I'll have to test with Try pushes.

For reference, the current default setting of the Mac sandbox prevents access to files in ~/Library (with a few exceptions, like the browser profile) and otherwise allows full read/write.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Jed. Here's a sample failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=13682808&repo=mozilla-inbound

The test creates a zip file in TmpD and then tries to access it in the content process via a moz-extension: URI (which is similar to resource:). Maybe TmpD is returning something in ~/Library?

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] {UTC-7} from comment #4)
> I can't reproduce this locally (10.10.5)

…because I forgot to test with --e10s.  (It's the default for running the browser now, but not the default for testing.  Oops.)

Anyway, TmpD is ~/Library/Caches/TemporaryItems (it's obtained via an OS API, but that's what it is empirically), which isn't whitelisted; bug 1190032 added one particular subdirectory, and this is not it.

So, for this bug it should suffice to allow reading from TmpD.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: whitelist reading from TemporaryItems

This patch also backs out the earlier patch to the test config.

It's worth mentioning that opening files from TemporaryItems is broken without this patch, and that might be something users would actually do — and adjusting SubstitutingProtocolHandler won't fix that.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8658865 [details] [diff] [review]
Patch: whitelist reading from TemporaryItems

I can't comment on backing out the previous patch.  But the change to security/sandbox/mac/Sandbox.mm looks fine to me.

In principal one might worry about our code being able to change temporary directories and files created by other apps ... but lets save that for later.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8658865 [details] [diff] [review]
Patch: whitelist reading from TemporaryItems

> whitelist reading from TemporaryItems

Note that this patch allows both reading and writing to TemporaryItems, to any of its subdirectories, and to any of the items in it.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> and to any of the items in it

and to any of the items in them

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8658865 [details] [diff] [review]
Patch: whitelist reading from TemporaryItems

Review of attachment 8658865 [details] [diff] [review]:
-----------------------------------------------------------------

I still think we should just allow read access to the entire file system. But this fixes the immediate problem.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Steven Michaud [:smichaud] from comment #9)
> Note that this patch allows both reading and writing to TemporaryItems, to
> any of its subdirectories, and to any of the items in it.

Indeed it does, and that's not quite what I meant.  I'll update the patch.

(In reply to Bill McCloskey (:billm) from comment #11)
> I still think we should just allow read access to the entire file system.

I'd tend to agree, but at the same time this isn't my normal area so I'm trying to tread lightly.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: whitelist reading (and only reading) from TemporaryItems

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8658929 [details] [diff] [review]
Patch: whitelist reading (and only reading) from TemporaryItems

Looks fine to me.  And yes, this one only allows reading.

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: whitelist reading (and only reading) from TemporaryItems [v2]

Sorry about losing track of this bug.  The patch needed some rebasing, but nothing major, so I'm carrying over r+ (but re-running Try).

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3c3a1b26874 (failures are empty test runs from |mach try|); also did https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e079417e80f because of some confusion over what does/doesn't get run for Mac e10s.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9e63379f6a

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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