1713776 - Crash in [@ __faccessat]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/bff96d05-357a-436a-8f63-d8b2a0210529

Reason: SIGSYS

Top 10 frames of crashing thread:

0 libc.so.6 __faccessat sysdeps/unix/sysv/linux/faccessat.c:29
1 libc.so.6 realpath_stk stdlib/stdlib/canonicalize.c:389
2 libc.so.6 realpath@@GLIBC_2.3 stdlib/stdlib/canonicalize.c:438
3 libxul.so nsLocalFile::Normalize xpcom/io/nsLocalFileUnix.cpp:598
4 libxul.so NS_RelaxStrictFileOriginPolicy netwerk/base/nsNetUtil.cpp:2475
5 libxul.so mozilla::ContentPrincipal::MayLoadInternal caps/ContentPrincipal.cpp:311
6 libxul.so mozilla::BasePrincipal::CheckMayLoad caps/BasePrincipal.cpp:534
7 libxul.so nsContentUtils::ChannelShouldInheritPrincipal dom/base/nsContentUtils.cpp:6913
8 libxul.so nsDocShell::InternalLoad docshell/base/nsDocShell.cpp:9628
9 libxul.so nsDocShell::OnLinkClickSync docshell/base/nsDocShell.cpp:12952

It seems that recent libc's on Ubuntu are calling the faccessat2 syscall (number 439) when calling realpath(). There's just a handful of crashes on file so this looks like a new thing.

Comment 1

[:gerard-majax]

https://sourceware.org/git/?p=glibc.git;a=commit;f=sysdeps/unix/sysv/linux/faccessat.c;h=3d3ab573a5f3071992cbc4f57d50d1d29d55bde2

Comment 2

[:gerard-majax]

Introduced by kernel 5.8: https://elixir.bootlin.com/linux/v5.8/source/fs/open.c#L474

Comment 3

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #2)

Introduced by kernel 5.8: https://elixir.bootlin.com/linux/v5.8/source/fs/open.c#L474

https://sourceware.org/git/?p=glibc.git;a=blobdiff;f=sysdeps/unix/sysv/linux/kernel-features.h;h=a6bbc3c94e8b4360690741cc9079d47bfa5b248e;hp=32533e94cf6febc396bf9fdcae6f00084c915049;hb=3d3ab573a5f3071992cbc4f57d50d1d29d55bde2;hpb=268ed383b57aa136d2e3a40be18a41cadba9e052

Comment 4

[:gerard-majax]

I think it is fine allowing, from man faccessat2:

   faccessat2()
       The  description  of faccessat() given above corresponds to POSIX.1 and to the implementation provided by glibc.  However, the glibc implementation was an imperfect emulation (see BUGS) that papered over the fact that the raw Linux faccessat() system call does not have a flags argument.  To allow for a proper implementation, Linux 5.8 added the faccessat2() system call, which supports the flags argument and allows a correct implementation of the faccessat() wrapper function.

Comment 5

[:gerard-majax]

The broker side might need some more work, though: https://searchfox.org/mozilla-central/rev/e9eb869e90a8d717678c3f38bf75843e345729ab/security/sandbox/linux/SandboxFilter.cpp#289-304

Comment 6

[Gian-Carlo Pascutto [:gcp]]

All the crashes are in content(file) so maybe the issue isn't necessarily with brokering but with the unbrokered version not knowing about the syscall?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

File content processes do have a broker, it just allows read access everywhere: https://searchfox.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#661

Comment 8

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

The crash report shows it's passing AT_EACCESS, which is the one that makes access work the same as stat (using euid instead of ruid). It shouldn't make a difference for desktop Firefox; B2G used to set broker threads' euids to that of the client process, to prevent confused deputy problems if the policy was too permissive, but that code was removed. (I don't know what KaiOS is doing here.) See also bug 1386279 and bug 1382246.

