From 095fbea56341cc17a7b2b2936dca470c89709f83 Mon Sep 17 00:00:00 2001
From: Nick Kralevich <nnk@google.com>
Date: Thu, 13 Sep 2018 11:07:14 -0700
Subject: [PATCH] Strengthen ptrace neverallow rules

Add additional compile time constraints on the ability to ptrace various
sensitive domains.

llkd: remove some domains which llkd should never ptrace, even on
debuggable builds, such as kernel threads and init.

crash_dump neverallows: Remove the ptrace neverallow checks because
it duplicates other neverallow assertions spread throughout the policy.

Test: policy compiles and device boots
Change-Id: Ia4240d1ce7143b983bb048e046bb4729d0af5a6e
---
 private/bpfloader.te  |  9 +++++++--
 private/crash_dump.te |  9 ++++++++-
 private/llkd.te       |  3 +++
 public/domain.te      |  3 ---
 public/init.te        |  3 +++
 public/kernel.te      | 31 +++++++++++++++++--------------
 public/ueventd.te     |  3 +++
 public/vendor_init.te |  3 +++
 8 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/private/bpfloader.te b/private/bpfloader.te
index bcfbf39f5..0b3381177 100644
--- a/private/bpfloader.te
+++ b/private/bpfloader.te
@@ -19,7 +19,11 @@ allow bpfloader netd:fd use;
 allow bpfloader netd:bpf { map_read map_write };
 allow bpfloader self:bpf { prog_load prog_run };
 
-# Neverallow rules
+dontaudit bpfloader self:global_capability_class_set sys_admin;
+
+###
+### Neverallow rules
+###
 neverallow { domain -bpfloader } *:bpf prog_load;
 neverallow { domain -bpfloader -netd -netutils_wrapper} *:bpf prog_run;
 neverallow { domain -netd -bpfloader } bpfloader_exec:file { execute execute_no_trans };
@@ -27,4 +31,5 @@ neverallow bpfloader domain:{ tcp_socket udp_socket rawip_socket } *;
 # only system_server, netd and bpfloader can read/write the bpf maps
 neverallow { domain -system_server -netd -bpfloader} netd:bpf { map_read map_write };
 
