1710940 - Crash event is not sent in isolated process

Status: ASSIGNED

(GeckoView :: General, defect, P2)

Description

[[:owlish] 🦉 PST]

Affected tests:

• ContentDelegateTest#crashContent
• ContentDelegateTest.#rashContent_tapAfterCrash
• GeckoSessionTestRuleTest#contentCrashIgnored
• ContentCrashTest#crashContent

Comment 1

[Chris Peterson [:cpeterson]]

Moving isolated process bugs to the new GeckoView::Sandboxing component.

Comment 2

[Chris Peterson [:cpeterson]]

107

Comment 3

[Olivia Hall [:olivia]]

This bug is related to issues reading the auxiliary vector at /proc/<crashing-pid>/auxv. Currently tested using the breakpad minidump writer.

The rust minidump writer bug of Improving handling of the auxiliary vector on Linux will likely become related to this bug on the change.

Comment 4

[Chris Peterson [:cpeterson]]

P2, waiting for gsvelto's new rust minidump writer to land.

Comment 5

[Gabriele Svelto [:gsvelto]]

FYI I've briefly investigated what would be needed to generate crash reports from isolated processes and it might require quite a bit of work. Going through Google documentation I discovered that it shouldn't be possible to use ptrace() like we currently do to inspect the child process. Specifically we need a broker process for ptrace()-ing the crashed child. That is the parent cannot ptrace_attach() to the child but must instead have the child fork a broker process that will do the ptrace() calls on its behalf.

The architecture of minidump-writer doesn't currently support this and will require a fairly significant refactoring.

Comment 6

[Olivia Hall [:olivia]]

Hi Gabriele,

We're looking at moving forward with isolated processes on Android.

I did some preliminary testing to see what the current state is of these junit failures. (A lot has changed since last we looked at this a couple of years ago with the rust minidump writer and bug 1620998!)

Here is a try from a couple of days ago running these tests.

In a failing isolated process run we get:

MOZ_CRASH(Crash via about:crashcontent)
...
04-25 14:55:05.682 31343 31343 I Binder:31343_3: type=1400 audit(0.0:3922): avc: denied { signal } for scontext=u:r:untrusted_app:s0:c512,c768 tcontext=u:r:isolated_app:s0:c512,c768 tclass=process permissive=1

// (GeckoCrashHelper never appears in the logs)

In a passing non-isolated process run, we see a lot less avc: denied calls overall, and a stack that looks like this:

MOZ_CRASH(Crash via about:crashcontent)
...
04-24 15:04:07.582 17499 17516 D GeckoCrashHelper: minidump_writer::linux::sections::mappings: retrieving build id for MappingInfo { start_address: 481648033792, size: 24576, system_mapping_info: SystemMappingInfo { start_address: 481648033792, end_address: 481648058368 }, offset: 0, permissions: MMPermissions(READ | WRITE | EXECUTE | PRIVATE), name: Some("/system/bin/app_process64") }

I'm still tracing and understanding how CrashHelper service works.

Could you take a look at this when you have some time?

Comment 7

[Gabriele Svelto [:gsvelto]]

This is a known problem that needed bug 1620998 to be addressed. In short, isolated processes need the ptrace() calls to be proxied, which is something that we don't do yet. Before bug 1620998 we had no way of doing it, but now we should be able to. I need some time to figure this out but first I have to deal with main process crashes so I won't be able to tackle this before 1-2 months.

Comment 8

[Gabriele Svelto [:gsvelto]]

Olivia, is there a list of restrictions placed on isolated processes with regards to system calls? I was trying to design the mechanism that the child processes will use to rendez-vous with the crash helper and let it generate the crash report, and I want to be sure my design does not involve bits that aren't required. Among those things I was wondering if an isolated process would be able to connect() to an AF_UNIX socket. My guess is that it shouldn't be able to, given even our sandbox prevents that unless the destination Unix address is explicitly allowed by the security policy.

Comment 9

[Olivia Hall [:olivia]]

Thanks for looking into this!

is there a list of restrictions placed on isolated processes with regards to system calls?

I'll try to find some documentation with some specific information. I've found this short video helpful for providing some general expectations.

