1660901 - Crash in [@ __fxstatat]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Crash report: https://crash-stats.mozilla.org/report/index/b6508761-63bc-471d-a88a-2bf7e0200821

Top 10 frames of crashing thread:

0 libc.so.6 __fxstatat /usr/src/debug/glibc-2.32/io/../sysdeps/unix/sysv/linux/wordsize-64/fxstatat.c:43
1 libc.so.6 statx_generic /usr/src/debug/glibc-2.32/io/./statx_generic.c:60
2 libgio-2.0.so.0 libgio-2.0.so.0@0x13ba9f 
3 firefox-bin arena_t::MallocSmall memory/build/mozjemalloc.cpp:2864
4 libglib-2.0.so.0 libglib-2.0.so.0@0x5957c 
5 libgobject-2.0.so.0 libgobject-2.0.so.0@0x19256 
6 libgobject-2.0.so.0 libgobject-2.0.so.0@0x5ab8f 
7 libgobject-2.0.so.0 libgobject-2.0.so.0@0x5ab8f 
8 libgobject-2.0.so.0 libgobject-2.0.so.0@0x190af 
9 libgobject-2.0.so.0 libgobject-2.0.so.0@0x5a33f 

Found this during nightly crash triage and it prompted me to scrape symbols for the testing packages of Fedora 33. Unfortunately I don't have all the symbols yet as seen in the stack trace above but this seems caused by a new glibc calling newfsstatat() (syscall 262).

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

I found a fully symbolicated crash and if you look at it you'll notice that I really need to fix bug 1648692 in order to have better traces on Fedora :-(

Comment 2

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

We support the case of fstatat that's similar to stat/lstat, where dirfd == AT_FDCWD. This is not that. But notice that the flags contain the Linux extension AT_EMPTY_PATH, which allows passing an empty string as the path, for behavior like fstat (including if dirfd isn't a directory).

The path is at 0x00007fcd0ca3e177, which is offset 0x16e177 in libgio-2.0.so.0, so if the byte at that address is 0, then this is a case that could be supported (but currently isn't). The RPMs I've found didn't match the build ID we have, so I can't be sure, but this info (AT_EMPTY_PATH and a constant path in libgio) points to this glib commit, which is the fstat-like case, as the likely cause.

Note that the path isn't required to be the empty string, so we can't pass this through in seccomp-bpf; it would have to be emulated with SECCOMP_RET_TRAP. (Or, after bug 1603307, we'd need to extract a copy of the fd, call fstat on it in the parent process, and write the struct stat back into the child's address space.)

Comment 3

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

Attachment: Bug 1660901 - Support the fstat-like subset of fstatat in the Linux sandbox policies.

Comment 4

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

Attachment: Bug 1660901 - Add some test cases for fstatat inside the content sandbox.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80baa04419c4
Support the fstat-like subset of fstatat in the Linux sandbox policies. r=gcp
https://hg.mozilla.org/integration/autoland/rev/56f6da03c7e3
Add some test cases for fstatat inside the content sandbox. r=gcp

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/80baa04419c4
https://hg.mozilla.org/mozilla-central/rev/56f6da03c7e3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting eventually or can it ride the trains to release?

Comment 8

[Julien Cristau [:jcristau]]

Looks like we're still seeing crashes e.g. bp-315269cb-bb57-4158-b9be-3b89c0200902 is on nightly 20200901094542 which should have had these patches.

Comment 9

[Julien Cristau [:jcristau]]

AFAICT in this case it's calling g_local_file_lstat which doesn't use AT_EMPTY_PATH

Comment 10

[Julien Cristau [:jcristau]]

Attachment: Bug 1660901 - ignore AT_NO_AUTOMOUNT in fstatat system call. r?jld

Per the manpage "Both stat() and lstat() act as though AT_NO_AUTOMOUNT
was set.", so don't bail if it's set in a call to fstatat.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1d7a6be184b
ignore AT_NO_AUTOMOUNT in fstatat system call. r=jld

Comment 12

[Razvan Maries]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

AFAICT, this only crashes on Nightly builds and doesn't need uplifting. Feel free to nominate approval if I'm misunderstanding something, though.

Comment 14

[Julien Cristau [:jcristau]]

I don't know how glib reacts to statx returning an error, we may want to uplift to esr78 to avoid finding out when glib 2.66 is released?

Comment 15

[Chih-Hsuan Yen [:yan12125]]

glib 2.66 is out and it hits Firefox 80.0.1 with many warnings

Sandbox: seccomp sandbox violation: pid 301342, tid 301342, syscall 262, args 33 140385171460495 140731898391248 4096 4096 1.
Sandbox: unsupported fd-relative fstatat(33, "", 0x7FFEB2CFEAD0, 4096)

The PID is from a content process, and none of content processes crashes due to this error. I feel it annoying, so I rebuild Firefox with changesets 80baa04419c4 and b1d7a6be184b, and there are no such warnings anymore.

I'm using Firefox 80.0.1 on Arch Linux.

There is another report with similar warnings from ESR 78.2.0 on Debian sid [1]. I guess it is also related to glib 2.66.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=970196

Comment 16

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Is this something we should consider backporting eventually or can it ride the trains to release?

We should uplift these patches: we may not immediately crash on release, but we're still failing the syscall and the caller probably won't have a fallback (these flags have existed since Linux 2.6.39), and this could cause problems.

The fstat case seen in the original reports is probably harmless — it's checking if a file is a directory when preparing to read from it, and it fails open which in context (GTK stylesheets?) is probably safe. But other libraries could also try to use it. And the case from comment #8 fixed in the followup patch is worse: that's lstat-equivalent, and from the call stack it looks like we could fail to load GTK theme / UI settings information as a result.

I'll write an uplift request.

Comment 17

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

Comment on attachment 9172528 [details]
Bug 1660901 - Support the fstat-like subset of fstatat in the Linux sandbox policies.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: These patches fix the sandbox policy to handle changes in a new version of glib, which Linux distributions are beginning to ship (alongside older Firefox).
• User impact if declined: This can cause anything using glib, in particular GTK, to see files as not existing (D89121), or to fail to correctly determine information about opened files (D88499). In the case of GTK this can affect reading theme and UI settings information. There may be other impacts we're not aware of yet.
• Fix Landed on Version: 82
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These patches add handling for system calls which were previously denied, and don't affect cases that were previously allowed. (More specifically: D89121 simply ignores setting a flag that we previously de facto treated as set, and D88499 is a simple rearrangement of function arguments to substitute a syscall that we do allow. There's relatively little subtlety in these patches.) D88500 has simple regression tests, although they aren't exhaustive.
• String or UUID changes made by this patch: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172528 [details]
Bug 1660901 - Support the fstat-like subset of fstatat in the Linux sandbox policies.

Approved for 78.4esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/a65fc6aca1f2
https://hg.mozilla.org/releases/mozilla-esr78/rev/0ce38d3c2aa2
https://hg.mozilla.org/releases/mozilla-esr78/rev/8ecb82a2f65c

Comment 21

[gwarser]

Any build before this landed is now crashing when bisecting. Any tips how to run mozregression now?

I'm on Manjaro KDE.

Comment 22

[gwarser]

--pref "security.sandbox.content.level:0" worked for me.