Run app payloads as non-root.
This is in preparation before moving to running multiple payload
processes in multiple UIDs.
Add a new payload user and group in the system-reserved range, only
within Microdroid, and assign them to the payload process. Fix up a
bunch of DAC permissions to make sure the payload still has access to
the things it should have.
Add a test to check we aren't running as root, and make some minor
test fixes.
This is a potentially breaking change, so for now I've disabled it via
Rust conditional compilation (and marked the new test as @Ignore). I
claim the changes that aren't protected by this are harmless.
I've run tests with and without the cfg option enabled.
Unrelated changes done in passing:
- Move a comment from reference to definition.
- Make sure encryptedstore logs any errors in full.
- Use with_context in a few more places.
Bug: 296393106
Test: atest MicrodroidTests
Change-Id: I6648580615a9fce906dd170f999e11f63e5874d9
diff --git a/apex/Android.bp b/apex/Android.bp
index 765372a..ccbdb3b 100644
--- a/apex/Android.bp
+++ b/apex/Android.bp
@@ -103,7 +103,6 @@
"microdroid_initrd_normal",
"microdroid.json",
"microdroid_kernel",
- // rialto_bin is a prebuilt target wrapping the signed bare-metal service VM.
"rialto_bin",
],
host_required: [
diff --git a/encryptedstore/Android.bp b/encryptedstore/Android.bp
index 94ebcfc..8ba5016 100644
--- a/encryptedstore/Android.bp
+++ b/encryptedstore/Android.bp
@@ -14,6 +14,7 @@
"libclap",
"libhex",
"liblog_rust",
+ "libmicrodroid_uids",
"libnix",
"libdm_rust",
],
diff --git a/encryptedstore/src/main.rs b/encryptedstore/src/main.rs
index 1a16f49..2a698ea 100644
--- a/encryptedstore/src/main.rs
+++ b/encryptedstore/src/main.rs
@@ -21,24 +21,32 @@
use anyhow::{ensure, Context, Result};
use clap::arg;
use dm::{crypt::CipherType, util};
-use log::info;
+use log::{error, info};
use std::ffi::CString;
use std::fs::{create_dir_all, OpenOptions};
use std::io::{Error, Read, Write};
use std::os::unix::ffi::OsStrExt;
-use std::os::unix::fs::FileTypeExt;
+use std::os::unix::fs::{FileTypeExt, PermissionsExt};
use std::path::{Path, PathBuf};
use std::process::Command;
const MK2FS_BIN: &str = "/system/bin/mke2fs";
const UNFORMATTED_STORAGE_MAGIC: &str = "UNFORMATTED-STORAGE";
-fn main() -> Result<()> {
+fn main() {
android_logger::init_once(
android_logger::Config::default()
.with_tag("encryptedstore")
.with_min_level(log::Level::Info),
);
+
+ if let Err(e) = try_main() {
+ error!("{:?}", e);
+ std::process::exit(1)
+ }
+}
+
+fn try_main() -> Result<()> {
info!("Starting encryptedstore binary");
let matches = clap_command().get_matches();
@@ -47,10 +55,12 @@
let key = matches.get_one::<String>("key").unwrap();
let mountpoint = Path::new(matches.get_one::<String>("mountpoint").unwrap());
// Note this error context is used in MicrodroidTests.
- encryptedstore_init(blkdevice, key, mountpoint).context(format!(
- "Unable to initialize encryptedstore on {:?} & mount at {:?}",
- blkdevice, mountpoint
- ))?;
+ encryptedstore_init(blkdevice, key, mountpoint).with_context(|| {
+ format!(
+ "Unable to initialize encryptedstore on {:?} & mount at {:?}",
+ blkdevice, mountpoint
+ )
+ })?;
Ok(())
}
@@ -65,7 +75,7 @@
fn encryptedstore_init(blkdevice: &Path, key: &str, mountpoint: &Path) -> Result<()> {
ensure!(
std::fs::metadata(blkdevice)
- .context(format!("Failed to get metadata of {:?}", blkdevice))?
+ .with_context(|| format!("Failed to get metadata of {:?}", blkdevice))?
.file_type()
.is_block_device(),
"The path:{:?} is not of a block device",
@@ -82,7 +92,12 @@
info!("Freshly formatting the crypt device");
format_ext4(&crypt_device)?;
}
- mount(&crypt_device, mountpoint).context(format!("Unable to mount {:?}", crypt_device))?;
+ mount(&crypt_device, mountpoint)
+ .with_context(|| format!("Unable to mount {:?}", crypt_device))?;
+ if needs_formatting {
+ std::fs::set_permissions(mountpoint, PermissionsExt::from_mode(0o770))
+ .context("Failed to chmod root directory")?;
+ }
Ok(())
}
@@ -124,6 +139,11 @@
}
fn format_ext4(device: &Path) -> Result<()> {
+ let root_dir_uid_gid = format!(
+ "root_owner={}:{}",
+ microdroid_uids::ROOT_UID,
+ microdroid_uids::MICRODROID_PAYLOAD_GID
+ );
let mkfs_options = [
"-j", // Create appropriate sized journal
/* metadata_csum: enabled for filesystem integrity
@@ -131,20 +151,22 @@
* 64bit: larger fields afforded by this feature enable full-strength checksumming.
*/
"-O metadata_csum, extents, 64bit",
- "-b 4096", // block size in the filesystem
+ "-b 4096", // block size in the filesystem,
+ "-E",
+ &root_dir_uid_gid,
];
let mut cmd = Command::new(MK2FS_BIN);
let status = cmd
.args(mkfs_options)
.arg(device)
.status()
- .context(format!("failed to execute {}", MK2FS_BIN))?;
+ .with_context(|| format!("failed to execute {}", MK2FS_BIN))?;
ensure!(status.success(), "mkfs failed with {:?}", status);
Ok(())
}
fn mount(source: &Path, mountpoint: &Path) -> Result<()> {
- create_dir_all(mountpoint).context(format!("Failed to create {:?}", &mountpoint))?;
+ create_dir_all(mountpoint).with_context(|| format!("Failed to create {:?}", &mountpoint))?;
let mount_options = CString::new(
"fscontext=u:object_r:encryptedstore_fs:s0,context=u:object_r:encryptedstore_file:s0",
)
diff --git a/libs/microdroid_uids/Android.bp b/libs/microdroid_uids/Android.bp
new file mode 100644
index 0000000..497948d
--- /dev/null
+++ b/libs/microdroid_uids/Android.bp
@@ -0,0 +1,15 @@
+package {
+ default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+rust_library {
+ name: "libmicrodroid_uids",
+ crate_name: "microdroid_uids",
+ srcs: ["src/lib.rs"],
+ edition: "2021",
+ // TODO(b/296393106): Figure out how/when to enable this
+ // cfgs: ["payload_not_root"],
+ apex_available: [
+ "com.android.virt",
+ ],
+}
diff --git a/libs/microdroid_uids/src/lib.rs b/libs/microdroid_uids/src/lib.rs
new file mode 100644
index 0000000..1f09c65
--- /dev/null
+++ b/libs/microdroid_uids/src/lib.rs
@@ -0,0 +1,24 @@
+// Copyright 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! User and group IDs within Microdroid
+
+/// Always the user ID of Root.
+pub const ROOT_UID: u32 = 0;
+
+/// Group ID shared by all payload users.
+pub const MICRODROID_PAYLOAD_GID: u32 = if cfg!(payload_not_root) { 6000 } else { 0 };
+
+/// User ID for the initial payload user.
+pub const MICRODROID_PAYLOAD_UID: u32 = if cfg!(payload_not_root) { 6000 } else { 0 };
diff --git a/microdroid/Android.bp b/microdroid/Android.bp
index 2d3f084..1e594b7 100644
--- a/microdroid/Android.bp
+++ b/microdroid/Android.bp
@@ -54,6 +54,8 @@
deps: [
"init_second_stage.microdroid",
"microdroid_build_prop",
+ "microdroid_etc_passwd",
+ "microdroid_etc_group",
"microdroid_init_debug_policy",
"microdroid_init_rc",
"microdroid_ueventd_rc",
@@ -156,6 +158,20 @@
installable: false, // avoid collision with system partition's ueventd.rc
}
+prebuilt_etc {
+ name: "microdroid_etc_passwd",
+ src: "microdroid_passwd",
+ filename: "passwd",
+ installable: false,
+}
+
+prebuilt_etc {
+ name: "microdroid_etc_group",
+ src: "microdroid_group",
+ filename: "group",
+ installable: false,
+}
+
prebuilt_root {
name: "microdroid_build_prop",
filename: "build.prop",
diff --git a/microdroid/init.rc b/microdroid/init.rc
index 42033d6..91931ab 100644
--- a/microdroid/init.rc
+++ b/microdroid/init.rc
@@ -28,6 +28,10 @@
on init
mkdir /mnt/apk 0755 system system
mkdir /mnt/extra-apk 0755 root root
+
+ # Allow the payload access to the console (default is 0600)
+ chmod 0666 /dev/console
+
# Microdroid_manager starts apkdmverity/zipfuse/apexd
start microdroid_manager
diff --git a/microdroid/microdroid_group b/microdroid/microdroid_group
new file mode 100644
index 0000000..4eb8fa5
--- /dev/null
+++ b/microdroid/microdroid_group
@@ -0,0 +1 @@
+system_payload::6000:
diff --git a/microdroid/microdroid_passwd b/microdroid/microdroid_passwd
new file mode 100644
index 0000000..bd15182
--- /dev/null
+++ b/microdroid/microdroid_passwd
@@ -0,0 +1 @@
+system_payload_0::6000:6000:::
diff --git a/microdroid_manager/Android.bp b/microdroid_manager/Android.bp
index 23f174e..fe0cf6a 100644
--- a/microdroid_manager/Android.bp
+++ b/microdroid_manager/Android.bp
@@ -32,6 +32,7 @@
"liblog_rust",
"libmicrodroid_metadata",
"libmicrodroid_payload_config",
+ "libmicrodroid_uids",
"libnix",
"libonce_cell",
"libopenssl",
diff --git a/microdroid_manager/microdroid_manager.rc b/microdroid_manager/microdroid_manager.rc
index e257547..da38564 100644
--- a/microdroid_manager/microdroid_manager.rc
+++ b/microdroid_manager/microdroid_manager.rc
@@ -8,8 +8,8 @@
# TODO(jooyung) remove this when microdroid_manager becomes a daemon
oneshot
# CAP_SYS_BOOT is required to exec kexecload from microdroid_manager
- # CAP_SETCAP is required to allow microdroid_manager to drop capabilities
+ # CAP_SETPCAP is required to allow microdroid_manager to drop capabilities
# before executing the payload
- capabilities AUDIT_CONTROL SYS_ADMIN SYS_BOOT SETPCAP
+ capabilities AUDIT_CONTROL SYS_ADMIN SYS_BOOT SETPCAP SETUID SETGID
user root
socket vm_payload_service stream 0666 system system
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index 319d2df..a48d540 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -528,8 +528,6 @@
}
impl Zipfuse {
- const MICRODROID_PAYLOAD_UID: u32 = 0; // TODO(b/264861173) should be non-root
- const MICRODROID_PAYLOAD_GID: u32 = 0; // TODO(b/264861173) should be non-root
fn mount(
&mut self,
noexec: MountForExec,
@@ -542,9 +540,13 @@
if let MountForExec::Disallowed = noexec {
cmd.arg("--noexec");
}
+ // Let root own the files in APK, so we can access them, but set the group to
+ // allow all payloads to have access too.
+ let (uid, gid) = (microdroid_uids::ROOT_UID, microdroid_uids::MICRODROID_PAYLOAD_GID);
+
cmd.args(["-p", &ready_prop, "-o", option]);
- cmd.args(["-u", &Self::MICRODROID_PAYLOAD_UID.to_string()]);
- cmd.args(["-g", &Self::MICRODROID_PAYLOAD_GID.to_string()]);
+ cmd.args(["-u", &uid.to_string()]);
+ cmd.args(["-g", &gid.to_string()]);
cmd.arg(zip_path).arg(mount_dir);
self.ready_properties.push(ready_prop);
cmd.spawn().with_context(|| format!("Failed to run zipfuse for {mount_dir:?}"))
@@ -850,10 +852,15 @@
fn exec_task(task: &Task, service: &Strong<dyn IVirtualMachineService>) -> Result<i32> {
info!("executing main task {:?}...", task);
let mut command = match task.type_ {
- TaskType::Executable => Command::new(&task.command),
+ TaskType::Executable => {
+ // TODO(b/296393106): Run system payloads as non-root.
+ Command::new(&task.command)
+ }
TaskType::MicrodroidLauncher => {
let mut command = Command::new("/system/bin/microdroid_launcher");
command.arg(find_library_path(&task.command)?);
+ command.uid(microdroid_uids::MICRODROID_PAYLOAD_UID);
+ command.gid(microdroid_uids::MICRODROID_PAYLOAD_GID);
command
}
};
diff --git a/rialto/Android.bp b/rialto/Android.bp
index ed9a284..7ac7136 100644
--- a/rialto/Android.bp
+++ b/rialto/Android.bp
@@ -76,6 +76,7 @@
}
prebuilt_etc {
+ // rialto_bin is a prebuilt target wrapping the signed bare-metal service VM.
name: "rialto_bin",
filename: "rialto.bin",
target: {
diff --git a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl
index 8d467cd..e81f6d7 100644
--- a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl
+++ b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl
@@ -55,6 +55,9 @@
/** Returns a mask of effective capabilities that the process running the payload binary has. */
String[] getEffectiveCapabilities();
+ /* Return the uid of the process running the binary. */
+ int getUid();
+
/* write the content into the specified file. */
void writeToFile(String content, String path);
diff --git a/tests/helper/src/java/com/android/microdroid/test/device/MicrodroidDeviceTestBase.java b/tests/helper/src/java/com/android/microdroid/test/device/MicrodroidDeviceTestBase.java
index e6d90ea..9f03ab7 100644
--- a/tests/helper/src/java/com/android/microdroid/test/device/MicrodroidDeviceTestBase.java
+++ b/tests/helper/src/java/com/android/microdroid/test/device/MicrodroidDeviceTestBase.java
@@ -451,6 +451,7 @@
public String mApkContentsPath;
public String mEncryptedStoragePath;
public String[] mEffectiveCapabilities;
+ public int mUid;
public String mFileContent;
public byte[] mBcc;
public long[] mTimings;
diff --git a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
index f6dc1b8..a928dcf 100644
--- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
+++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
@@ -72,6 +72,7 @@
import org.junit.After;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.function.ThrowingRunnable;
@@ -1523,6 +1524,30 @@
}
@Test
+ @Ignore // Figure out how to run this conditionally
+ @CddTest(requirements = {"9.17/C-1-1"})
+ public void payloadIsNotRoot() throws Exception {
+ assumeSupportedDevice();
+
+ VirtualMachineConfig config =
+ newVmConfigBuilder()
+ .setPayloadBinaryName("MicrodroidTestNativeLib.so")
+ .setMemoryBytes(minMemoryRequired())
+ .setDebugLevel(DEBUG_LEVEL_FULL)
+ .build();
+ VirtualMachine vm = forceCreateNewVirtualMachine("test_vm", config);
+ TestResults testResults =
+ runVmTestService(
+ TAG,
+ vm,
+ (ts, tr) -> {
+ tr.mUid = ts.getUid();
+ });
+ testResults.assertNoException();
+ assertThat(testResults.mUid).isNotEqualTo(0);
+ }
+
+ @Test
@CddTest(requirements = {"9.17/C-1-1"})
public void encryptedStorageIsPersistent() throws Exception {
assumeSupportedDevice();
@@ -1971,8 +1996,12 @@
| OsConstants.S_IROTH
| OsConstants.S_IWOTH
| OsConstants.S_IXOTH;
- assertThat(testResults.mFileMode & allPermissionsMask)
- .isEqualTo(OsConstants.S_IRUSR | OsConstants.S_IXUSR);
+ int expectedPermissions =
+ OsConstants.S_IRUSR
+ | OsConstants.S_IXUSR
+ | OsConstants.S_IRGRP
+ | OsConstants.S_IXGRP;
+ assertThat(testResults.mFileMode & allPermissionsMask).isEqualTo(expectedPermissions);
}
// Taken from bionic/libc/kernel/uapi/linux/mount.h
diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp
index 297b505..c9b5e3a 100644
--- a/tests/testapk/src/native/testbinary.cpp
+++ b/tests/testapk/src/native/testbinary.cpp
@@ -248,6 +248,11 @@
return ScopedAStatus::ok();
}
+ ScopedAStatus getUid(int* out) override {
+ *out = getuid();
+ return ScopedAStatus::ok();
+ }
+
ScopedAStatus runEchoReverseServer() override {
auto result = start_echo_reverse_server();
if (result.ok()) {
diff --git a/tests/vmshareapp/src/java/com/android/microdroid/test/sharevm/VmShareServiceImpl.java b/tests/vmshareapp/src/java/com/android/microdroid/test/sharevm/VmShareServiceImpl.java
index 0ddf70b..dc8908b 100644
--- a/tests/vmshareapp/src/java/com/android/microdroid/test/sharevm/VmShareServiceImpl.java
+++ b/tests/vmshareapp/src/java/com/android/microdroid/test/sharevm/VmShareServiceImpl.java
@@ -220,6 +220,11 @@
}
@Override
+ public int getUid() throws RemoteException {
+ throw new UnsupportedOperationException("Not supported");
+ }
+
+ @Override
public void writeToFile(String content, String path) throws RemoteException {
throw new UnsupportedOperationException("Not supported");
}
diff --git a/zipfuse/src/inode.rs b/zipfuse/src/inode.rs
index ef48389..3175a30 100644
--- a/zipfuse/src/inode.rs
+++ b/zipfuse/src/inode.rs
@@ -31,10 +31,11 @@
const INVALID: Inode = 0;
const ROOT: Inode = 1;
-const DEFAULT_DIR_MODE: u32 = libc::S_IRUSR | libc::S_IXUSR;
+const DEFAULT_DIR_MODE: u32 = libc::S_IRUSR | libc::S_IXUSR | libc::S_IRGRP | libc::S_IXGRP;
// b/264668376 some files in APK don't have unix permissions specified. Default to 400
// otherwise those files won't be readable even by the owner.
-const DEFAULT_FILE_MODE: u32 = libc::S_IRUSR;
+const DEFAULT_FILE_MODE: u32 = libc::S_IRUSR | libc::S_IRGRP;
+const EXECUTABLE_FILE_MODE: u32 = DEFAULT_FILE_MODE | libc::S_IXUSR | libc::S_IXGRP;
/// `InodeData` represents an inode which has metadata about a file or a directory
#[derive(Debug)]
@@ -191,7 +192,7 @@
// additional binaries that they might want to execute.
// An example of such binary is measure_io one used in the authfs performance tests.
// More context available at b/265261525 and b/270955654.
- file_mode |= libc::S_IXUSR;
+ file_mode = EXECUTABLE_FILE_MODE;
}
while let Some(name) = iter.next() {