Christoph Kerschbaumer [:ckerschb] Reporter
	Description • 11 years ago

Individually removing each single one of these syscalls leads to the same stacktrace (see attachment). It seems that remoting the call to FcInitBringUptoDate() [1] allows us to remove all of these syscalls from the whitelist. (Unless they are used somewhere else, but most likely they are just here.)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontconfigUtils.cpp#568

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 1 • 11 years ago

On a different machine, I do get a different stacktrace. It still seems that all these syscalls are closely tied to widgets with the same root to be TabChild::Init [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#677

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 2 • 11 years ago

It seems we can eliminate the majority of syscalls from the seccomp whitelist if we defer the call to |SetCurrentProcessSandbox|. The reason why this patch works is because all the syscalls happen during initialization (startup), e.g. nsXPLookAndFeel is a singleton and only created once [1]. Only when creating a new LookAndFeelObject, we have to perform so many syscalls (follow [2,3]).
We can use the flag |mSyscallFilter| so we do not install the same whitelist whenever we create a new tab.
What do you think :kang, i think your idea might work, even if we do not have a separate process per tab? How would that patch affect B2G?

[1] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsXPLookAndFeel.cpp#243
[2] http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#49
[3] http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#1079

	Guillaume Destuynder [:kang] [this account is no longer active]
	Comment 3 • 11 years ago

Comment on attachment 8338850 [details] [diff] [review]
move_setcurrentprocesssandbox.patch

as discussed on irc this sounds very cool
we can't land this as is of course as it impacts b2g.. im testing that asap on b2g.

	Guillaume Destuynder [:kang] [this account is no longer active]
	Comment 4 • 11 years ago

just tested on B2G, this seems to work.

that would also solve bug 880808.
and it would also let us revert bug 921817. (i also did a quick test, when reverted, browser tabs have seccomp set)

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 8 • 11 years ago

Comment on attachment 8342171 [details] [diff] [review]
bug_942698_move_setcurrentprocesssandbox.patch

It seems that a thread is running at the same time as the sandbox init code which leads to a race condition. Occasionally the child performs a syscall not on the whitelist. This means that the new place to call SetCurrentProcessSandbox is not suitable. We have to somehow defer the tab init, till all threads have completed init, similar to what NUWA does since it needs the same checkpoints.
Maybe there is a way to do this properly in Gecko's code that we are not aware of.
Canceling review for now.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 9 • 11 years ago

I tried different locations which might be better suited to init the sandbox, but even the most promising spot in ContentChild::RecvPBrowserConstructor [1] has the problem that threads are still running. I put a break at [1] and generated the subsequent dump of all threads. Is there a possibility to make sure that all threads are done with their initialization?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#680

(gdb) info threads
  Id   Target Id         Frame 
  11   Thread 0x7fffe71ff700 (LWP 32063) "Chrome_ChildThr" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:39
  10   Thread 0x7fffe56a2700 (LWP 32064) "dconf worker" 0x00007fffeeb77a43 in __GI___poll (fds=<optimized out>, 
    nfds=<optimized out>, timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:87
  9    Thread 0x7fffe4ea1700 (LWP 32065) "gdbus" 0x00007fffeeb77a43 in __GI___poll (fds=<optimized out>, 
    nfds=<optimized out>, timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:87
* 8    Thread 0x7fffe2cff700 (LWP 32066) "JS GC Helper" pthread_cond_wait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:162
  7    Thread 0x7fffe1eff700 (LWP 32067) "JS Watchdog" pthread_cond_wait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:162
  6    Thread 0x7fffe16fe700 (LWP 32068) "Socket Thread" 0x00007fffeeb77a43 in __GI___poll (fds=<optimized out>, 
    nfds=<optimized out>, timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:87
  5    Thread 0x7fffdb8c1700 (LWP 32069) "threaded-ml" 0x00007fffeeb77a43 in __GI___poll (fds=<optimized out>, 
    nfds=<optimized out>, timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:87
  4    Thread 0x7fffdb0c0700 (LWP 32070) "BgHangManager" pthread_cond_wait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:162
  3    Thread 0x7fffda5a1700 (LWP 32071) "Timer" pthread_cond_timedwait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S:215
  2    Thread 0x7fffd9753700 (LWP 32072) "pool" pthread_cond_timedwait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S:215
  1    Thread 0x7ffff7fbf9c0 (LWP 32062) "Browser" 0x00007fffeeb4f08d in nanosleep ()
    at ../sysdeps/unix/syscall-template.S:82

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 10 • 11 years ago

Benjamin,

Bill just told me that you might know a good spot where we can init the sandbox where all threads are done with their initialization. Do you even think there is a sport where we won't end up having race conditions?

	Benjamin Smedberg
	Comment 11 • 11 years ago

I don't understand the question. The browser creates threads at runtime, so there is a never a point where all threads have already been created.

Do you perhaps mean a point before *any* threads except the main thread are created? Why wouldn't you just do this in main()?

	Guillaume Destuynder [:kang] [this account is no longer active]
	Comment 12 • 10 years ago

the sandbox can't be initialized before *any* thread is created, as many threads will do various resource access (in particular, filesystem access) during their initialization (as in, after the thread is created).

:ckerschb is looking for a place to initialize the sandbox after most of those threads are initialized - in B2G that's currently either after Nuwa forks or during RecvSetCurrentProcessSandbox.
More threads may be initialized after that, but most of those do very little access to the filesystem or other resources (and eventually, won't do any).

The point being that a sandbox is only a mechanism to control resource accesses, if the program itself requires dangerous accesses, those need to be moved or avoided (by initializing the sandbox later, for example, as long as it doesn't get untrusted input such starting to parse html/js/etc documents) in some way.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 13 • 10 years ago

I am working full time on contentPolicies and haven't worked on sandboxing in months. Cleaning out my assigned bugs at the moment. Guillaume, do you wanna take this one?

	Guillaume Destuynder [:kang] [this account is no longer active]
	Comment 14 • 10 years ago

Jed is now the module owner for B2G and Linux sandbox - also I think he fixed most of, or all the above

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 15 • 10 years ago

The list in the previous subject isn't quite right: calls that operate on existing capabilities, like sendmsg/recvmsg, are usually[*] okay; it's the ones that operate on other namespaces (files, network addresses, processes) that we need to move away from.

[*] Usually.  The fancier and less well-tested they are, the more likely they are to have their own vulnerabilities; e.g., vmsplice(2) vs. write(2).

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 16 • 10 years ago

Move process sandboxing bugs to the new Bugzilla component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 17 • 7 years ago

Re-flagging this for triage, because I'm not sure how much sense it makes anymore — filesystem access is brokered and I don't think we have any plans to not have a broker in some form, and networking already has its own sub-meta-bug.