-dontaudit bpfloader self:global_capability_class_set sys_admin;
+# No domain should be allowed to ptrace bpfloader
+neverallow { domain userdebug_or_eng(`-llkd') } bpfloader:process ptrace;
diff --git a/private/crash_dump.te b/private/crash_dump.te
index aabff29f3..831ff04a7 100644
--- a/private/crash_dump.te
+++ b/private/crash_dump.te
@@ -17,6 +17,13 @@ userdebug_or_eng(`
   allow crash_dump { llkd logd }:process { ptrace signal sigchld sigstop sigkill };
 ')
 
+###
+### neverallow assertions
+###
+
+# ptrace neverallow assertions are spread throughout the other policy
+# files, so we avoid adding redundant assertions here
+
 neverallow crash_dump {
   bpfloader
   init
@@ -29,6 +36,6 @@ neverallow crash_dump {
   ueventd
   vendor_init
   vold
-}:process { ptrace signal sigstop sigkill };
+}:process { signal sigstop sigkill };
 
 neverallow crash_dump self:process ptrace;
diff --git a/private/llkd.te b/private/llkd.te
index 73e3f5818..900d403f2 100644
--- a/private/llkd.te
+++ b/private/llkd.te
@@ -22,9 +22,12 @@ allow llkd domain:process sigkill;
 userdebug_or_eng(`
   allow llkd {
     domain
+    -kernel
     -keystore
     -init
     -llkd
+    -ueventd
+    -vendor_init
   }:process ptrace;
 ')
 
diff --git a/public/domain.te b/public/domain.te
index 412c93d7d..08f7e3ee0 100644
--- a/public/domain.te
+++ b/public/domain.te
@@ -394,9 +394,6 @@ neverallow { domain -init } usermodehelper:file { append write };
 neverallow { domain -init -ueventd } sysfs_usermodehelper:file { append write };
 neverallow { domain -init -vendor_init } proc_security:file { append open read write };
 
-# No domain should be allowed to ptrace init.
-neverallow * init:process ptrace;
-
 # Nobody is allowed to make binder calls into init.
 # Only servicemanager may transfer binder references to init
 # vendor_init shouldn't use binder at all.
diff --git a/public/init.te b/public/init.te
index d06219503..d898603c8 100644
--- a/public/init.te
+++ b/public/init.te
@@ -541,3 +541,6 @@ neverallow init shell_data_file:dir { write add_name remove_name };
 
 # Init should not access sysfs node that are not explicitly labeled.
 neverallow init sysfs:file { open read write };
+
+# No domain should be allowed to ptrace init.
+neverallow * init:process ptrace;
diff --git a/public/kernel.te b/public/kernel.te
index af02c7e49..3a440ebb6 100644
--- a/public/kernel.te
+++ b/public/kernel.te
@@ -81,6 +81,21 @@ allow kernel media_rw_data_file:file create_file_perms;
 # Access to /data/misc/vold/virtual_disk.
 allow kernel vold_data_file:file read;
 
+# Allow the first-stage init (which is running in the kernel domain) to execute the
+# dynamic linker when it re-executes /init to switch into the second stage.
+# Until Linux 4.8, the program interpreter (dynamic linker in this case) is executed
+# before the domain is switched to the target domain. So, we need to allow the kernel
+# domain (the source domain) to execute the dynamic linker (system_file type).
+# TODO(b/110147943) remove these allow rules when we no longer need to support Linux
+# kernel older than 4.8.
+allow kernel system_file:file execute;
+# The label for the dynamic linker is rootfs in the recovery partition. This is because
+# the recovery partition which is rootfs does not support xattr and thus labeling can't be
+# done at build-time. All files are by default labeled as rootfs upon booting.
+recovery_only(`
+  allow kernel rootfs:file execute;
+')
+
 ###
 ### neverallow rules
 ###
@@ -104,17 +119,5 @@ neverallow kernel *:file { entrypoint execute_no_trans };
 # on files being accessed.
 neverallow kernel self:global_capability_class_set { dac_override dac_read_search };
 
-# Allow the first-stage init (which is running in the kernel domain) to execute the
-# dynamic linker when it re-executes /init to switch into the second stage.
-# Until Linux 4.8, the program interpreter (dynamic linker in this case) is executed
-# before the domain is switched to the target domain. So, we need to allow the kernel
-# domain (the source domain) to execute the dynamic linker (system_file type).
-# TODO(b/110147943) remove these allow rules when we no longer need to support Linux
-# kernel older than 4.8.
-allow kernel system_file:file execute;
-# The label for the dynamic linker is rootfs in the recovery partition. This is because
-# the recovery partition which is rootfs does not support xattr and thus labeling can't be
-# done at build-time. All files are by default labeled as rootfs upon booting.
-recovery_only(`
-  allow kernel rootfs:file execute;
-')
+# Nobody should be ptracing kernel threads
+neverallow * kernel:process ptrace;
diff --git a/public/ueventd.te b/public/ueventd.te
index 4f68318fb..dfd4f2ca1 100644
--- a/public/ueventd.te
+++ b/public/ueventd.te
@@ -70,3 +70,6 @@ neverallow ueventd dev_type:blk_file ~{ getattr relabelfrom relabelto create set
 
 # Only relabelto as we would never want to relabelfrom kmem_device or port_device
 neverallow ueventd { kmem_device port_device }:chr_file ~{ getattr create setattr unlink relabelto };
+
+# Nobody should be able to ptrace ueventd
+neverallow * ueventd:process ptrace;
diff --git a/public/vendor_init.te b/public/vendor_init.te
index 19d906b47..007c563dc 100644
--- a/public/vendor_init.te
+++ b/public/vendor_init.te
@@ -229,3 +229,6 @@ neverallow vendor_init { file_type fs_type }:file execute_no_trans;
 # Init never adds or uses services via service_manager.
 neverallow vendor_init service_manager_type:service_manager { add find };
 neverallow vendor_init servicemanager:service_manager list;
+
+# vendor_init should never be ptraced
+neverallow * vendor_init:process ptrace;
-- 
GitLab