Closed Bug 1673202 Opened 1 year ago Closed 1 year ago

[Fedora 34] Startup Crash in [@ mozilla::SandboxPolicyCommon::StatAtTrap] and [@ mozilla::SigSysHandler]

Categories

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

Desktop
Linux
defect

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox-esr78 --- fixed
firefox81 --- unaffected
firefox82 --- wontfix
firefox83 --- wontfix
firefox84 --- fixed

People

(Reporter: aryx, Assigned: jld)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

Crash reports for at least 5 installations, all running Fedora 34

Crash report: https://crash-stats.mozilla.org/report/index/2b5823c7-77dd-464e-87fe-b6abe0201022

Reason: SIGSEGV /SEGV_MAPERR

Top 1 frames of crashing thread:

0 libmozsandbox.so mozilla::SandboxPolicyCommon::StatAtTrap /builddir/build/BUILD/firefox-82.0/security/sandbox/linux/SandboxFilter.cpp:240
Summary: [Fedora 34] Crash in [@ mozilla::SandboxPolicyCommon::StatAtTrap] → [Fedora 34] Startup Crash in [@ mozilla::SandboxPolicyCommon::StatAtTrap]

This is bad. The crashes all seem to be on writes to the stack.

I think this is infinite recursion: we catch fstatat, recognize the AT_EMPTY_PATH flag and empty path, and map it to a call to fstat… and then libc maps it back to fstatat.

This was added in glibc 2.33, judging by the symbol versions:

00000000000f1730 W fstat@@GLIBC_2.33
00000000000f1730 W fstat64@@GLIBC_2.33

(gdb) disas fstat
Dump of assembler code for function fstat64:
   0x00000000000f1730 <+0>:     endbr64 
   0x00000000000f1734 <+4>:     mov    %rsi,%rdx
   0x00000000000f1737 <+7>:     mov    $0x1000,%ecx
   0x00000000000f173c <+12>:    lea    0x9dddf(%rip),%rsi        # 0x18f522
   0x00000000000f1743 <+19>:    jmpq   0xf1770 <fstatat64>

Older glibc has fstat as an inline function that expands into a call to __fxstat, which (despite the name suggesting statx) is actually fstat with an ABI version number(?).

This is fixable: invoke the fstat syscall directly.

But it's not great that glibc does that; this case can't be handled in seccomp-bpf itself (because it depends on the memory at the path pointer), so every call to fstat is going to have to be processed in userland code. Currently that means a signal handler; post-USERNOTIF, that means context switching to another process, extracting the fd from the child, fstating it in the other process, writing the result back into the child's address space, and then finally unblocking the child. This is in contrast to the real fstat, which can just be allowed in the seccomp filter (a relatively fast path that the kernel developers are trying to make faster).

I suppose if that becomes a performance issue, we could interpose fstat to call the real fstat.

Assignee: nobody → jld
Severity: -- → S1
Crash Signature: [@ mozilla::SandboxPolicyCommon::StatAtTrap] → [@ mozilla::SandboxPolicyCommon::StatAtTrap] [@ mozilla::SigSysHandler]
OS: Unspecified → Linux
Priority: -- → P1
Hardware: Unspecified → Desktop
Summary: [Fedora 34] Startup Crash in [@ mozilla::SandboxPolicyCommon::StatAtTrap] → [Fedora 34] Startup Crash in [@ mozilla::SandboxPolicyCommon::StatAtTrap] and [@ mozilla::SigSysHandler]

Fedora's bug report has a proper stack trace (ours cuts off after the first frame), clearly showing the unbounded recursion.

Regressed by: 1660901

The glibc change that caused this seems to be 8ed005daf.

As noticed on the Fedora bug, this affects only their builds (and only Rawhide, not release Fedora; glibc 2.33 isn't released yet), and not Mozilla's builds, because it depends on whether those inline function definitions are present in the headers on the build host.

Another problem: GMP processes have no broker, so they don't get the fstatat trap, so fstat will fail with ENOSYS (or, on Nightly, crash with SIGSYS). That's less severe; I'll file a followup bug.

Sandbox policies handle the case of fstatat(fd, "", AT_EMPTY_PATH|...)
by invoking the SIGSYS handler (because seccomp-bpf can't tell if the
string will be empty when the syscall would use it), which makes the
equivalent call to fstat.

Unfortunately, recent development versions of glibc implement fstat by
calling fstatat, which causes unbounded recursion and stack overflow.
(This depends on the headers present at build time; see the bug for more
details.) This patch switches it to use the fstat (or fstat64 on
32-bit) syscall directly.

Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63d29101d708
Call fstat directly in Linux sandbox fstatat interception. r=gcp
See Also: → 1673771
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Duplicate of this bug: 1673189

The patch landed in nightly and beta is affected.
:jld, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(jld)

Jed, our last beta builds tomorrow, should we uplift this?

(In reply to Pascal Chevrel:pascalc from comment #11)

Jed, our last beta builds tomorrow, should we uplift this?

I don't think we need to; see comment #4. It doesn't affect our builds, only downstream builds using a prerelease version of glibc on the build host, like Fedora Rawhide — and Fedora already took the patch.

But we'll likely want to uplift to ESR 78.

Flags: needinfo?(jld)

Comment on attachment 9183888 [details]
Bug 1673202 - Call fstat directly in Linux sandbox fstatat interception.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Without this patch Firefox 78 ESR might stop working on to-be-released Fedora distributions. (Our own builds would still work, but not distro shipped ones)
  • User impact if declined: Nonfunctional ESR 78 in the future.
  • Fix Landed on Version: 84
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It is risky in the sense that Firefox 84 hasn't rolled out to release yet and we'd likely want to do that before the uplift.

(No distro right now will hit this case)

  • String or UUID changes made by this patch:
Attachment #9183888 - Flags: approval-mozilla-esr78?

Comment on attachment 9183888 [details]
Bug 1673202 - Call fstat directly in Linux sandbox fstatat interception.

Approved for 78.6esr.

Attachment #9183888 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
You need to log in before you can comment on or make changes to this bug.