Fix partition verification
Switch from a denylist to an allowlist for the selinux labels of disk
partitions. Fix the code to match against type rather than the full
label. Extend the list of exempted partitions. Add a unit test (and
delete the redundant placeholder test). Simplify SeContext since we no
longer need to construct one from a string, add the ability to extract
the type.
Along the way, I improved error reporting to give the full context for
errors (otherwise the interesting bits get omitted).
Bug: 237054515
Test: atest MicrodroidHostTestCases ComposHostTestCases
Test: test virtualizationservice_device_test
Change-Id: Ia3993a9b783b1f31bc5312af40dd5a17bf4ebfb0
diff --git a/virtualizationservice/src/aidl.rs b/virtualizationservice/src/aidl.rs
index 473cbd5..6a4cc93 100644
--- a/virtualizationservice/src/aidl.rs
+++ b/virtualizationservice/src/aidl.rs
@@ -146,7 +146,7 @@
let size = size.try_into().map_err(|e| {
Status::new_exception_str(
ExceptionCode::ILLEGAL_ARGUMENT,
- Some(format!("Invalid size {}: {}", size, e)),
+ Some(format!("Invalid size {}: {:?}", size, e)),
)
})?;
let image = clone_file(image_fd)?;
@@ -154,13 +154,13 @@
image.set_len(0).map_err(|e| {
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to reset a file: {}", e)),
+ Some(format!("Failed to reset a file: {:?}", e)),
)
})?;
let mut part = QcowFile::new(image, size).map_err(|e| {
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to create QCOW2 image: {}", e)),
+ Some(format!("Failed to create QCOW2 image: {:?}", e)),
)
})?;
@@ -175,7 +175,7 @@
.map_err(|e| {
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to initialize partition as {:?}: {}", partition_type, e)),
+ Some(format!("Failed to initialize partition as {:?}: {:?}", partition_type, e)),
)
})?;
@@ -251,7 +251,7 @@
for incoming_stream in listener.incoming() {
let mut incoming_stream = match incoming_stream {
Err(e) => {
- warn!("invalid incoming connection: {}", e);
+ warn!("invalid incoming connection: {:?}", e);
continue;
}
Ok(s) => s,
@@ -305,7 +305,7 @@
std::thread::spawn(|| {
if let Err(e) = handle_stream_connection_tombstoned() {
- warn!("Error receiving tombstone from guest or writing them. Error: {}", e);
+ warn!("Error receiving tombstone from guest or writing them. Error: {:?}", e);
}
});
@@ -366,7 +366,7 @@
Status::new_service_specific_error_str(
-1,
Some(format!(
- "Failed to create temporary directory {:?} for VM files: {}",
+ "Failed to create temporary directory {:?} for VM files: {:?}",
temporary_directory, e
)),
)
@@ -382,7 +382,7 @@
Status::new_service_specific_error_str(
-1,
Some(format!(
- "Failed to load app config from {}: {}",
+ "Failed to load app config from {}: {:?}",
&config.configPath, e
)),
)
@@ -395,29 +395,28 @@
// Check if partition images are labeled incorrectly. This is to prevent random images
// which are not protected by the Android Verified Boot (e.g. bits downloaded by apps) from
- // being loaded in a pVM. Specifically, for images in the raw config, nothing is allowed
- // to be labeled as app_data_file. For images in the app config, nothing but the instance
- // partition is allowed to be labeled as such.
+ // being loaded in a pVM. This applies to everything in the raw config, and everything but
+ // the non-executable, generated partitions in the app config.
config
.disks
.iter()
.flat_map(|disk| disk.partitions.iter())
.filter(|partition| {
if is_app_config {
- partition.label != "vm-instance"
+ !is_safe_app_partition(&partition.label)
} else {
true // all partitions are checked
}
})
.try_for_each(check_label_for_partition)
- .map_err(|e| Status::new_service_specific_error_str(-1, Some(e.to_string())))?;
+ .map_err(|e| Status::new_service_specific_error_str(-1, Some(format!("{:?}", e))))?;
let zero_filler_path = temporary_directory.join("zero.img");
write_zero_filler(&zero_filler_path).map_err(|e| {
error!("Failed to make composite image: {:?}", e);
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to make composite image: {}", e)),
+ Some(format!("Failed to make composite image: {:?}", e)),
)
})?;
@@ -442,10 +441,10 @@
// will be sent back to the client (i.e. the VM owner) for readout.
let ramdump_path = temporary_directory.join("ramdump");
let ramdump = prepare_ramdump_file(&ramdump_path).map_err(|e| {
- error!("Failed to prepare ramdump file: {}", e);
+ error!("Failed to prepare ramdump file: {:?}", e);
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to prepare ramdump file: {}", e)),
+ Some(format!("Failed to prepare ramdump file: {:?}", e)),
)
})?;
@@ -482,7 +481,7 @@
error!("Failed to create VM with config {:?}: {:?}", config, e);
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to create VM: {}", e)),
+ Some(format!("Failed to create VM: {:?}", e)),
)
})?,
);
@@ -499,7 +498,7 @@
for stream in listener.incoming() {
let stream = match stream {
Err(e) => {
- warn!("invalid incoming connection: {}", e);
+ warn!("invalid incoming connection: {:?}", e);
continue;
}
Ok(s) => s,
@@ -572,7 +571,7 @@
error!("Failed to make composite image with config {:?}: {:?}", disk, e);
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to make composite image: {}", e)),
+ Some(format!("Failed to make composite image: {:?}", e)),
)
})?;
@@ -730,10 +729,33 @@
/// Check if a partition has selinux labels that are not allowed
fn check_label_for_partition(partition: &Partition) -> Result<()> {
let ctx = getfilecon(partition.image.as_ref().unwrap().as_ref())?;
- if ctx == SeContext::new("u:object_r:app_data_file:s0").unwrap() {
- Err(anyhow!("Partition {} shouldn't be labeled as {}", &partition.label, ctx))
- } else {
- Ok(())
+ check_label_is_allowed(&ctx).with_context(|| format!("Partition {} invalid", &partition.label))
+}
+
+// Return whether a partition is exempt from selinux label checks, because we know that it does
+// not contain code and is likely to be generated in an app-writable directory.
+fn is_safe_app_partition(label: &str) -> bool {
+ // See make_payload_disk in payload.rs.
+ label == "vm-instance"
+ || label == "microdroid-apk-idsig"
+ || label == "payload-metadata"
+ || label.starts_with("extra-idsig-")
+}
+
+fn check_label_is_allowed(ctx: &SeContext) -> Result<()> {
+ // We only want to allow code in a VM payload to be sourced from places that apps, and the
+ // system, do not have write access to.
+ // (Note that sepolicy must also grant read access for these types to both virtualization
+ // service and crosvm.)
+ // App private data files are deliberately excluded, to avoid arbitrary payloads being run on
+ // user devices (W^X).
+ match ctx.selinux_type()? {
+ | "system_file" // immutable dm-verity protected partition
+ | "apk_data_file" // APKs of an installed app
+ | "staging_data_file" // updated/staged APEX imagess
+ | "shell_data_file" // test files created via adb shell
+ => Ok(()),
+ _ => bail!("Label {} is not allowed", ctx),
}
}
@@ -802,7 +824,7 @@
VsockStream::connect_with_cid_port(self.instance.cid, port as u32).map_err(|e| {
Status::new_service_specific_error_str(
-1,
- Some(format!("Failed to connect: {}", e)),
+ Some(format!("Failed to connect: {:?}", e)),
)
})?;
Ok(vsock_stream_to_pfd(stream))
@@ -881,7 +903,7 @@
let pfd = ParcelFileDescriptor::new(ramdump);
for callback in callbacks {
if let Err(e) = callback.onRamdump(cid as i32, &pfd) {
- error!("Error notifying ramdump of VM CID {}: {}", cid, e);
+ error!("Error notifying ramdump of VM CID {}: {:?}", cid, e);
}
}
}
@@ -983,7 +1005,7 @@
file.as_ref().try_clone().map_err(|e| {
Status::new_exception_str(
ExceptionCode::BAD_PARCELABLE,
- Some(format!("Failed to clone File from ParcelFileDescriptor: {}", e)),
+ Some(format!("Failed to clone File from ParcelFileDescriptor: {:?}", e)),
)
})
}
@@ -1005,7 +1027,7 @@
VersionReq::parse(s).map_err(|e| {
Status::new_exception_str(
ExceptionCode::BAD_PARCELABLE,
- Some(format!("Invalid platform version requirement {}: {}", s, e)),
+ Some(format!("Invalid platform version requirement {}: {:?}", s, e)),
)
})
}
@@ -1132,3 +1154,33 @@
)
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn test_is_allowed_label_for_partition() -> Result<()> {
+ let expected_results = vec![
+ ("u:object_r:system_file:s0", true),
+ ("u:object_r:apk_data_file:s0", true),
+ ("u:object_r:app_data_file:s0", false),
+ ("u:object_r:app_data_file:s0:c512,c768", false),
+ ("u:object_r:privapp_data_file:s0:c512,c768", false),
+ ("invalid", false),
+ ("user:role:apk_data_file:severity:categories", true),
+ ("user:role:apk_data_file:severity:categories:extraneous", false),
+ ];
+
+ for (label, expected_valid) in expected_results {
+ let context = SeContext::new(label)?;
+ let result = check_label_is_allowed(&context);
+ if expected_valid {
+ assert!(result.is_ok(), "Expected label {} to be allowed, got {:?}", label, result);
+ } else if result.is_ok() {
+ bail!("Expected label {} to be disallowed", label);
+ }
+ }
+ Ok(())
+ }
+}
diff --git a/virtualizationservice/src/main.rs b/virtualizationservice/src/main.rs
index 93a5966..cb10eff 100644
--- a/virtualizationservice/src/main.rs
+++ b/virtualizationservice/src/main.rs
@@ -72,12 +72,3 @@
}
Ok(())
}
-
-#[cfg(test)]
-mod tests {
- /// We need to have at least one test to avoid errors running the test suite, so this is a
- /// placeholder until we have real tests.
- #[test]
- #[ignore]
- fn placeholder() {}
-}
diff --git a/virtualizationservice/src/payload.rs b/virtualizationservice/src/payload.rs
index 42c51a1..7807cd6 100644
--- a/virtualizationservice/src/payload.rs
+++ b/virtualizationservice/src/payload.rs
@@ -201,7 +201,7 @@
}
/// Creates a DiskImage with partitions:
-/// metadata: metadata
+/// payload-metadata: metadata
/// microdroid-apex-0: apex 0
/// microdroid-apex-1: apex 1
/// ..
diff --git a/virtualizationservice/src/selinux.rs b/virtualizationservice/src/selinux.rs
index e450dee..0485943 100644
--- a/virtualizationservice/src/selinux.rs
+++ b/virtualizationservice/src/selinux.rs
@@ -14,7 +14,7 @@
//! Wrapper to libselinux
-use anyhow::{anyhow, Context, Result};
+use anyhow::{anyhow, bail, Context, Result};
use std::ffi::{CStr, CString};
use std::fmt;
use std::fs::File;
@@ -30,6 +30,7 @@
/// `freecon` to free the resources when dropped. In its second variant it stores
/// an `std::ffi::CString` that can be initialized from a Rust string slice.
#[derive(Debug)]
+#[allow(dead_code)] // CString variant is used in tests
pub enum SeContext {
/// Wraps a raw context c-string as returned by libselinux.
Raw(*mut ::std::os::raw::c_char),
@@ -78,12 +79,27 @@
impl SeContext {
/// Initializes the `SeContext::CString` variant from a Rust string slice.
+ #[allow(dead_code)] // Used in tests
pub fn new(con: &str) -> Result<Self> {
Ok(Self::CString(
CString::new(con)
.with_context(|| format!("Failed to create SeContext with \"{}\"", con))?,
))
}
+
+ pub fn selinux_type(&self) -> Result<&str> {
+ let context = self.deref().to_str().context("Label is not valid UTF8")?;
+
+ // The syntax is user:role:type:sensitivity[:category,...],
+ // ignoring security level ranges, which don't occur on Android. See
+ // https://github.com/SELinuxProject/selinux-notebook/blob/main/src/security_context.md
+ // We only want the type.
+ let fields: Vec<_> = context.split(':').collect();
+ if fields.len() < 4 || fields.len() > 5 {
+ bail!("Syntactically invalid label {}", self);
+ }
+ Ok(fields[2])
+ }
}
pub fn getfilecon(file: &File) -> Result<SeContext> {