1695556 - Windows sandbox does not allow accessing files in MOZ_DEVELOPER_REPO_DIR

Status: RESOLVED FIXED

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

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Bug 1321922 added symlink support to Windows but the sandbox does not allow symlinks to files inside MOZ_DEVELOPER_REPO_DIR, causing bug 1635428 and bug 1694675. The Linux sandbox explicitly adds the directory as an exception and the Windows one should also be able to do the same.

This currently degrades Windows dev experience.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

https://searchfox.org/mozilla-central/rev/26330a08b1f9d06938faa0aa5e0f8c7a58064aa2/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc#413

It seems the Chromium Windows sandbox itself does not support symlinks at all. Can we somehow workaround this?

Comment 2

[Bob Owen (:bobowen)]

(In reply to Kagami :saschanaz from comment #1)

https://searchfox.org/mozilla-central/rev/26330a08b1f9d06938faa0aa5e0f8c7a58064aa2/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc#413

It seems the Chromium Windows sandbox itself does not support symlinks at all. Can we somehow workaround this?

I think maybe we would have to add a change into the chromium sandbox code to allow rules to be added that ignored the reparse point when it is a developer build.
It might be easier to add the option for the sandbox and then only use it for developer builds.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Does that mean we are free to modify the sandbox code without pinging upstream Chromium? 👀

I'd like to try this if anyone else is not interested, since it just got S4/P5 😬. This is biting me recently.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Kagami :saschanaz from comment #3)

Does that mean we are free to modify the sandbox code without pinging upstream Chromium? 👀

I'd like to try this if anyone else is not interested, since it just got S4/P5 😬. This is biting me recently.

We have quite a few patches, I'd be OK with one for this I think.
We could do with upstreaming at least some of our patches, but it's all a matter of priorities, as usual.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Cool, are there some patch files maintained separately or can I directly modify the files?

Comment 6

[Bob Owen (:bobowen)]

(In reply to Kagami :saschanaz from comment #5)

Cool, are there some patch files maintained separately or can I directly modify the files?

We modify directly, but also add a patch into here for the changes, so we can re-apply when we pull a new version from upstream.
There is a patch_order.txt file, to add the patch name to as well.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1695556 - Part 1: Resolve symlinks on sandbox

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1695556 - Part 2: Allow access to MOZ_DEVELOPER_REPO_DIR on dev builds

Depends on D112285

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1695556 - Part 3: Remove excluded() from files.py

Depends on D112286

Comment 10

[Bob Owen (:bobowen)]

I have a patch to allow symbolic links in the sandbox policy rules.
Just need to remove the various bits of resolution and see if I can come up with some meaningful tests.

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

Sounds cool, thanks!

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Bob Owen (:bobowen) from comment #10)

I have a patch to allow symbolic links in the sandbox policy rules.
Just need to remove the various bits of resolution and see if I can come up with some meaningful tests.

Bob: we believe that resolving this issue will continue to help MSIX edge cases, the latest being Bug 1748441. Can we get a status update? I will work with my manager, ahabibi@, to get this prioritized appropriately. Thanks!

Comment 13

[Bob Owen (:bobowen)]

(In reply to Nick Alexander :nalexander [he/him] from comment #12)

(In reply to Bob Owen (:bobowen) from comment #10)

I have a patch to allow symbolic links in the sandbox policy rules.
Just need to remove the various bits of resolution and see if I can come up with some meaningful tests.

Bob: we believe that resolving this issue will continue to help MSIX edge cases, the latest being Bug 1748441. Can we get a status update? I will work with my manager, ahabibi@, to get this prioritized appropriately. Thanks!

Sorry about the delay here. I have a patch to hopefully resolve this problem and I've been in a discussion with chromium devs about this and possible extra checks/safe guards around symbolic links.
I'll get the patch up onto the bug early next week, so you can test and hopefully we can get it landed soon.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Bob Owen (:bobowen) from comment #13)

(In reply to Nick Alexander :nalexander [he/him] from comment #12)

(In reply to Bob Owen (:bobowen) from comment #10)

I have a patch to allow symbolic links in the sandbox policy rules.
Just need to remove the various bits of resolution and see if I can come up with some meaningful tests.

Bob: we believe that resolving this issue will continue to help MSIX edge cases, the latest being Bug 1748441. Can we get a status update? I will work with my manager, ahabibi@, to get this prioritized appropriately. Thanks!

Sorry about the delay here. I have a patch to hopefully resolve this problem and I've been in a discussion with chromium devs about this and possible extra checks/safe guards around symbolic links.
I'll get the patch up onto the bug early next week, so you can test and hopefully we can get it landed soon.

Bob: That sounds great, thanks!

Comment 15

[Bob Owen (:bobowen)]

Attachment: Bug 1695556 p1: Allow reparse points in chromium sandbox code. r=handyman!

Comment 16

[Bob Owen (:bobowen)]

Attachment: Bug 1695556 p2: Stop resolving symlinks for content sandbox rules. r=handyman!

Depends on D135692

Comment 17

[Bob Owen (:bobowen)]

Attachment: Bug 1695556 p3: Add file tests for content process sandbox. r=handyman!

Depends on D135693

Comment 18

[Bob Owen (:bobowen)]

I think these patches are pretty much ready for review.
The only thing I think I need to do is add a chromium changes patch to the first one, but that doesn't affect the functionality.

I've not heard back from the chromium people, but assuming this fixes the issues for bug 1727878, I think I'm comfortable on landing this given the conversation we've already had.
If it doesn't fix things for that bug, then I'll probably wait until the other idea for checks has been explored.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Bob Owen (:bobowen) from comment #18)

I think these patches are pretty much ready for review.
The only thing I think I need to do is add a chromium changes patch to the first one, but that doesn't affect the functionality.

I've not heard back from the chromium people, but assuming this fixes the issues for bug 1727878, I think I'm comfortable on landing this given the conversation we've already had.
If it doesn't fix things for that bug, then I'll probably wait until the other idea for checks has been explored.

Bob: thanks for posting these. I can confirm that with these applied, my local build packaged as an MSIX can successfully refresh. That is, this appears to address Bug 1727878. Great work!

If you could shepherd this to the finish line, we would appreciate it. Note that the testing patch, at least, needed to be rebased when I applied it.

Comment 20

[Bob Owen (:bobowen)]

Thanks, I've Been tied up with win32k stuff, but I'll get the patches finished and up for review now.
I know it will delay by another release, but as I think I've said before, it's probably best to land these at the start of a cycle.

Comment 21

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=759049fa6b7fb43ededee45b3edd72e02c3c4a6c

Comment 22

[Bob Owen (:bobowen)]

Attachment: Bug 1695556 p1a: Add allow reparse points in chromium sandbox code patch. r=handyman!

Depends on D135692

Comment 23

[Nick Alexander :nalexander [he/him]]

bobowen: is this waiting on anything to land?

Comment 24

[Bob Owen (:bobowen)]

(In reply to Nick Alexander :nalexander [he/him] from comment #23)

bobowen: is this waiting on anything to land?

No just had to make some changes from the review and a recent build change meant I had to set up my build environment again.
https://treeherder.mozilla.org/jobs?repo=try&revision=b8530395c1bf02da1974f5a3630ef41613648e3f

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a8fa3fdafb2
p1: Allow reparse points in chromium sandbox code. r=handyman
https://hg.mozilla.org/integration/autoland/rev/22d4074d46cd
p1a: Add allow reparse points in chromium sandbox code patch. r=handyman
https://hg.mozilla.org/integration/autoland/rev/f18b4a63a6ef
p2: Stop resolving symlinks for content sandbox rules. r=handyman
https://hg.mozilla.org/integration/autoland/rev/558688086ef6
p3: Add file tests for content process sandbox. r=handyman,ipc-reviewers,jld

Comment 26

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5a8fa3fdafb2
https://hg.mozilla.org/mozilla-central/rev/22d4074d46cd
https://hg.mozilla.org/mozilla-central/rev/f18b4a63a6ef
https://hg.mozilla.org/mozilla-central/rev/558688086ef6

Comment 27

[Phabricator Automation]

Comment on attachment 9216246 [details]
WIP: Bug 1695556 - Part 2: Allow access to MOZ_DEVELOPER_REPO_DIR on dev builds

Revision D112286 was moved to bug 1635428. Setting attachment 9216246 [details] to obsolete.

Comment 28

[Phabricator Automation]

Comment on attachment 9216247 [details]
WIP: Bug 1695556 - Part 3: Remove excluded() from files.py

Revision D112287 was moved to bug 1635428. Setting attachment 9216247 [details] to obsolete.