Opened 2 years ago

Closed 7 months ago

Last modified 7 months ago

#20283 closed defect (fixed)

Tor Browser should run without a `/proc` filesystem.

Reported by: yawning Owned by: pospeselr
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-sandboxing, TorBrowserTeam201804R
Cc: gk, intrigeri, mcs, tbb-team Actual Points:
Parent ID: #20773 Points:
Reviewer: Sponsor: Sponsor4

Description

Currently Tor Browser crashes immediately on startup if a proc filesystem is not mounted on /proc. This also affects the upstream firefox code, so it technically is a Mozilla bug.

too much recursion
Segmentation fault (core dumped)

/proc contains a large amount of information about the host system that can be used to fingerprint/identify users and additionally historically has been the source or part of many kernel security problems.

While this problem can be mitigated by a MAC system (eg: AppArmor) to constrain what Firefox can access under /proc, the ideal fix is for Firefox to support running without /proc, while degrading gracefully (there is no truly ubiquitous MAC system available on all common Linux distributions by default, and the problem is severe enough that it should be resolved correctly).

Child Tickets

Attachments (1)

0001-Bug-20283-Tor-Browser-should-run-without-a-proc-file.patch (5.6 KB) - added by pospeselr 7 months ago.
js::GetNativeStackBaseImpl fix

Download all attachments as: .zip

Change History (27)

comment:1 Changed 2 years ago by gk

Cc: gk added

Do you know whether there is already an upstream bug for this?

comment:2 Changed 2 years ago by yawning

AFAIK no, but I will admit that I didn't look very hard through bugzilla. It's probably a fairly uncommon use case, so I'm not sure how much upstream will care about it.

comment:3 Changed 2 years ago by yawning

Parent ID: #20773

comment:4 Changed 23 months ago by intrigeri

Cc: intrigeri added

comment:5 Changed 17 months ago by yawning

I worked around this in sandboxed-tor-browser so I don't care about this anymore.

comment:7 Changed 8 months ago by pospeselr

Owner: changed from tbb-team to pospeselr
Status: newassigned

comment:8 Changed 8 months ago by mcs

Cc: mcs added

comment:9 Changed 8 months ago by yawning

There are at least two issues that I know of that prevent running Firefox without /proc mounted.

The first is that Firefox uses /proc/self/task to see if it spawned any threads. The warning can be ignored on any kernel that supports SECCOMP_FILTER_FLAG_TSYNC (>= 3.17), but may result in "bad" if the kernel is old, and no, I do not remember what the bad is.

The second is that Firefox will crash with too much recursion if /proc is not mounted. The culprit there is that Firefox will query the stack size with pthread_attr_getstack() which will return a stack size of 0, if /proc is not mounted for the default thread (tid == pid).

Note that there may be other horrific things that happen, or other things that break without /proc, but I was not able to find any at the time that I cared about this. Finding and debugging such things is left as an exercise for the student. Fixing this properly probably requires upstream to care about this use case.

comment:10 Changed 8 months ago by jld

If SECCOMP_FILTER_FLAG_TSYNC isn't available and /proc/self/task can't be listed, the sandbox can't start. The process is already multithreaded, so we have to signal all the threads to tell them to apply seccomp, and we don't have access to the libc's internal list of threads (or the lock protecting it) so we have to ask the kernel via procfs.

The single-threadedness check, however, has been removed in Firefox 60, as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1401062.

comment:11 Changed 8 months ago by boklm

Cc: tbb-team added

comment:12 Changed 8 months ago by yawning

Note that there may be other horrific things that happen

The content process will SIGSEGV on JS heavy pages if pthread_attr_getstack() returns a NULL stackaddr.

comment:13 Changed 8 months ago by pospeselr

I've identified all (hopefully) the callers in the parent firefox and the child plugin_container (nothing unique in plugin_container though that isn't called in the parent). The spawned glxtest also has lots of reads going to /proc as well.

For patch verification, what's the procedure for hiding /proc from a process in general?

comment:14 in reply to:  10 Changed 8 months ago by gk

Replying to jld:

If SECCOMP_FILTER_FLAG_TSYNC isn't available and /proc/self/task can't be listed, the sandbox can't start. The process is already multithreaded, so we have to signal all the threads to tell them to apply seccomp, and we don't have access to the libc's internal list of threads (or the lock protecting it) so we have to ask the kernel via procfs.

The single-threadedness check, however, has been removed in Firefox 60, as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1401062.

That's actually #23915 and we should be good with that.

Last edited 8 months ago by gk (previous) (diff)

comment:15 in reply to:  13 Changed 8 months ago by gk

Replying to pospeselr:

I've identified all (hopefully) the callers in the parent firefox and the child plugin_container (nothing unique in plugin_container though that isn't called in the parent). The spawned glxtest also has lots of reads going to /proc as well.

For patch verification, what's the procedure for hiding /proc from a process in general?

I don't know but you could mess with Yawning's sandboxed-tor-browser (which brought this issue up in the first place), see: #20773 and the patch for it in 95857360ec7f84cf9f0a01855c15881c89919133.

comment:16 Changed 7 months ago by cypherpunks

From conversation with pospeselr:

I suspect that glibc relies on /proc only for the initial thread. It is possible that for the initial thread you can get the stack base address and size using getcontext(2). In particular, if pthread_getattr_np fails or pthread_attr_getstack returns null, and the thread in question is the process's initial thread, then it is possible that

ucontext_t uc;
void *base;
size_t size;

if (getcontext(&uc) == -1)
	err(1, "getcontext");
base = uc.uc_stack.ss_sp;
size = uc.uc_stack.ss_size;