My guess is that it shouldn't be able to, given even our sandbox prevents that unless the destination Unix address is explicitly allowed by the security policy.

Yeah, that seems like a reasonable assumption to me too.

I'll see if I can find out some more information and leave my ni?.

:owlish, do you have some additional resources or thoughts here?

Comment 10

[Gabriele Svelto [:gsvelto]]

A further bit of clarification regarding comment 7 and 8. From the information I could find about isolated processes it seems like we wouldn't be able to call ptrace() directly from another non-isolated process. Right now the crash generation flow involves the crash helper process ptrace()-ing a crashed child process to extract the data that will go into the crash report. If this is not possible then we'd need an IPC channel to have been established between the crash helper and the isolated child process (because we can't connect to an external process from the isolated process). When the child process crashes it should fork() itself, call ptrace() on the forked process and then communicate back the results across the IPC channel. If this is the case then it's gonna be quite a bit of work and a dedicated abstraction in the minidump-writer crate for this particular case.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

is there a list of restrictions placed on isolated processes with regards to system calls?

You want to look into the AOSP source, e.g. the SELinux policy for isolated process sits in there.

Comment 12

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #11)

is there a list of restrictions placed on isolated processes with regards to system calls?

You want to look into the AOSP source, e.g. the SELinux policy for isolated process sits in there.

From what I can tell, the main problem with ptrace is that each isolated process is assigned a unique uid/gid which are different from any regular app or any other isolated service, so ptrace fails the old-fashioned same-uid check (and so do accesses to /proc/%d/{stat,maps,…}, which may cause some other problems outside the scope of this bug; and see also comment #3 about the auxv, I think).

But, I believe that the SELinux policy would also block this, even if there would otherwise be a way for the child process to opt into being ptraced by a different-uid tracer. The only rule I can see that allows an untrusted_app to ptrace applies only if the tracee is also an untrusted_app, not an isolated_app (and the same for isolated_app: it can only ptrace another isolated process). There's also a neverallow rule in the app_domain macro which would cause a policy compilation error if such an allow rule existed.

Comment 13

[Olivia Hall [:olivia]]

Thanks for looking into this bug!

Just adding some more docs to add to the reference pile:

• SELinux Android docs
• Chromium docs that mention isolated processes
• Reference to ptrace and Breakpad in Android code search in the rules (in other API levels as well)

Comment 14

[Gabriele Svelto [:gsvelto]]

It sounds like the only way to ptrace() an isolated child process will be do to something along the lines of what Crashpad does. That is:

• When we hit the exception handler we need to fork the crashed child process, to generate a new child that will ptrace()-eable from it
• The crashed child process ptrace()s the newly forked child and generates a minidump
• The crashed child process finally closes its forked child, informs the parent that it generated a dump and shuts down

There's a few quirks I've deliberately left out from the description above

• We need a pre-established IPC channel between the main process and the child processes, I'm implementing it in bug 1964600
• Isolated processes cannot access the filesystem IIRC, so we either need to write the minidump to memory and pass it over IPC to the main process or we need to pass the child process a file descriptor we've opened in the parent
• The child process wouldn't have access to all the information required for generating a full-fledged crash report, so we'd need to add the missing bits afterwards
• Obviously we wouldn't want to do all this stuff from within the exception handler, it would be madness. So we probably need something else, maybe a parked thread we'd wake up on demand to generate the minidump from a non-crashed context.

Comment 15

[Makoto Kato [:m_kato]]

(In reply to Olivia Hall [:olivia] from comment #13)

Thanks for looking into this bug!

Just adding some more docs to add to the reference pile:

• SELinux Android docs

Isolated mode's SELinux seems to be Permissive on our emulator image. (getenforce returns Permnissive` ) But isolated mode's service process's UID is different for other service process.

Comment 16

[[:owlish] 🦉 PST]

Hi Gabriele, we are working on implementing isolated processes on Android. Would you be able to take on this bug?

Comment 17

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

(In reply to Gabriele Svelto [:gsvelto] from comment #14)

• The crashed child process ptrace()s the newly forked child and generates a minidump

My understanding of crashpad's design is a bit different: the forked child process ptraces the crashed process, and proxies low-level operations: basically reading the process's memory, thread registers, and /proc/{pid}/… files. The minidump generation logic remains in the crash agent, with an abstraction layer over where it would normally call ptrace or access /proc directly.

In particular, this means that the proxy, in the clone of the crashed process, can be fairly minimal and may not even need to malloc (I'm not 100% but I think this is what crashpad is going for; e.g., using getdents instead of readdir). That could be useful in case of heap metadata corruption, or a crash inside malloc, or if an async signal interrupts malloc and the handler crashes, etc.

Maybe you've already come to the same conclusion, but I thought I'd mention it for the historical record at least.

• Obviously we wouldn't want to do all this stuff from within the exception handler, it would be madness.

Not entirely. The signal handler is on the alternative stack so stack exhaustion isn't an issue, and if there were any locks held by the crashing thread then moving to another thread wouldn't help that. The one thing that might matter is if thread-local storage was inconsistent at the time of crash. But, with the approach of using a minimal proxy like what crashpad does, I think the whole thing could be async signal safe (no locks, no TLS).

Comment 18

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

(In reply to Makoto Kato [:m_kato] from comment #15)

Isolated mode's SELinux seems to be Permissive on our emulator image.

It is, and this has caused problems in the past — I'm pretty sure it's why bug 1670277 was missed by CI and only caught by testing on physical devices. It would be nice if CI could leave enforcement turned on, but I assume that's complicated. In the meantime, for patches that do subtle Linux things, I'd suggest testing manually with setenforce 1.

Comment 19

[Gabriele Svelto [:gsvelto]]

(In reply to [:owlish] 🦉 PST from comment #16)

Hi Gabriele, we are working on implementing isolated processes on Android. Would you be able to take on this bug?

I don't have the cycles for it, but I've talked about it to Chris Martin last Friday and he might be able to. He just did a pretty thorough overhaul of the minidump-writer interface so he's got fresh knowledge of the affected code. I'm CC'ing both him and our manager Jens.

Comment 20

[Gabriele Svelto [:gsvelto]]

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

My understanding of crashpad's design is a bit different: the forked child process ptraces the crashed process, and proxies low-level operations: basically reading the process's memory, thread registers, and /proc/{pid}/… files. The minidump generation logic remains in the crash agent, with an abstraction layer over where it would normally call ptrace or access /proc directly.

In particular, this means that the proxy, in the clone of the crashed process, can be fairly minimal and may not even need to malloc (I'm not 100% but I think this is what crashpad is going for; e.g., using getdents instead of readdir). That could be useful in case of heap metadata corruption, or a crash inside malloc, or if an async signal interrupts malloc and the handler crashes, etc.

Maybe you've already come to the same conclusion, but I thought I'd mention it for the historical record at least.

Thanks for the thorough explanation. I hadn't looked at the Crashpad code in a while so I could use a refresher.

Not entirely. The signal handler is on the alternative stack so stack exhaustion isn't an issue, and if there were any locks held by the crashing thread then moving to another thread wouldn't help that. The one thing that might matter is if thread-local storage was inconsistent at the time of crash. But, with the approach of using a minimal proxy like what crashpad does, I think the whole thing could be async signal safe (no locks, no TLS).

Right, I hadn't thought about the possibility of the crashed threads holding locks. The memory allocation angle is non-trivial. If the crashed process got stuck in the memory allocator then the forked process wouldn't be able to do any allocations, at least not via the regular allocator. Breakpad worked around that problem by having a "leaking" allocator that it could use from within those contexts, but it's a very intrusive change to the minidump generation. I need to ponder about this a little bit more.

Comment 21

[Chris Martin [:cmartin]]

Attachment: Bug 1710940 - Part 1: Fix minidump-writer in isolated processes

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

FYI to verify that it will work with content processes you can as well use the following wdspec test:
https://searchfox.org/firefox-main/source/testing/web-platform/mozilla/tests/webdriver/harness/detect_crash.py

Please note that via bug 2011830 I'm going to mark this test as expected fail for now. You should update the relevant wpt meta data file and remove the entry before pushing the patch to autoland.