1635428 - ActorManagerChild.jsm fails to load on Windows if it's a symlink

Status: RESOLVED FIXED

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

Description

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

1. Build Gecko
2. Run ./mach mochitest layout/base/tests/test_reftests_with_caret.html and check it succeeds
3. Replace obj-x86_64-pc-mingw32\dist\bin\modules\ActorManagerChild.jsm with a symlink to toolkit\modules\ActorManagerChild.jsm.
4. Run step 2 again

Expected: It should also succeed
Actual: It does not, the failure log says:

[JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: " {file: "chrome://global/content/browser-content.js" line: 12}]

Debugger says mozJSComponentLoader::ObjectForLocation fails inside mozJSComponentLoader::Import.

Comment 1

[Andrew McCreight [:mccr8]]

It would be good to know more precisely where this is going wrong. Looking over ObjectForLocation, my guess is that something is going wrong inside nsIFileURL::GetFile call, but maybe not.

Comment 2

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

Further debugging says it fails by ERROR_ACCESS_DENIED:

[0x0]   xul!ConvertWinError + 0x147   
[0x1]   xul!OpenFile + 0x293   
[0x2]   xul!nsLocalFile::OpenNSPRFileDescMaybeShareDelete + 0xa9   
[0x3]   xul!nsLocalFile::OpenNSPRFileDesc + 0x43   
[0x4]   xul!mozilla::loader::AutoMemMap::init + 0x9d   
[0x5]   xul!mozJSComponentLoader::ObjectForLocation + 0x75e   
[0x6]   xul!mozJSComponentLoader::Import + 0xb09   
[0x7]   xul!mozilla::dom::ChromeUtils::Import + 0x29c   
[0x8]   xul!mozilla::dom::ChromeUtils_Binding::import + 0x516   
[0x9]   xul!CallJSNative + 0x32b   
[0xa]   xul!js::InternalCallOrConstruct + 0xcab   
[0xb]   xul!InternalCall + 0x19a   
[0xc]   xul!js::CallFromStack + 0x23   
[0xd]   xul!Interpret + 0x12e92   
[0xe]   xul!js::RunScript + 0x50d   
[0xf]   xul!js::ExecuteKernel + 0x1dd   
[0x10]   xul!js::Execute + 0x22a   
[0x11]   xul!ExecuteScript + 0x84   
[0x12]   xul!ExecuteScript + 0x24b   
[0x13]   xul!JS::CloneAndExecuteScript + 0x16e   
[0x14]   xul!nsMessageManagerScriptExecutor::LoadScriptInternal + 0x45f   
[0x15]   xul!mozilla::dom::BrowserChild::RecvLoadRemoteScript + 0x154   
[0x16]   xul!mozilla::dom::PBrowserChild::OnMessageReceived + 0xc096   
[0x17]   xul!mozilla::dom::PContentChild::OnMessageReceived + 0x17e   
[0x18]   xul!mozilla::dom::ContentChild::OnMessageReceived + 0x8c   
[0x19]   xul!mozilla::ipc::MessageChannel::DispatchAsyncMessage + 0x179   
[0x1a]   xul!mozilla::ipc::MessageChannel::DispatchMessage + 0x3eb   
[0x1b]   xul!mozilla::ipc::MessageChannel::RunMessage + 0x20f   
[0x1c]   xul!mozilla::ipc::MessageChannel::MessageTask::Run + 0xdd   
[0x1d]   xul!nsThread::ProcessNextEvent + 0xc39   
[0x1e]   xul!NS_ProcessNextEvent + 0x8e   
[0x1f]   xul!mozilla::ipc::MessagePump::Run + 0x26d   
[0x20]   xul!mozilla::ipc::MessagePumpForChildProcess::Run + 0xf7   
[0x21]   xul!MessageLoop::RunInternal + 0x5f   
[0x22]   xul!MessageLoop::RunHandler + 0x5f   
[0x23]   xul!MessageLoop::Run + 0x3e   
[0x24]   xul!nsBaseAppShell::Run + 0x71   
[0x25]   xul!nsAppShell::Run + 0x3c   
[0x26]   xul!XRE_RunAppShell + 0x9b   
[0x27]   xul!mozilla::ipc::MessagePumpForChildProcess::Run + 0x3a   
[0x28]   xul!MessageLoop::RunInternal + 0x5f   
[0x29]   xul!MessageLoop::RunHandler + 0x5f   
[0x2a]   xul!MessageLoop::Run + 0x3e   
[0x2b]   xul!XRE_InitChildProcess + 0xaa0   
[0x2c]   xul!mozilla::BootstrapImpl::XRE_InitChildProcess + 0x2a   
[0x2d]   firefox!content_process_main + 0x115   
[0x2e]   firefox!NS_internal_main + 0x18c   
[0x2f]   firefox!wmain + 0x26f   
[0x30]   firefox!invoke_main + 0x22   
[0x31]   firefox!__scrt_common_main_seh + 0x10c   
[0x32]   KERNEL32!BaseThreadInitThunk + 0x14   
[0x33]   ntdll!RtlUserThreadStart + 0x21   

Not sure why only symlinks fail here.

Comment 3

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

Confused, it only fails for ActorManagerChild.jsm and ContentSessionStore.jsm while it succeeds for others.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:peterv, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

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

I found that a certain content process can read symlinks while other processes cannot. 🤷‍♀️

Disabling browser.tabs.remote.autostart makes it work everywhere, not sure what's happening.

Comment 6

[:Gijs (he/him)]

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

I found that a certain content process can read symlinks while other processes cannot. 🤷‍♀️

Probably the "file" content process. The other content processes are sandboxed so their access to disk is limited.

Disabling browser.tabs.remote.autostart makes it work everywhere, not sure what's happening.

This disables e10s which means no sandboxing (and no content processes, I would have thought...)

Comment 7

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

The other content processes are sandboxed so their access to disk is limited.

It's weird that only symlinks are affected. Is it something we can control?

Comment 8

[:Gijs (he/him)]

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

The other content processes are sandboxed so their access to disk is limited.

It's weird that only symlinks are affected. Is it something we can control?

I... don't think it's that weird? The sandbox will be on the basis of a list of directories/files that the process is allowed to access. If an attacker were able to just create a symlink in the accessible directories/files and point it to somewhere that wasn't accessible, and open/read/write/execute files that way, that'd render the sandbox useless.

Put differently, I would expect that if you replaced the ChromeUtils.import call URI in the content process with a file:/// one, that also wouldn't work, and that if you copied the file to ActorManagerChild2.jsm and symlinked it within the same (ie object) directory, it would work - ie it's nothing to do with it being a symlink, it's to do with the path at which the (target) file lives (in this case, somewhere in the source dir) being outside of the "allowed" set of files (which include the objdir but not the source dir, by the looks of it).

Anyway, I could totally be wrong about all this - I'm really not an expert on our sandbox system. As I noted on slack, you should probably ask :bobowen or :gcp .

Comment 9

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

that if you copied the file to ActorManagerChild2.jsm and symlinked it within the same (ie object) directory

I just tried it but it still fails with access denied error. Maybe it could be a sandbox bug? NIing :bobowen for more details...

Comment 10

[Bob Owen (:bobowen)]

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

that if you copied the file to ActorManagerChild2.jsm and symlinked it within the same (ie object) directory

I just tried it but it still fails with access denied error. Maybe it could be a sandbox bug? NIing :bobowen for more details...

Normally Firefox is installed in Program Files which (at the moment) means the content process can read omni.ja.
Sometime that's not the case (for example it can be installed under the user's home dir or like this for development), so we also add a sandbox policy rule to allow read access to the bin dir anyway.