So I think we could just silently ignore AT_EACCESS, because we shouldn't be in a situation where that's observable. The other option is to fail with ENOSYS and force userspace fallback. (Other solutions include: sending the call to the broker complete with flags, and having it return ENOSYS if the kernel doesn't have faccessat2 and flags != 0, in which case we'd get userspace fallback plus the cost of an additional broker call; or, to optimize that, detect whether the kernel has faccessat2 before starting the sandbox, and return ENOSYS directly from seccomp if not. That's probably more complexity than we need at this time.)

In this crash report it's not using AT_SYMLINK_NOFOLLOW, which suggests that we don't need to worry about that (yet).

Comment 9

[:gerard-majax]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #8)

The crash report shows it's passing AT_EACCESS, which is the one that makes access work the same as stat (using euid instead of ruid). It shouldn't make a difference for desktop Firefox; B2G used to set broker threads' euids to that of the client process, to prevent confused deputy problems if the policy was too permissive, but that code was removed. (I don't know what KaiOS is doing here.) See also bug 1386279 and bug 1382246.

So I think we could just silently ignore AT_EACCESS, because we shouldn't be in a situation where that's observable. The other option is to fail with ENOSYS and force userspace fallback. (Other solutions include: sending the call to the broker complete with flags, and having it return ENOSYS if the kernel doesn't have faccessat2 and flags != 0, in which case we'd get userspace fallback plus the cost of an additional broker call; or, to optimize that, detect whether the kernel has faccessat2 before starting the sandbox, and return ENOSYS directly from seccomp if not. That's probably more complexity than we need at this time.)

In this crash report it's not using AT_SYMLINK_NOFOLLOW, which suggests that we don't need to worry about that (yet).

I think the call to faccessat is already brokered because of comment #7 and because adding __NR_faccessat2 at https://searchfox.org/mozilla-central/rev/e9eb869e90a8d717678c3f38bf75843e345729ab/security/sandbox/linux/SandboxFilter.cpp#631-632 is enough to get the call to complete correctly.

Comment 10

[:gerard-majax]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #8)

[...]

So I think we could just silently ignore AT_EACCESS, because we shouldn't be in a situation where that's observable. The other option is to fail with ENOSYS and force userspace fallback. (Other solutions include: sending the call to the broker complete with flags, and having it return ENOSYS if the kernel doesn't have faccessat2 and flags != 0, in which case we'd get userspace fallback plus the cost of an additional broker call; or, to optimize that, detect whether the kernel has faccessat2 before starting the sandbox, and return ENOSYS directly from seccomp if not. That's probably more complexity than we need at this time.)

I'm not completely sure I get your point there on the part of forcing user fallback: we receive a __NR_faccessat2 syscall from the glibc faccessat implementation https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/faccessat.c;h=13160d32499c4e581d78e451461f280a4377cf3e;hb=HEAD#l29 and when __ASSUME_FACCESSAT2 is defined (which is the case on glibc 2.33 with kernel v5.8+) then it's the end of the flow.

Comment 11

[:gerard-majax]

Attachment: Bug 1713776 - Allow faccessat2 r?gcp

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/486b10fd4485
Allow faccessat2 r=gcp

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/486b10fd4485

Comment 14

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

The patch landed in nightly and beta is affected.
:gerard-majax, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
If yes, don't forget to request an uplift for the patches in the regression caused by this fix.

For more information, please visit auto_nag documentation.

Comment 15

[:gerard-majax]

Comment on attachment 9224700 [details]
Bug 1713776 - Allow faccessat2 r?gcp

Beta/Release Uplift Approval Request

• User impact if declined: Accessing user data (reading local webpage, uploading content) might fail on recent linux kernel + glibc combination (e.g., ubuntu 21.04)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1714459
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky: without the change, feature is broken and unusable. The fix is small, consists just in adding handling for the new syscall using existing code path.
• String changes made/needed: N/A

Comment 16

[Julien Cristau [:jcristau]]

AIUI, without the change, glibc will fall back to faccessat on its own (outside of nightly, anyway), so there's no actual user-visible change?

Comment 17

[:gerard-majax]

(In reply to Julien Cristau [:jcristau] from comment #16)

AIUI, without the change, glibc will fall back to faccessat on its own (outside of nightly, anyway), so there's no actual user-visible change?

No, glibc will force faccessat2 when conditions are met, see __ASSUME_FACCESSAT2: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/faccessat.c;h=13160d32499c4e581d78e451461f280a4377cf3e;hb=HEAD#l30

Comment 18

[Julien Cristau [:jcristau]]

In theory, yes. In practice, I'd be surprised if any distro built glibc with --enable-kernel=5.8 just yet.