From a7cb3671455caf4ad14e1937ec520f519deaee6b Mon Sep 17 00:00:00 2001 From: Nikita Ioffe Date: Fri, 24 Feb 2023 23:10:34 +0000 Subject: [PATCH] Make sure that files under bin/ directory of apk get X permission This change adds support for adding executables under bin/ directory of the apk, so that when that apk is mounted in the Microdroid the payload can then execute such binaries. As an example, put measure_io binary into the MicrodroidTestApp.apk, and add a test that asserts that /mnt/apk/bin/measure_io has R & X permissions. In the following change the authfs benchmark tests will be migrated to use the measure_io binary under /mnt/apk/bin/measure_io instead of pushing one to the /data partition inside Microdroid. This is required to reland the change that mounts /data with MS_NOEXEC. Bug: 265261525 Bug: 270955654 Test: atest MicrodroidTestApp Change-Id: Ia5294f2a1bc2a54505670425bbd835c7793c6f29 --- authfs/tests/benchmarks/Android.bp | 16 ++++++++ .../microdroid/testservice/ITestService.aidl | 3 ++ .../test/device/MicrodroidDeviceTestBase.java | 1 + tests/testapk/Android.bp | 1 + .../microdroid/test/MicrodroidTests.java | 37 +++++++++++++++++++ tests/testapk/src/native/testbinary.cpp | 12 ++++++ .../test/sharevm/VmShareServiceImpl.java | 5 +++ zipfuse/src/inode.rs | 23 ++++++++---- 8 files changed, 91 insertions(+), 7 deletions(-) diff --git a/authfs/tests/benchmarks/Android.bp b/authfs/tests/benchmarks/Android.bp index 9bdef7ba..96cbd1d7 100644 --- a/authfs/tests/benchmarks/Android.bp +++ b/authfs/tests/benchmarks/Android.bp @@ -36,3 +36,19 @@ cc_binary { "libbase", ], } + +// Package measure_io binary into a jar, to bundle with the MicrodroidTestApp. +// When MicrodroidTestApp is mounted inside the Microdroid, the zipfuse will +// add the +x permission on it. +java_genrule { + name: "measure_io_as_jar", + out: ["measure_io.jar"], + srcs: [ + ":measure_io", + ], + cmd: "out_dir=$$(dirname $(out))" + + "&& bin_dir=\"bin\" " + + "&& mkdir -p $$out_dir/$$bin_dir" + + "&& cp $(in) $$out_dir/$$bin_dir" + + "&& jar cf $(out) -C $$out_dir $$bin_dir", +} diff --git a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl index 46111341..c1e39dba 100644 --- a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl +++ b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl @@ -56,6 +56,9 @@ interface ITestService { /* get the content of the specified file. */ String readFromFile(String path); + /* get file permissions of the give file by stat'ing it */ + int getFilePermissions(String path); + /** * Request the service to exit, triggering the termination of the VM. This may cause any * requests in flight to fail. 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 be3c1da4..4a33d6f3 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 @@ -443,6 +443,7 @@ public abstract class MicrodroidDeviceTestBase { public String mFileContent; public byte[] mBcc; public long[] mTimings; + public int mFileMode; public void assertNoException() { if (mException != null) { diff --git a/tests/testapk/Android.bp b/tests/testapk/Android.bp index bafab535..9f804337 100644 --- a/tests/testapk/Android.bp +++ b/tests/testapk/Android.bp @@ -31,6 +31,7 @@ android_test { "cbor-java", "truth-prebuilt", "compatibility-common-util-devicesidelib", + "measure_io_as_jar", ], jni_libs: [ "MicrodroidTestNativeLib", 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 f3f12521..0a815424 100644 --- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java +++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java @@ -45,6 +45,7 @@ import android.os.ParcelFileDescriptor; import android.os.ParcelFileDescriptor.AutoCloseInputStream; import android.os.ParcelFileDescriptor.AutoCloseOutputStream; import android.os.SystemProperties; +import android.system.OsConstants; import android.system.virtualmachine.VirtualMachine; import android.system.virtualmachine.VirtualMachineCallback; import android.system.virtualmachine.VirtualMachineConfig; @@ -1779,6 +1780,42 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase { } } + @Test + @CddTest(requirements = {"9.17/C-1-5"}) + public void testFileUnderBinHasExecutePermission() throws Exception { + assumeSupportedKernel(); + + VirtualMachineConfig vmConfig = + newVmConfigBuilder() + .setPayloadBinaryName("MicrodroidTestNativeLib.so") + .setMemoryBytes(minMemoryRequired()) + .setDebugLevel(DEBUG_LEVEL_FULL) + .build(); + VirtualMachine vm = forceCreateNewVirtualMachine("test_vm_perms", vmConfig); + + TestResults testResults = + runVmTestService( + TAG, + vm, + (ts, tr) -> { + tr.mFileMode = ts.getFilePermissions("/mnt/apk/bin/measure_io"); + }); + + testResults.assertNoException(); + int allPermissionsMask = + OsConstants.S_IRUSR + | OsConstants.S_IWUSR + | OsConstants.S_IXUSR + | OsConstants.S_IRGRP + | OsConstants.S_IWGRP + | OsConstants.S_IXGRP + | OsConstants.S_IROTH + | OsConstants.S_IWOTH + | OsConstants.S_IXOTH; + assertThat(testResults.mFileMode & allPermissionsMask) + .isEqualTo(OsConstants.S_IRUSR | OsConstants.S_IXUSR); + } + private static class VmShareServiceConnection implements ServiceConnection { private final CountDownLatch mLatch = new CountDownLatch(1); diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp index 19d6cd41..07c8cd42 100644 --- a/tests/testapk/src/native/testbinary.cpp +++ b/tests/testapk/src/native/testbinary.cpp @@ -251,6 +251,18 @@ Result start_test_service() { return ScopedAStatus::ok(); } + ScopedAStatus getFilePermissions(const std::string& path, int32_t* out) override { + struct stat sb; + if (stat(path.c_str(), &sb) != -1) { + *out = sb.st_mode; + } else { + std::string msg = "stat " + path + " failed : " + std::strerror(errno); + return ScopedAStatus::fromExceptionCodeWithMessage(EX_SERVICE_SPECIFIC, + msg.c_str()); + } + return ScopedAStatus::ok(); + } + ScopedAStatus quit() override { exit(0); } }; auto testService = ndk::SharedRefBase::make(); 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 278e1a22..8c995d7f 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 @@ -229,6 +229,11 @@ public class VmShareServiceImpl extends Service { return mServiceInVm.readFromFile(path); } + @Override + public int getFilePermissions(String path) throws RemoteException { + throw new UnsupportedOperationException("Not supported"); + } + @Override public void quit() throws RemoteException { throw new UnsupportedOperationException("Not supported"); diff --git a/zipfuse/src/inode.rs b/zipfuse/src/inode.rs index 3edbc499..ea634227 100644 --- a/zipfuse/src/inode.rs +++ b/zipfuse/src/inode.rs @@ -99,12 +99,8 @@ impl InodeData { InodeData { mode, size: 0, data: InodeDataData::Directory(HashMap::new()) } } - fn new_file(zip_index: ZipIndex, zip_file: &zip::read::ZipFile) -> InodeData { - InodeData { - mode: zip_file.unix_mode().unwrap_or(DEFAULT_FILE_MODE), - size: zip_file.size(), - data: InodeDataData::File(zip_index), - } + fn new_file(zip_index: ZipIndex, mode: u32, zip_file: &zip::read::ZipFile) -> InodeData { + InodeData { mode, size: zip_file.size(), data: InodeDataData::File(zip_index) } } fn add_to_directory(&mut self, name: CString, entry: DirectoryEntry) { @@ -188,6 +184,16 @@ impl InodeTable { let mut parent = ROOT; let mut iter = path.iter().peekable(); + + let mut file_mode = DEFAULT_FILE_MODE; + if path.starts_with("bin/") { + // Allow files under bin to have execute permission, this enables payloads to bundle + // 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; + } + while let Some(name) = iter.next() { // TODO(jiyong): remove this check by canonicalizing `path` if name == ".." { @@ -211,8 +217,11 @@ impl InodeTable { } // No inode found. Create a new inode and add it to the inode table. + // At the moment of writing this comment the apk file doesn't specify any + // permissions (apart from the ones on lib/), but it might change in the future. + // TODO(b/270955654): should we control the file permissions ourselves? let inode = if is_file { - InodeData::new_file(i, &file) + InodeData::new_file(i, file.unix_mode().unwrap_or(file_mode), &file) } else if is_leaf { InodeData::new_dir(file.unix_mode().unwrap_or(DEFAULT_DIR_MODE)) } else {