From 61bb9d6a810d077929b809dc2e1e13e4c8a9878a Mon Sep 17 00:00:00 2001 From: Andrew Gunnerson Date: Wed, 11 Oct 2023 19:27:30 -0400 Subject: [PATCH] Add more checks to ensure AVB is actually providing security avbroot currently has a check to ensure that AVB signature verification isn't completely disabled by non-zero vbmeta header flags. This is done to ensure that the user isn't given a false sense of security when the OS is built in an insecure way. This commit makes a few changes better reject insecure OTAs. The first change is extending the non-zero flags check to all vbmeta images. Previously, only vbmeta images that needed to be updated as a result of patching/re-signing were checked. The second change is ensuring that there are no critical partitions present in `payload.bin`, but missing from the vbmeta descriptors. For example, if the vbmeta descriptor for `system.img` is missing, the patching process will fail with a fatal error that cannot be bypassed. Unfortunately, it's not feasible to check every partition because there is no known device where every partition is protected by AVB. Google's Tensor-based devices are the best and only have a single partition not protected by AVB: `modem`. Qualcomm-based devices have many partitions not protected by AVB. OnePlus devices have many partitions that are checked directly (against the appended header) and thus, aren't listed in any vbmeta image. Due to this, only partitions that AOSP understands are checked. OEM- specific partitions (which, security-wise, may be just as important) are not checked. The third change is ensuring that there's only a single root of trust among the vbmeta images. This way, it's impossible to have, for example, a `vbmeta_unused` image that contains all the descriptors and an empty `vbmeta` image that's actually read by the bootloader. Signed-off-by: Andrew Gunnerson --- avbroot/src/cli/ota.rs | 227 ++++++++++++++++++++++++++++++----------- 1 file changed, 167 insertions(+), 60 deletions(-) diff --git a/avbroot/src/cli/ota.rs b/avbroot/src/cli/ota.rs index be07de8..e176672 100644 --- a/avbroot/src/cli/ota.rs +++ b/avbroot/src/cli/ota.rs @@ -59,12 +59,19 @@ static PARTITION_PRIORITIES: phf::Map<&'static str, &[&'static str]> = phf_map! }; fn joined(into_iter: impl IntoIterator) -> String { - let items = into_iter - .into_iter() - .map(|i| i.to_string()) - .collect::>(); + use std::fmt::Write; - items.join(", ") + let mut result = String::new(); + + for (i, item) in into_iter.into_iter().enumerate() { + if i > 0 { + result.push_str(", "); + } + + write!(result, "{item}").expect("Failed to allocate"); + } + + result } fn sorted(iter: impl Iterator) -> Vec { @@ -236,16 +243,14 @@ fn patch_boot_images( Ok(()) } -/// From the set of input images (modified partitions + all vbmeta partitions) -/// and determine the order to patch the vbmeta images so that it can be done in -/// a single pass. -fn get_vbmeta_patch_order( +/// Load the specified vbmeta image headers. If an image has a vbmeta footer, +/// then an error is returned because the vbmeta patching logic only ever writes +/// root vbmeta images. +fn load_vbmeta_images( images: &mut HashMap>, vbmeta_images: &HashSet, -) -> Result)>> { - let mut dep_graph = HashMap::<&str, HashSet>::new(); - let mut headers = HashMap::<&str, Header>::new(); - let mut missing = images.keys().cloned().collect::>(); +) -> Result> { + let mut result = HashMap::new(); for name in vbmeta_images { let reader = images.get_mut(name).unwrap(); @@ -253,9 +258,83 @@ fn get_vbmeta_patch_order( .with_context(|| format!("Failed to load vbmeta image: {name}"))?; if let Some(f) = footer { - warning!("{name} is a vbmeta partition, but has a footer: {f:?}"); + bail!("{name} is a vbmeta partition, but has a footer: {f:?}"); } + result.insert(name.clone(), header); + } + + Ok(result) +} + +/// Check if a partition is critical to AVB's chain of trust. This is not +/// foolproof and uses a heuristic based on AOSP's boot process. OEM-specific +/// partitions may be equally important, but it's infeasible to list them all. +fn is_critical_to_avb(name: &str) -> bool { + name.ends_with("boot") + || name.starts_with("odm") + || name.starts_with("system") + || name.starts_with("vbmeta") + || name.starts_with("vendor") + || name == "dtbo" + || name == "product" + || name == "pvmfw" + || name == "recovery" +} + +/// Check that all critical partitions within the payload are protected by a +/// vbmeta image in `vbmeta_headers`. +fn ensure_partitions_protected( + manifest: &DeltaArchiveManifest, + vbmeta_headers: &HashMap, +) -> Result<()> { + let critical_partitions = manifest + .partitions + .iter() + .map(|p| &p.partition_name) + .filter(|n| is_critical_to_avb(n)) + .cloned() + .collect::>(); + + // vbmeta partitions first. + let mut avb_partitions = vbmeta_headers.keys().cloned().collect::>(); + + // Then, everything referred to by the descriptors. + for header in vbmeta_headers.values() { + let partition_names = header + .descriptors + .iter() + .filter_map(|d| d.partition_name()) + .map(|n| n.to_owned()); + + avb_partitions.extend(partition_names); + } + + let missing = critical_partitions + .difference(&avb_partitions) + .collect::>(); + + if !missing.is_empty() { + bail!( + "Found critical partitions that are not protected by AVB: {}", + joined(missing), + ); + } + + Ok(()) +} + +/// From the set of input images (modified partitions + all vbmeta partitions), +/// determine the order to patch the vbmeta images so that it can be done in a +/// single pass. +fn get_vbmeta_patch_order( + images: &mut HashMap>, + vbmeta_headers: &HashMap, +) -> Result)>> { + let mut dep_graph = HashMap::<&str, HashSet>::new(); + let mut missing = images.keys().cloned().collect::>(); + + for (name, header) in vbmeta_headers { dep_graph.insert(name, HashSet::new()); missing.remove(name); @@ -273,34 +352,31 @@ fn get_vbmeta_patch_order( missing.remove(partition_name); } } - - headers.insert(name, header); } if !missing.is_empty() { warning!("Partitions aren't protected by AVB: {:?}", joined(missing)); } - // Prune vbmeta images we don't need. - loop { - let unneeded = dep_graph - .iter() - .find(|(_, d)| d.is_empty()) - .map(|(&n, _)| n.to_owned()); - match unneeded { - Some(name) => { - dep_graph.remove(name.as_str()); - headers.remove(name.as_str()); + // Ensure that there's only a single root of trust. Otherwise, there could + // be eg. a `vbmeta_unused` containing all the relevant descriptors, but is + // never loaded by the bootloader. + let mut roots = BTreeSet::new(); - for deps in dep_graph.values_mut() { - deps.remove(name.as_str()); - } - } - None => break, + for name in vbmeta_headers.keys() { + if !dep_graph.values().any(|d| d.contains(name)) { + roots.insert(name.as_str()); } } - // Compute the patching order. This only includes vbmeta images. + // For zero roots, let TopologicalSort report the cycle. + if roots.len() > 1 { + bail!("Found multiple root vbmeta images: {}", joined(roots)); + } + + // Compute the patching order. This only includes vbmeta images. All vbmeta + // images are included (even those that have no dependencies) so that + // update_vbmeta_headers() can check and update the flags field if needed. let mut topo = TopologicalSort::::new(); let mut order = vec![]; @@ -313,13 +389,9 @@ fn get_vbmeta_patch_order( while !topo.is_empty() { match topo.pop() { Some(item) => { - // Only include vbmeta images that we need to modify. - if headers.contains_key(item.as_str()) { - order.push(( - item.clone(), - headers.remove(item.as_str()).unwrap(), - dep_graph.remove(item.as_str()).unwrap(), - )); + // Only include vbmeta images. + if dep_graph.contains_key(item.as_str()) { + order.push((item.clone(), dep_graph.remove(item.as_str()).unwrap())); } } None => bail!("vbmeta dependency graph has cycle: {topo:?}"), @@ -329,26 +401,43 @@ fn get_vbmeta_patch_order( Ok(order) } -/// Update vbmeta descriptors based on the footers from the specified images and -/// then re-sign the vbmeta images. -fn update_vbmeta_descriptors( +/// Update vbmeta headers. +/// +/// * If [`Header::flags`] is non-zero, then an error is returned because the +/// value renders AVB useless. If `clear_vbmeta_flags` is set to true, then +/// the value is set to 0 instead. +/// * [`Header::descriptors`] is updated for each dependency listed in `order`. +/// * [`Header::algorithm_type`] is updated with an algorithm type that matches +/// `key`. This is not a factor when determining if a header is changed. +/// +/// If changes were made to a vbmeta header, then the image in `images` will be +/// replaced with a new in-memory reader containing the new image. Otherwise, +/// the image is removed from `images` entirely to avoid needing to repack it. +fn update_vbmeta_headers( images: &mut HashMap>, - order: &mut [(String, Header, HashSet)], + headers: &mut HashMap, + order: &mut [(String, HashSet)], clear_vbmeta_flags: bool, key: &RsaPrivateKey, block_size: u64, ) -> Result<()> { - for (name, parent_header, deps) in order { + let mut unchanged = HashSet::new(); + + for (name, deps) in order { + let parent_header = headers.get_mut(name).unwrap(); + let orig_parent_header = parent_header.clone(); + if parent_header.flags != 0 { if clear_vbmeta_flags { parent_header.flags = 0; } else { - bail!("{name} header flags disable AVB {:#x}", parent_header.flags); + bail!( + "Verified boot is disabled by {name}'s header flags: {:#x}", + parent_header.flags, + ); } } - parent_header.set_algo_for_key(key)?; - for dep in deps.iter() { // This can't fail since the descriptor must have existed for the // dependency to exist. @@ -396,19 +485,32 @@ fn update_vbmeta_descriptors( } } - parent_header - .sign(key) - .with_context(|| format!("Failed to sign vbmeta header for image: {name}"))?; + // Only sign and rewrite the image if we need to. Some vbmeta images may + // have no dependencies and are only being processed to ensure that the + // flags are set to a sane value. + if parent_header != &orig_parent_header { + parent_header.set_algo_for_key(key)?; + parent_header + .sign(key) + .with_context(|| format!("Failed to sign vbmeta header for image: {name}"))?; - let mut writer = Cursor::new(Vec::new()); - parent_header - .to_writer(&mut writer) - .with_context(|| format!("Failed to write vbmeta image: {name}"))?; + let mut writer = Cursor::new(Vec::new()); + parent_header + .to_writer(&mut writer) + .with_context(|| format!("Failed to write vbmeta image: {name}"))?; - padding::write_zeros(&mut writer, block_size) - .with_context(|| format!("Failed to write vbmeta padding: {name}"))?; + padding::write_zeros(&mut writer, block_size) + .with_context(|| format!("Failed to write vbmeta padding: {name}"))?; - *images.get_mut(name).unwrap() = Box::new(writer); + *images.get_mut(name).unwrap() = Box::new(writer); + } else { + unchanged.insert(name.as_str()); + } + } + + // No need to package a replacement image if it's unchanged. + for name in unchanged { + images.remove(name); } Ok(()) @@ -515,11 +617,15 @@ fn patch_ota_payload( cancel_signal, )?; - let mut vbmeta_order = get_vbmeta_patch_order(&mut input_streams, &vbmeta_images)?; + let mut vbmeta_headers = load_vbmeta_images(&mut input_streams, &vbmeta_images)?; + + ensure_partitions_protected(&header_locked.manifest, &vbmeta_headers)?; + + let mut vbmeta_order = get_vbmeta_patch_order(&mut input_streams, &vbmeta_headers)?; status!( "Patching vbmeta images: {}", - joined(vbmeta_order.iter().map(|(n, _, _)| n)), + joined(vbmeta_order.iter().map(|(n, _)| n)), ); // Get rid of input readers for vbmeta partitions we don't need to modify. @@ -530,8 +636,9 @@ fn patch_ota_payload( } } - update_vbmeta_descriptors( + update_vbmeta_headers( &mut input_streams, + &mut vbmeta_headers, &mut vbmeta_order, clear_vbmeta_flags, key_avb,