980924 - Call to open() in libgenlock conflicts with sandbox

Status: RESOLVED WONTFIX

(Core :: Security: Process Sandboxing, defect)

Description

[Stephanie Ouillon [:arroway] (needinfo me)]

When rendering layout in a sandboxed content process, function genlock_attach_lock calls open() on /dev/genlock with flag O_RDWR.

Comment 1

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: bt-genlock_attach_lock.txt

Comment 2

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: 1-libgenlock.patch

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: 2-Update_sandbox_BPF_rules.patch

Comment 4

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: 3-Pseudo_testing_open_genlock.patch

Comment 5

[Stephanie Ouillon [:arroway] (needinfo me)]

Remoting those calls through IPDL impacts performance in a significant way since they're made for layout rendering. So I considered whitelisting these calls in the BPF rules.

I declared a new macro ALLOW_GENLOCK_OPEN_SYSCALL in the BFP filter. In seccomp-bpf we can only access the address of the pathname argument of the syscall. But the value of the string of the file requested to be open from the libgenlock is always the same ("/dev/genlock"). So if we ensure the value is always the same for a given address, we can filter open syscalls based on the address of the pathname argument.

In 1-libgenlock.patch, I modified the qcom libgenlock to declare the pathname string of the requested file in the .rodata section.
Then there's a trick since we can't know this address beforehand and hardcode it in the macro. So when enabling sandboxing for each process, the libgenlock is loaded, then the address is retrieved and injected in the ALLOW_GENLOCK_OPEN_SYSCALL in place of a 0xdeadc0de marker. 

3-Pseudo_testing_open_genlock.patch is a hack for testing the rule is working, since the homescreen crashes because of other seccomp violations if ALLOW_SYSCALL(open) is removed.

Note this patch requires to rebuild the libgenlock and push it to /system/lib/.

Comment 6

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

(In reply to Stéphanie Ouillon [:arroway] from comment #5)
> In 1-libgenlock.patch, I modified the qcom libgenlock to declare the
> pathname string of the requested file in the .rodata section.

If the content process is compromised, what prevents it from using mprotect() to make the page(s) containing the string writable (or replacing them with a new writable mapping) and writing a different string at the same address?

Comment 7

[Stephanie Ouillon [:arroway] (needinfo me)]

Right, I guess we could always ban the call to mprotect() on this particular address...

Comment 8

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: 2-Update_sandbox_BPF_rules.patch

So it took more time and more BFP rules than I expected but here it is the fix to prevent mprotect() from accessing the genlock_file address with the PROT_WRITE flag. 
One interesting thing I didn't see before : libgenlock.so is not loaded in the process at the time it is sandboxed. So I dlopen() it and never close it, so that next time the linker loads the library it occupies the same address space.

Comment 9

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: 3-Pseudo_testing_open_genlock.patch

Comment 10

[Stephanie Ouillon [:arroway] (needinfo me)]

And maybe I should change the 0xdeadc0de marker for something like 0x00000bad to avoid unexpected stuff to happen.

Comment 11

[Guillaume Destuynder [:kang] [this account is no longer active]]

do you have some test data on the performance difference when the check is done in the parent?

Comment 12

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

Move process sandboxing bugs to the new Bugzilla component.

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

Comment 13

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

Sorry for the bugspam; filter on 086f2ac3-ac15-4299-889b-009181af5029.

Comment 14

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

Sorry for the bugspam; filter on 086f2ac3-ac15-4299-889b-009181af5029.