Restrict creating per-user encrypted directories
Creating a per-user encrypted directory such as /data/system_ce/0 and
the subdirectories in it too early has been a recurring bug. Typically,
individual services in system_server are to blame; system_server has
permission to create these directories, and it's easy to write
"mkdirs()" instead of "mkdir()". Such bugs are very bad, as they
prevent these directories from being encrypted, as encryption policies
can only be set on empty directories. Due to recent changes, a factory
reset is now forced in such cases, which helps detect these bugs;
however, it would be much better to prevent them in the first place.
This CL locks down the ability to create these directories to just vold
and init, or to just vold when possible. This is done by assigning new
types to the directories that contain these directories, and then only
allowing the needed domains to write to these parent directories. This
is similar to what https://r.android.com/1117297 did for /data itself.
Three new types are used instead of just one, since these directories
had three different types already (system_data_file, media_rw_data_file,
vendor_data_file), and this allows the policy to be a bit more precise.
A significant limitation is that /data/user/0 is currently being created
by init during early boot. Therefore, this CL doesn't help much for
/data/user/0, though it helps a lot for the other directories. As the
next step, I'll try to eliminate the /data/user/0 quirk. Anyway, this
CL is needed regardless of whether we're able to do that.
Test: Booted cuttlefish. Ran 'sm partition disk:253,32 private', then
created and deleted a user. Used 'ls -lZ' to check the relevant
SELinux labels on both internal and adoptable storage. Also did
similar tests on raven, with the addition of going through the
setup wizard and using an app that creates media files. No
relevant SELinux denials seen during any of this.
Bug: 156305599
Change-Id: I1fbdd180f56dd2fe4703763936f5850cef8ab0ba
diff --git a/private/apexd.te b/private/apexd.te
index 0cafd3c..6db0fd9 100644
--- a/private/apexd.te
+++ b/private/apexd.te
@@ -33,9 +33,12 @@
allow apexd apex_rollback_data_file:dir create_dir_perms;
allow apexd apex_rollback_data_file:file create_file_perms;
-# Allow apexd to read directories under /data/misc_de in order to snapshot and
-# restore apex data for all users.
-allow apexd system_data_file:dir r_dir_perms;
+# Allow apexd to read /data/misc_de and the directories under it, in order to
+# snapshot and restore apex data for all users.
+allow apexd {
+ system_userdir_file
+ system_data_file
+}:dir r_dir_perms;
# allow apexd to create loop devices with /dev/loop-control
allow apexd loop_control_device:chr_file rw_file_perms;
diff --git a/private/compat/33.0/33.0.cil b/private/compat/33.0/33.0.cil
index 7a64f0a..4439277 100644
--- a/private/compat/33.0/33.0.cil
+++ b/private/compat/33.0/33.0.cil
@@ -1919,7 +1919,7 @@
(typeattributeset media_metrics_service_33_0 (media_metrics_service))
(typeattributeset media_projection_service_33_0 (media_projection_service))
(typeattributeset media_router_service_33_0 (media_router_service))
-(typeattributeset media_rw_data_file_33_0 (media_rw_data_file))
+(typeattributeset media_rw_data_file_33_0 (media_rw_data_file media_userdir_file))
(typeattributeset media_session_service_33_0 (media_session_service))
(typeattributeset media_variant_prop_33_0 (media_variant_prop))
(typeattributeset mediadrm_config_prop_33_0 (mediadrm_config_prop))
@@ -2362,7 +2362,7 @@
(typeattributeset system_boot_reason_prop_33_0 (system_boot_reason_prop))
(typeattributeset system_bootstrap_lib_file_33_0 (system_bootstrap_lib_file))
(typeattributeset system_config_service_33_0 (system_config_service))
-(typeattributeset system_data_file_33_0 (system_data_file))
+(typeattributeset system_data_file_33_0 (system_data_file system_userdir_file))
(typeattributeset system_data_root_file_33_0 (system_data_root_file))
(typeattributeset system_dlkm_file_33_0 (system_dlkm_file))
(typeattributeset system_event_log_tags_file_33_0 (system_event_log_tags_file))
@@ -2505,7 +2505,7 @@
(typeattributeset vendor_app_file_33_0 (vendor_app_file))
(typeattributeset vendor_cgroup_desc_file_33_0 (vendor_cgroup_desc_file))
(typeattributeset vendor_configs_file_33_0 (vendor_configs_file))
-(typeattributeset vendor_data_file_33_0 (vendor_data_file))
+(typeattributeset vendor_data_file_33_0 (vendor_data_file vendor_userdir_file))
(typeattributeset vendor_default_prop_33_0 (vendor_default_prop))
(typeattributeset vendor_file_33_0 (vendor_file))
(typeattributeset vendor_framework_file_33_0 (vendor_framework_file))
diff --git a/private/file_contexts b/private/file_contexts
index af51799..0c45a88 100644
--- a/private/file_contexts
+++ b/private/file_contexts
@@ -563,7 +563,8 @@
/data/local/tmp(/.*)? u:object_r:shell_data_file:s0
/data/local/tmp/ltp(/.*)? u:object_r:nativetest_data_file:s0
/data/local/traces(/.*)? u:object_r:trace_data_file:s0
-/data/media(/.*)? u:object_r:media_rw_data_file:s0
+/data/media u:object_r:media_userdir_file:s0
+/data/media/.* u:object_r:media_rw_data_file:s0
/data/mediadrm(/.*)? u:object_r:media_data_file:s0
/data/nativetest(/.*)? u:object_r:nativetest_data_file:s0
/data/nativetest64(/.*)? u:object_r:nativetest_data_file:s0
@@ -580,6 +581,12 @@
/data/rollback/\d+/[^/]+/.*\.apk u:object_r:apk_data_file:s0
/data/rollback/\d+/[^/]+/.*\.apex u:object_r:staging_data_file:s0
/data/fonts/files(/.*)? u:object_r:font_data_file:s0
+/data/misc_ce u:object_r:system_userdir_file:s0
+/data/misc_de u:object_r:system_userdir_file:s0
+/data/system_ce u:object_r:system_userdir_file:s0
+/data/system_de u:object_r:system_userdir_file:s0
+/data/user u:object_r:system_userdir_file:s0
+/data/user_de u:object_r:system_userdir_file:s0
# Misc data
/data/misc/adb(/.*)? u:object_r:adb_keys_file:s0
@@ -665,8 +672,10 @@
/data/misc/profiles/ref(/.*)? u:object_r:user_profile_data_file:s0
/data/misc/profman(/.*)? u:object_r:profman_dump_data_file:s0
/data/vendor(/.*)? u:object_r:vendor_data_file:s0
-/data/vendor_ce(/.*)? u:object_r:vendor_data_file:s0
-/data/vendor_de(/.*)? u:object_r:vendor_data_file:s0
+/data/vendor_ce u:object_r:vendor_userdir_file:s0
+/data/vendor_ce/.* u:object_r:vendor_data_file:s0
+/data/vendor_de u:object_r:vendor_userdir_file:s0
+/data/vendor_de/.* u:object_r:vendor_data_file:s0
# storaged proto files
/data/misc_de/[0-9]+/storaged(/.*)? u:object_r:storaged_data_file:s0
@@ -721,8 +730,17 @@
#############################
# Expanded data files
#
-/mnt/expand(/.*)? u:object_r:mnt_expand_file:s0
-/mnt/expand/[^/]+(/.*)? u:object_r:system_data_file:s0
+/mnt/expand u:object_r:mnt_expand_file:s0
+# CAREFUL: the two system_data_file patterns below can't be replaced with one
+# pattern "/mnt/expand/[^/]+(/.*)?", since SELinux would prioritize that over
+# "/mnt/expand/[^/]+/user". This is because when a path is matched by two
+# patterns that contain regex meta-characters, SELinux just chooses the longer
+# pattern (or the later pattern if the patterns are the same length), rather
+# than the pattern containing fewer regex meta-characters. Splitting the
+# pattern into "/mnt/expand/[^/]+" and "/mnt/expand/[^/]+/.*" works around this
+# problem, except for 1-character filenames which we aren't using.
+/mnt/expand/[^/]+ u:object_r:system_data_file:s0
+/mnt/expand/[^/]+/.* u:object_r:system_data_file:s0
/mnt/expand/[^/]+/app(/.*)? u:object_r:apk_data_file:s0
/mnt/expand/[^/]+/app/[^/]+/oat(/.*)? u:object_r:dalvikcache_data_file:s0
# /mnt/expand/..../app/[randomStringA]/[packageName]-[randomStringB]/base.apk layout
@@ -730,8 +748,13 @@
/mnt/expand/[^/]+/app/vmdl[^/]+\.tmp(/.*)? u:object_r:apk_tmp_file:s0
/mnt/expand/[^/]+/app/vmdl[^/]+\.tmp/oat(/.*)? u:object_r:dalvikcache_data_file:s0
/mnt/expand/[^/]+/local/tmp(/.*)? u:object_r:shell_data_file:s0
-/mnt/expand/[^/]+/media(/.*)? u:object_r:media_rw_data_file:s0
+/mnt/expand/[^/]+/media u:object_r:media_userdir_file:s0
+/mnt/expand/[^/]+/media/.* u:object_r:media_rw_data_file:s0
/mnt/expand/[^/]+/misc/vold(/.*)? u:object_r:vold_data_file:s0
+/mnt/expand/[^/]+/misc_ce u:object_r:system_userdir_file:s0
+/mnt/expand/[^/]+/misc_de u:object_r:system_userdir_file:s0
+/mnt/expand/[^/]+/user u:object_r:system_userdir_file:s0
+/mnt/expand/[^/]+/user_de u:object_r:system_userdir_file:s0
# coredump directory for userdebug/eng devices
/cores(/.*)? u:object_r:coredump_file:s0
diff --git a/private/mediaprovider_app.te b/private/mediaprovider_app.te
index a9a52bb..dc6882b 100644
--- a/private/mediaprovider_app.te
+++ b/private/mediaprovider_app.te
@@ -12,6 +12,7 @@
allow mediaprovider_app fuse_device:chr_file { read write ioctl getattr };
# Allow MediaProvider to read/write media_rw_data_file files and dirs
+allow mediaprovider_app media_userdir_file:dir r_dir_perms;
allow mediaprovider_app media_rw_data_file:file create_file_perms;
allow mediaprovider_app media_rw_data_file:dir create_dir_perms;
diff --git a/private/perfetto.te b/private/perfetto.te
index 5897aed..0904a67 100644
--- a/private/perfetto.te
+++ b/private/perfetto.te
@@ -110,6 +110,9 @@
data_file_type
-system_data_file
-system_data_root_file
+ -media_userdir_file
+ -system_userdir_file
+ -vendor_userdir_file
# TODO(b/72998741) Remove exemption. Further restricted in a subsequent
# neverallow. Currently only getattr and search are allowed.
-vendor_data_file
diff --git a/private/system_server.te b/private/system_server.te
index 9eea9c1..b5e9e45 100644
--- a/private/system_server.te
+++ b/private/system_server.te
@@ -486,6 +486,10 @@
allow system_server keychain_data_file:file create_file_perms;
allow system_server keychain_data_file:lnk_file create_file_perms;
+# Read the user parent directories like /data/user. Don't allow write access,
+# as vold and init are responsible for creating and deleting the subdirectories.
+allow system_server system_userdir_file:dir r_dir_perms;
+
# Manage /data/app.
allow system_server apk_data_file:dir create_dir_perms;
allow system_server apk_data_file:{ file lnk_file } { create_file_perms link };
diff --git a/private/traced.te b/private/traced.te
index a6e200e..ec31a20 100644
--- a/private/traced.te
+++ b/private/traced.te
@@ -95,6 +95,9 @@
-perfetto_traces_bugreport_data_file
-system_data_file
-system_data_root_file
+ -media_userdir_file
+ -system_userdir_file
+ -vendor_userdir_file
# TODO(b/72998741) Remove vendor_data_file exemption. Further restricted in a
# subsequent neverallow. Currently only getattr and search are allowed.
-vendor_data_file
diff --git a/private/traced_probes.te b/private/traced_probes.te
index 66d5ac4..f2be14d 100644
--- a/private/traced_probes.te
+++ b/private/traced_probes.te
@@ -126,6 +126,9 @@
-dalvikcache_data_file
-system_data_file
-system_data_root_file
+ -media_userdir_file
+ -system_userdir_file
+ -vendor_userdir_file
-system_app_data_file
-backup_data_file
-bootstat_data_file
diff --git a/private/vold.te b/private/vold.te
index cb7b1bc..22553ea 100644
--- a/private/vold.te
+++ b/private/vold.te
@@ -66,3 +66,45 @@
-apexd
-gsid
} vold_service:service_manager find;
+
+# Allow vold to create and delete per-user directories like /data/user/$userId.
+allow vold {
+ media_userdir_file
+ system_userdir_file
+ vendor_userdir_file
+}:dir {
+ add_name
+ remove_name
+ write
+};
+
+# Only vold should create (and delete) per-user directories like
+# /data/user/$userId. This is very important, as these directories need to be
+# encrypted with per-user keys, which only vold can do. Encryption can only be
+# set up on empty directories, so creation and encryption must happen together.
+#
+# Exception: init creates /data/user/0 and /data/media/obb, so that needs to be
+# allowed for now. (/data/media/obb isn't actually a per-user directory, but
+# it's located in /data/media so it constrains the sepolicy for that directory.)
+neverallow {
+ domain
+ -vold
+} {
+ vendor_userdir_file
+}:dir {
+ add_name
+ remove_name
+ write
+};
+neverallow {
+ domain
+ -vold
+ -init
+} {
+ system_userdir_file
+ media_userdir_file
+}:dir {
+ add_name
+ remove_name
+ write
+};
diff --git a/private/zygote.te b/private/zygote.te
index db39005..9368621 100644
--- a/private/zygote.te
+++ b/private/zygote.te
@@ -66,13 +66,15 @@
# them for app data isolation. Also traverse these directories (via
# /data_mirror) to find the allowlisted per-app directories to bind-mount in.
allow zygote {
- # /data/data, /data/user{,_de}, /mnt/expand/$volume/user{,_de}
+ # /data/user{,_de}, /mnt/expand/$volume/user{,_de}
+ system_userdir_file
+ # /data/data
system_data_file
# /data/misc/profiles/cur
user_profile_root_file
# /data/misc/profiles/ref
user_profile_data_file
- # /storage/emulated/$uid/Android/{data,obb}
+ # /storage/emulated/$userId/Android/{data,obb}
media_rw_data_file
}:dir { mounton search };
@@ -100,6 +102,7 @@
# standard labels. Note: it seems that not all dirs are actually relabeled yet,
# but it works anyway since all domains can search tmpfs:dir.
allow zygote tmpfs:{ dir lnk_file } relabelfrom;
+allow zygote system_userdir_file:dir relabelto;
allow zygote system_data_file:{ dir lnk_file } relabelto;
# Read if sdcardfs is supported