From 9a5992336e888533ac3f6536f7ad9a70eb861396 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 4 May 2022 22:18:02 +0000 Subject: [PATCH] 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 --- private/apexd.te | 9 +++++--- private/compat/33.0/33.0.cil | 6 +++--- private/file_contexts | 35 ++++++++++++++++++++++++------ private/mediaprovider_app.te | 1 + private/perfetto.te | 3 +++ private/system_server.te | 4 ++++ private/traced.te | 3 +++ private/traced_probes.te | 3 +++ private/vold.te | 42 ++++++++++++++++++++++++++++++++++++ private/zygote.te | 7 ++++-- public/domain.te | 22 ++++++++++++++++--- public/file.te | 10 ++++++++- public/init.te | 1 + public/installd.te | 9 ++++---- public/toolbox.te | 6 +++--- public/vold.te | 8 +++++-- 16 files changed, 142 insertions(+), 27 deletions(-) diff --git a/private/apexd.te b/private/apexd.te index 0cafd3c89..6db0fd90e 100644 --- a/private/apexd.te +++ b/private/apexd.te @@ -33,9 +33,12 @@ allow apexd apex_module_data_file:file { create_file_perms relabelfrom }; 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 7a64f0ad2..443927780 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 af5179983..0c45a88ce 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 a9a52bbe8..dc6882b9c 100644 --- a/private/mediaprovider_app.te +++ b/private/mediaprovider_app.te @@ -12,6 +12,7 @@ r_dir_file(mediaprovider_app, mnt_pass_through_file) 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 5897aed4a..0904a67e0 100644 --- a/private/perfetto.te +++ b/private/perfetto.te @@ -110,6 +110,9 @@ neverallow perfetto { 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 9eea9c1c4..b5e9e452f 100644 --- a/private/system_server.te +++ b/private/system_server.te @@ -486,6 +486,10 @@ allow system_server keychain_data_file:dir create_dir_perms; 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 a6e200e62..ec31a20f1 100644 --- a/private/traced.te +++ b/private/traced.te @@ -95,6 +95,9 @@ neverallow traced { -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 66d5ac44e..f2be14d7e 100644 --- a/private/traced_probes.te +++ b/private/traced_probes.te @@ -126,6 +126,9 @@ neverallow traced_probes { -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 cb7b1bcd1..22553ea14 100644 --- a/private/vold.te +++ b/private/vold.te @@ -66,3 +66,45 @@ neverallow { -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 db390053a..936862144 100644 --- a/private/zygote.te +++ b/private/zygote.te @@ -66,13 +66,15 @@ allow zygote apex_art_data_file:file { r_file_perms execute }; # 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 @@ allow zygote tmpfs:lnk_file create; # 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 diff --git a/public/domain.te b/public/domain.te index 6258c7a0d..bc3f373bd 100644 --- a/public/domain.te +++ b/public/domain.te @@ -242,16 +242,30 @@ r_dir_file(domain, sysfs_usb); allow domain sysfs_transparent_hugepage:dir search; allow domain sysfs_transparent_hugepage:file r_file_perms; -# files under /data. +# Allow search access, and sometimes getattr access, to various directories +# under /data. We are fairly lenient in allowing search access to top-level +# dirs that commonly need to be traversed to get access to the "real" files, as +# this greatly simplifies the policy and doesn't open up much attack surface. not_full_treble(` allow domain system_data_file:dir getattr; ') allow { coredomain appdomain } system_data_file:dir getattr; -# /data has the label system_data_root_file. Vendor components need the search -# permission on system_data_root_file for path traversal to /data/vendor. +# Anything that accesses anything in /data needs search access to /data itself. +# This includes vendor components, as they need to access /data/vendor. allow domain system_data_root_file:dir { search getattr } ; +# system_data_file is the default type for directories in /data. Anything +# accessing data files with a more specific type often has to traverse a +# system_data_file directory such as /data/misc to get there. allow domain system_data_file:dir search; +# Anything that accesses files in /data/user (and /data/user_de, etc.) needs +# search access to these directories themselves. getattr access is sometimes +# needed too. +allow { coredomain appdomain } system_userdir_file:dir { search getattr }; +# Anything that accesses files in /data/media needs search access to /data/media +# itself. +allow { coredomain appdomain } media_userdir_file:dir search; # TODO restrict this to non-coredomain +allow domain vendor_userdir_file:dir { getattr search }; allow domain vendor_data_file:dir { getattr search }; # required by the dynamic linker @@ -853,6 +867,7 @@ full_treble_only(` core_data_file_type -system_data_file # default label for files on /data. Covered below... -system_data_root_file + -vendor_userdir_file -vendor_data_file -zoneinfo_data_file with_native_coverage(`-method_trace_data_file') @@ -865,6 +880,7 @@ full_treble_only(` -unencrypted_data_file -system_data_file -system_data_root_file + -vendor_userdir_file -vendor_data_file -zoneinfo_data_file with_native_coverage(`-method_trace_data_file') diff --git a/public/file.te b/public/file.te index 9d333f5de..009e86d5f 100644 --- a/public/file.te +++ b/public/file.te @@ -299,13 +299,20 @@ type coredump_file, file_type; type system_data_root_file, file_type, data_file_type, core_data_file_type; # Default type for anything under /data. type system_data_file, file_type, data_file_type, core_data_file_type; +# Default type for directories containing per-user encrypted directories, such +# as /data/user and /data/user_de. +type system_userdir_file, file_type, data_file_type, core_data_file_type; # Type for /data/system/packages.list. # TODO(b/129332765): Narrow down permissions to this. # Find out users of system_data_file that should be granted only this. type packages_list_file, file_type, data_file_type, core_data_file_type; type game_mode_intervention_list_file, file_type, data_file_type, core_data_file_type; -# Default type for anything under /data/vendor{_ce,_de}. +# Default type for anything inside /data/vendor_{ce,de}. type vendor_data_file, file_type, data_file_type; +# Type for /data/vendor_{ce,de} themselves. This has core_data_file_type +# because these directories themselves are platform-managed; only the files +# *inside* them are vendor data. (Somewhat similar to system_data_root_file.) +type vendor_userdir_file, file_type, data_file_type, core_data_file_type; # Unencrypted data type unencrypted_data_file, file_type, data_file_type, core_data_file_type; # installd-create files in /data/misc/installd such as layout_version @@ -427,6 +434,7 @@ type keychain_data_file, file_type, data_file_type, core_data_file_type; type keystore_data_file, file_type, data_file_type, core_data_file_type; type media_data_file, file_type, data_file_type, core_data_file_type; type media_rw_data_file, file_type, data_file_type, core_data_file_type, mlstrustedobject; +type media_userdir_file, file_type, data_file_type, core_data_file_type; type misc_user_data_file, file_type, data_file_type, core_data_file_type; type net_data_file, file_type, data_file_type, core_data_file_type; type network_watchlist_data_file, file_type, data_file_type, core_data_file_type; diff --git a/public/init.te b/public/init.te index ce0d130fe..d7b89f1fe 100644 --- a/public/init.te +++ b/public/init.te @@ -224,6 +224,7 @@ allow init { -system_dlkm_file_type -system_file_type -vendor_file_type + -vendor_userdir_file -vold_data_file }:dir { write add_name remove_name rmdir relabelfrom }; diff --git a/public/installd.te b/public/installd.te index 46796afa0..216704d3c 100644 --- a/public/installd.te +++ b/public/installd.te @@ -42,13 +42,12 @@ allow installd seapp_contexts_file:file r_file_perms; allow installd asec_image_file:dir search; allow installd asec_image_file:file getattr; -# Create /data/user and /data/user/0 if necessary. -# Also required to initially create /data/data subdirectories +# Required to initially create subdirectories of /data/user/$userId # and lib symlinks before the setfilecon call. May want to # move symlink creation after setfilecon in installd. allow installd system_data_file:dir create_dir_perms; -# Also, allow read for lnk_file so that we can process /data/user/0 links when -# optimizing application code. +# Also, allow read for lnk_file so that we can process symlinks within +# /data/user/$userId when optimizing application code. allow installd system_data_file:lnk_file { create getattr read setattr unlink }; # Manage lower filesystem via pass_through mounts @@ -62,6 +61,7 @@ allow installd system_data_file:dir relabelfrom; allow installd media_rw_data_file:dir relabelto; # Delete /data/media files through sdcardfs, instead of going behind its back +allow installd media_userdir_file:dir r_dir_perms; allow installd tmpfs:dir r_dir_perms; allow installd storage_file:dir search; allow installd { sdcard_type fuse }:dir { search open read write remove_name getattr rmdir }; @@ -71,6 +71,7 @@ allow installd { sdcard_type fuse }:file { getattr unlink }; allow installd mirror_data_file:dir { create_dir_perms mounton }; # Upgrade /data/misc/keychain for multi-user if necessary. +allow installd system_userdir_file:dir r_dir_perms; allow installd misc_user_data_file:dir create_dir_perms; allow installd misc_user_data_file:file create_file_perms; allow installd keychain_data_file:dir create_dir_perms; diff --git a/public/toolbox.te b/public/toolbox.te index 93adbc40c..3705a92df 100644 --- a/public/toolbox.te +++ b/public/toolbox.te @@ -22,11 +22,11 @@ neverallow { domain -init } toolbox:process transition; neverallow * toolbox:process dyntransition; neverallow toolbox { file_type fs_type -toolbox_exec}:file entrypoint; -# rm -rf directories in /data +# rm -rf /data/per_boot allow toolbox system_data_root_file:dir { remove_name write }; allow toolbox system_data_file:dir { rmdir rw_dir_perms }; allow toolbox system_data_file:file { getattr unlink }; # chattr +F /data/media in init -allow toolbox media_rw_data_file:dir { r_dir_perms setattr }; -allowxperm toolbox media_rw_data_file:dir ioctl { FS_IOC_SETFLAGS FS_IOC_GETFLAGS }; +allow toolbox media_userdir_file:dir { r_dir_perms setattr }; +allowxperm toolbox media_userdir_file:dir ioctl { FS_IOC_SETFLAGS FS_IOC_GETFLAGS }; diff --git a/public/vold.te b/public/vold.te index b0fb6d0df..07f0fd32e 100644 --- a/public/vold.te +++ b/public/vold.te @@ -99,8 +99,8 @@ allow vold media_rw_data_file:file create_file_perms; # Allow mounting (lower filesystem) on parts of media for performance allow vold media_rw_data_file:dir mounton; -# Allow setting extended attributes (for project quota IDs) on files and dirs -# and to enable project ID inheritance through FS_IOC_SETFLAGS +# Allow setting project quota IDs and enabling project ID inheritance on +# /data/media/$userId/* and /mnt/expand/$volume/media/$userId/* allowxperm vold media_rw_data_file:{ dir file } ioctl { FS_IOC_FSGETXATTR FS_IOC_FSSETXATTR @@ -124,6 +124,10 @@ allow vold mnt_pass_through_file:lnk_file create_file_perms; allow vold mnt_expand_file:dir { create_dir_perms mounton }; allow vold apk_data_file:dir { create getattr setattr }; allow vold shell_data_file:dir { create getattr setattr }; +allow vold system_userdir_file:dir { create getattr setattr }; +allow vold media_userdir_file:dir { create getattr setattr open read ioctl }; +# Needed to set the casefold flag on /mnt/expand/$volume/media +allowxperm vold media_userdir_file:dir ioctl { FS_IOC_GETFLAGS FS_IOC_SETFLAGS }; # Allow to mount incremental file system on /data/incremental and create files allow vold apk_data_file:dir { mounton rw_dir_perms };