However, the access brokered using those policy rules doesn't allow reparse points, which are used to create symlinks, so that's why this is failing.
If you were to give Users access to the file or whole source tree that should provide a temporary fix, but once we manage to improve the content process sandbox further it'll probably break again.
When we do that we should try and come up with a way to make this better for devs.

Comment 11

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

Thanks bob for the details!

To make things clear, the rule allows linux symlinks but not the Windows reparse points, am I right? Is there a good reason for that or is it a bug?

Comment 12

[Bob Owen (:bobowen)]

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

Thanks bob for the details!

To make things clear, the rule allows linux symlinks but not the Windows reparse points, am I right? Is there a good reason for that or is it a bug?

The linux sandbox is completely different, I'm not sure how they handle this situation.
Reparse points are an NTFS thing and amongst other things they are used to implement symlinks on Windows.
The chromium sandbox explicitly prevents them being used in its policy rules.

Comment 13

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

For the record this is the line where the Linux sandbox allows access to the local repository directory, per :gcp.

https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#471

Comment 14

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

Attachment: Bug 1635428 - Part 1: Move mozilla::IsDevelopmentBuild to Omnijar.h r=smaug!,mccr8!

Comment 15

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

Attachment: Bug 1635428 - Part 1: Move mozilla::IsDevelopmentBuild to Omnijar.h r=smaug!,mccr8!

Comment 16

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

Attachment: Bug 1635428 - Part 2: Allow access to MOZ_DEVELOPER_REPO_DIR on dev builds r=bobowen

Comment 17

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

Attachment: Bug 1635428 - Part 3: Remove excluded() from files.py r=#firefox-build-system-reviewers

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cef8943e0c6
Part 1: Move mozilla::IsDevelopmentBuild to Omnijar.h r=mccr8

Comment 19

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing build bustages on nsMacUtilsImpl.cpp

Backout link : https://hg.mozilla.org/integration/autoland/rev/984d237ab7b225cdb86759f816abe6a9df953f34

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&author=krosylight%40mozilla.com&selectedTaskRun=RUZbnLlWS6StEPlbJGSd1Q.0

Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=373013847&repo=autoland&lineNumber=40538

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2bfe3ac5c01
Part 1: Move mozilla::IsDevelopmentBuild to Omnijar.h r=mccr8

Comment 21

[Natalia Csoregi [:nataliaCs]]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/654ad1462745
Part 2: Allow access to MOZ_DEVELOPER_REPO_DIR on dev builds r=bobowen

Comment 23

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/654ad1462745

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/597ae59d1786
Part 3: Remove excluded() from files.py r=firefox-build-system-reviewers,glandium

Comment 25

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/597ae59d1786