will recover the stack base address and size that you want.

comment:17 in reply to:  16 Changed 7 months ago by yawning

Replying to cypherpunks:

I suspect that glibc relies on /proc only for the initial thread.

Yes.

it is possible that

ucontext_t uc;
void *base;
size_t size;

if (getcontext(&uc) == -1)
	err(1, "getcontext");
base = uc.uc_stack.ss_sp;
size = uc.uc_stack.ss_size;

will recover the stack base address and size that you want.

99% sure this does not work without /proc mounted.

comment:18 Changed 7 months ago by pospeselr

As mentioned by cypherpunks, the issue here only occurs on the main thread which you can verify with a little spelunking through the glibc source. The relevant query APIs work expected on non-main threads since pthreads populates the required info in memory.

Verifying a patch now on TreeHerder that fixes this issue by reading the __libc_stack_end symbol (glibc sets this void* to the first stack frame during program init). Solution suggested by mozilla's jld, and the folks in #jsapi (those paying attention at least) didn't seem offended at the idea of doing so for the main thread.

Last edited 7 months ago by pospeselr (previous) (diff)

comment:19 Changed 7 months ago by gk

Keywords: TorBrowserTeam201804 added
Priority: MediumVery High

Changed 7 months ago by pospeselr

js::GetNativeStackBaseImpl fix

comment:20 Changed 7 months ago by pospeselr

Status: assignedneeds_review

comment:21 Changed 7 months ago by gk

Keywords: TorBrowserTeam201804R added; TorBrowserTeam201804 removed

comment:22 Changed 7 months ago by pospeselr

Audited all the remaining syscalls (was only catch'ing open and openat before) and discovered three more things which don't seem to be an issue for us. Here's the full list of what we're working with (on x64 Linux) roughly in order of invocation for future reference (open here covers both open and openat, didn't save that data unfortunately):

Caller syscall Requested Path Relevant Source
mozilla::HasUserNamespaceSupport() access "/proc/self/ns/user" /security/sandbox/linux/SandboxInfo.cpp
Failure here 'just' disables 'User Namespaces' (as shown under the Sandbox section of about:support), not sure what the security consequences of this is but tor browser boots and runs fine
selinuxfs_exists() open "/proc/filessytems"
This call happens down in dlopen when opening libxul.so. Failure to open this file will result in selinxufs_exists() always reporting selinuxfs exists ( see selinuxfs_exists(void) but everything seems to go along even if it is a false positive)
mozilla::(anonymous namespace)::IsSingleThreaded () xstat "/proc/self/task" /security/sandbox/linux/SandboxInfo.cpp
Failure to read and count number of files under this directory causes tor browser to guess and indicate that it is not single threaded
mozilla::ComputeProcessUptimeThread(void*) open "/proc/self/task/$tid/stat", "/proc/self/stat" /mozglue/misc/TimeStamp_posix.cpp
This code fails gracefully and sets uptime to 0
nsNotifyAddrListener::calculateNetworkId() open "/proc/net/route", "/proc/net/arp" /netwerk/system/linux/nsNotifyAddrListener_Linux.cpp
This code fails gracefully, network id is only used once for telemetry, otherwise this is dead code
js::GetNativeStackBaseImpl() open "/proc/self/maps" /js/src/jsnativestack.cpp
This code gets an address at the top of the usable stack, required for internal JS stuff or else bad things happen. Attached patch fixes this issue using __libc_stack_end
mozilla::ThreadStackHelper::GetThreadStackBase() open "/proc/self/maps" /xpcom/threads/ThreadStackHelper.cpp
Similar to js::GetNativeStackBaseImpl() except the implementation isn't there for all platforms and the saved value isn't used for anything, fails gracefully
nsSystemInfo::Init() open "/proc/cpuino" /xpcom/base/nsSystemInfo.cpp
Fails gracefully, otherwise scrapes all your CPU info and stores it in the nsISystemInfo property bag for god knows what reasons
gfxFcPlatformFontList::FindGenericFamilies() readlink "/proc/self/exe" /gfx/thebes/gfxFcPlatformFontList.cpp
Fails gracefully when intercepting this syscall and returning error (-1)
nsLocalFile::GetDiskSpaceAvailable() open "/proc/self/mountinfo" /xpcom/io/nsLocalFileUnix.cpp
Fails gracefully when given file isn't found and a simpler estimate is used instead

comment:23 Changed 7 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks for writing all those cases up in comment:22.

So, I think we could just avoid integrating the latest jsnativestack.cpp changes into esr52, given that the those did not get backported and thus might carry breakage risks. E.g. the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1423287 makes me sightly nervous due to touching the GNUC part, but my testing seems to be fine with it. (No, the try result done by dmajor does not change my nervousness factor as they very likely use a different toolchain (different GCC and binutils) versions than we)

However, that's for an alpha release and I think taking the patch especially after the Mozilla review is fine for that. Therefore: Applied to tor-browser-52.7.3esr-8.0-1 (as commit 90e16dd25b6eb199c016fc8b5e9478a3454d36e1) to get it into 8.0a6.

Nice work!

comment:24 Changed 7 months ago by tom

Is this something we should try to uplift?

comment:25 in reply to:  24 Changed 7 months ago by gk

Replying to tom:

Is this something we should try to uplift?

It already has r+ in https://bugzilla.mozilla.org/show_bug.cgi?id=859782. Sure, if we could get this into beta, great! But I am not so optimistic.

comment:26 Changed 7 months ago by gk

Sponsor: Sponsor4
Note: See TracTickets for help on using tickets.