Closed Bug 980924 Opened 7 years ago Closed 4 years ago

Call to open() in libgenlock conflicts with sandbox

Categories

(Core :: Security: Process Sandboxing, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: arroway, Assigned: arroway)

References

Details

(Whiteboard: sb+)

Attachments

(4 files, 2 obsolete files)

When rendering layout in a sandboxed content process, function genlock_attach_lock calls open() on /dev/genlock with flag O_RDWR.
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/.
Attachment #8401423 - Flags: feedback?(jld)
Attachment #8401423 - Flags: feedback?(gdestuynder)
Attachment #8401421 - Flags: feedback?(jld)
Attachment #8401421 - Flags: feedback?(gdestuynder)
Assignee: nobody → stephouillon
(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?
Right, I guess we could always ban the call to mprotect() on this particular address...
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.
Attachment #8401423 - Attachment is obsolete: true
Attachment #8401424 - Attachment is obsolete: true
Attachment #8401423 - Flags: feedback?(gdestuynder)
And maybe I should change the 0xdeadc0de marker for something like 0x00000bad to avoid unexpected stuff to happen.
do you have some test data on the performance difference when the check is done in the parent?
Move process sandboxing bugs to the new Bugzilla component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)
Component: Security → Security: Process Sandboxing
Sorry for the bugspam; filter on 086f2ac3-ac15-4299-889b-009181af5029.
Blocks: 1121295
Sorry for the bugspam; filter on 086f2ac3-ac15-4299-889b-009181af5029.
No longer blocks: 930258
Whiteboard: sb+
Status: NEW → RESOLVED
Closed: 4 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.