From db39223a7c8a479d80c266fddfcb48f277f28052 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Sun, 1 Oct 2023 19:21:11 +0200 Subject: [PATCH 1/2] stub: make handling of insecure boot more explicit When Secure Boot is not available (unsupported or disabled), Lanzaboote will attempt to boot kernels and initrds even when they fail the hash verification. Previously, this would happen by falling back to use LoadImage on the kernel, which fails if Secure Boot is available, as the kernel is not signed. The SecureBoot variable offers a more explicit way of checking whether Secure Boot is available. If the firmware supports Secure Boot, it initializes this variable to 1 if it is enabled, and to 0 if it is disabled. Applications are not supposed to modify this variable, and in particular, since only trusted applications are loaded when Secure Boot is active, we can assume it is never changed to 0 or deleted if Secure Boot is active. Hence, we can be sure of Secure Boot being inactive if this variable is absent or set to 0, and thus treat all hash verification errors as non-fatal and proceed to boot arbitrary kernels and initrds (a warning is still logged in this case). In all other cases, we treat all hash verification failures as fatal security violations, as it must be done in the case where Secure Boot is active (it is expected that this does not lead to any false positives in practice, unless there are bigger problems anyway). --- nix/tests/lanzaboote.nix | 2 +- rust/uefi/stub/src/main.rs | 2 +- rust/uefi/stub/src/thin.rs | 171 +++++++++++++++---------------------- 3 files changed, 73 insertions(+), 102 deletions(-) diff --git a/nix/tests/lanzaboote.nix b/nix/tests/lanzaboote.nix index d9e6458..e1888d3 100644 --- a/nix/tests/lanzaboote.nix +++ b/nix/tests/lanzaboote.nix @@ -144,7 +144,7 @@ let machine.crash() machine.start() '' + (if useSecureBoot then '' - machine.wait_for_console_text("Hash mismatch") + machine.wait_for_console_text("hash does not match") '' else '' # Just check that the system came up. print(machine.succeed("bootctl", timeout=120)) diff --git a/rust/uefi/stub/src/main.rs b/rust/uefi/stub/src/main.rs index 21254bd..bc5c6de 100644 --- a/rust/uefi/stub/src/main.rs +++ b/rust/uefi/stub/src/main.rs @@ -77,7 +77,7 @@ fn main(handle: Handle, mut system_table: SystemTable) -> Status { #[cfg(feature = "thin")] { - status = thin::boot_linux(handle, system_table) + status = thin::boot_linux(handle, system_table).status() } status diff --git a/rust/uefi/stub/src/thin.rs b/rust/uefi/stub/src/thin.rs index 13fa10b..bd431ea 100644 --- a/rust/uefi/stub/src/thin.rs +++ b/rust/uefi/stub/src/thin.rs @@ -1,12 +1,10 @@ -use alloc::vec::Vec; -use log::warn; +use log::{error, warn}; use sha2::{Digest, Sha256}; -use uefi::fs::FileSystem; -use uefi::{prelude::*, proto::loaded_image::LoadedImage, CStr16, CString16, Result}; +use uefi::{fs::FileSystem, guid, prelude::*, table::runtime::VariableVendor, CString16, Result}; use crate::common::{boot_linux_unchecked, extract_string}; use linux_bootloader::pe_section::pe_section; -use linux_bootloader::{linux_loader::InitrdLoader, uefi_helpers::booted_image_file}; +use linux_bootloader::uefi_helpers::booted_image_file; type Hash = sha2::digest::Output; @@ -59,53 +57,25 @@ impl EmbeddedConfiguration { } } -/// Boot the Linux kernel via the UEFI PE loader. +/// Verify some data against its expected hash. /// -/// This should only succeed when UEFI Secure Boot is off (or -/// broken...), because the Lanzaboote tool does not sign the kernel. -/// -/// In essence, we can use this routine to detect whether Secure Boot -/// is actually enabled. -fn boot_linux_uefi( - handle: Handle, - system_table: SystemTable, - kernel_data: Vec, - kernel_cmdline: &CStr16, - initrd_data: Vec, -) -> uefi::Result<()> { - let kernel_handle = system_table.boot_services().load_image( - handle, - uefi::table::boot::LoadImageSource::FromBuffer { - buffer: &kernel_data, - file_path: None, - }, - )?; - - let mut kernel_image = system_table - .boot_services() - .open_protocol_exclusive::(kernel_handle)?; - - unsafe { - kernel_image.set_load_options( - kernel_cmdline.as_ptr() as *const u8, - // This unwrap is "safe" in the sense that any - // command-line that doesn't fit 4G is surely broken. - u32::try_from(kernel_cmdline.num_bytes()).unwrap(), - ); +/// In case of a mismatch: +/// * If Secure Boot is active, an error message is logged, and the SECURITY_VIOLATION error is returned to stop the boot. +/// * If Secure Boot is not active, only a warning is logged, and the boot process is allowed to continue. +fn check_hash(data: &[u8], expected_hash: Hash, name: &str, secure_boot: bool) -> uefi::Result<()> { + let hash_correct = Sha256::digest(data) == expected_hash; + if !hash_correct { + if secure_boot { + error!("{name} hash does not match!"); + return Err(Status::SECURITY_VIOLATION.into()); + } else { + warn!("{name} hash does not match! Continuing anyway."); + } } - - let mut initrd_loader = InitrdLoader::new(system_table.boot_services(), handle, initrd_data)?; - - let status = system_table - .boot_services() - .start_image(kernel_handle) - .status(); - - initrd_loader.uninstall(system_table.boot_services())?; - status.to_result() + Ok(()) } -pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> Status { +pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> uefi::Result<()> { uefi_services::init(&mut system_table).unwrap(); // SAFETY: We get a slice that represents our currently running @@ -121,6 +91,40 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> Status .expect("Failed to extract configuration from binary. Did you run lzbt?") }; + // The firmware initialized SecureBoot to 1 if performing signature checks, and 0 if it doesn't. + // Applications are not supposed to modify this variable (in particular, don't change the value from 1 to 0). + let secure_boot_enabled = system_table + .runtime_services() + .get_variable( + cstr16!("SecureBoot"), + &VariableVendor(guid!("8be4df61-93ca-11d2-aa0d-00e098032b8c")), + &mut [1], + ) + .and_then(|(value, _)| match value { + [0] => Ok(false), + [1] => Ok(true), + [v] => { + warn!( + "Unexpected value of SecureBoot variable: {v}. Performing verification anyway." + ); + Ok(true) + } + _ => Err(Status::BAD_BUFFER_SIZE.into()), + }) + .unwrap_or_else(|err| { + if err.status() == Status::NOT_FOUND { + warn!("SecureBoot variable not found. Assuming Secure Boot is not supported."); + false + } else { + warn!("Failed to read SecureBoot variable: {err}. Performing verification anyway."); + true + } + }); + + if !secure_boot_enabled { + warn!("Secure Boot is not active!"); + } + let kernel_data; let initrd_data; @@ -139,57 +143,24 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> Status .expect("Failed to read initrd file into memory"); } - let is_kernel_hash_correct = Sha256::digest(&kernel_data) == config.kernel_hash; - let is_initrd_hash_correct = Sha256::digest(&initrd_data) == config.initrd_hash; + check_hash( + &kernel_data, + config.kernel_hash, + "Kernel", + secure_boot_enabled, + )?; + check_hash( + &initrd_data, + config.initrd_hash, + "Initrd", + secure_boot_enabled, + )?; - if !is_kernel_hash_correct { - warn!("Hash mismatch for kernel!"); - } - - if !is_initrd_hash_correct { - warn!("Hash mismatch for initrd!"); - } - - if is_kernel_hash_correct && is_initrd_hash_correct { - boot_linux_unchecked( - handle, - system_table, - kernel_data, - &config.cmdline, - initrd_data, - ) - .status() - } else { - // There is no good way to detect whether Secure Boot is - // enabled. This is unfortunate, because we want to give the - // user a way to recover from hash mismatches when Secure Boot - // is off. - // - // So in case we get a hash mismatch, we will try to load the - // Linux image using LoadImage. What happens then depends on - // whether Secure Boot is enabled: - // - // **With Secure Boot**, the firmware will reject loading the - // image with status::SECURITY_VIOLATION. - // - // **Without Secure Boot**, the firmware will just load the - // Linux kernel. - // - // This is the behavior we want. A slight turd is that we - // increase the attack surface here by exposing the unverfied - // Linux image to the UEFI firmware. But in case the PE loader - // of the firmware is broken, we have little hope of security - // anyway. - - warn!("Trying to continue as non-Secure Boot. This will fail when Secure Boot is enabled."); - - boot_linux_uefi( - handle, - system_table, - kernel_data, - &config.cmdline, - initrd_data, - ) - .status() - } + boot_linux_unchecked( + handle, + system_table, + kernel_data, + &config.cmdline, + initrd_data, + ) } From b02a7e2a7f5f91054ac1dbac3d271cfd613698a2 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 2 Oct 2023 08:33:04 +0200 Subject: [PATCH 2/2] stub: use command line from loader in insecure mode When booting without Secure Boot active, it is not necessary to defend against a malicious command line being passed from the loader. So just use it in this case, to facilitaty some debugging and recovery use cases. Fixes: https://github.com/nix-community/lanzaboote/issues/226 --- rust/uefi/linux-bootloader/src/pe_loader.rs | 6 +- rust/uefi/stub/src/common.rs | 72 ++++++++++++++++++++- rust/uefi/stub/src/fat.rs | 16 ++--- rust/uefi/stub/src/thin.rs | 52 +++------------ 4 files changed, 91 insertions(+), 55 deletions(-) diff --git a/rust/uefi/linux-bootloader/src/pe_loader.rs b/rust/uefi/linux-bootloader/src/pe_loader.rs index 4a39039..c500813 100644 --- a/rust/uefi/linux-bootloader/src/pe_loader.rs +++ b/rust/uefi/linux-bootloader/src/pe_loader.rs @@ -9,7 +9,7 @@ use uefi::{ boot::{AllocateType, MemoryType}, Boot, SystemTable, }, - CStr16, Handle, Status, + Handle, Status, }; /// UEFI mandates 4 KiB pages. @@ -174,7 +174,7 @@ impl Image { self, handle: Handle, system_table: &SystemTable, - load_options: &CStr16, + load_options: &[u8], ) -> Status { let mut loaded_image = system_table .boot_services() @@ -196,7 +196,7 @@ impl Image { ); loaded_image.set_load_options( load_options.as_ptr() as *const u8, - u32::try_from(load_options.num_bytes()).unwrap(), + u32::try_from(load_options.len()).unwrap(), ); } diff --git a/rust/uefi/stub/src/common.rs b/rust/uefi/stub/src/common.rs index aaee529..e8fe20c 100644 --- a/rust/uefi/stub/src/common.rs +++ b/rust/uefi/stub/src/common.rs @@ -1,5 +1,9 @@ use alloc::vec::Vec; -use uefi::{prelude::*, CStr16, CString16, Result}; +use log::warn; +use uefi::{ + guid, prelude::*, proto::loaded_image::LoadedImage, table::runtime::VariableVendor, CStr16, + CString16, Result, +}; use linux_bootloader::linux_loader::InitrdLoader; use linux_bootloader::pe_loader::Image; @@ -12,6 +16,70 @@ pub fn extract_string(pe_data: &[u8], section: &str) -> Result { Ok(CString16::try_from(string.as_str()).map_err(|_| Status::INVALID_PARAMETER)?) } +/// Obtain the kernel command line that should be used for booting. +/// +/// If Secure Boot is active, this is always the embedded one (since the one passed from the bootloader may come from a malicious type 1 entry). +/// If Secure Boot is not active, the command line passed from the bootloader is used, falling back to the embedded one. +pub fn get_cmdline( + embedded: &CStr16, + boot_services: &BootServices, + secure_boot_enabled: bool, +) -> Vec { + if secure_boot_enabled { + // The command line passed from the bootloader cannot be trusted, so it is not used when Secure Boot is active. + embedded.as_bytes().to_vec() + } else { + let passed = boot_services + .open_protocol_exclusive::(boot_services.image_handle()) + .map(|loaded_image| loaded_image.load_options_as_bytes().map(|b| b.to_vec())); + match passed { + Ok(Some(passed)) => passed, + // If anything went wrong, fall back to the embedded command line. + _ => embedded.as_bytes().to_vec(), + } + } +} + +/// Check whether Secure Boot is active, and we should be enforcing integrity checks. +/// +/// In case of doubt, true is returned to be on the safe side. +pub fn get_secure_boot_status(runtime_services: &RuntimeServices) -> bool { + // The firmware initialized SecureBoot to 1 if performing signature checks, and 0 if it doesn't. + // Applications are not supposed to modify this variable (in particular, don't change the value from 1 to 0). + let secure_boot_enabled = runtime_services + .get_variable( + cstr16!("SecureBoot"), + &VariableVendor(guid!("8be4df61-93ca-11d2-aa0d-00e098032b8c")), + &mut [1], + ) + .and_then(|(value, _)| match value { + [0] => Ok(false), + [1] => Ok(true), + [v] => { + warn!( + "Unexpected value of SecureBoot variable: {v}. Performing verification anyway." + ); + Ok(true) + } + _ => Err(Status::BAD_BUFFER_SIZE.into()), + }) + .unwrap_or_else(|err| { + if err.status() == Status::NOT_FOUND { + warn!("SecureBoot variable not found. Assuming Secure Boot is not supported."); + false + } else { + warn!("Failed to read SecureBoot variable: {err}. Performing verification anyway."); + true + } + }); + + if !secure_boot_enabled { + warn!("Secure Boot is not active!"); + } + + secure_boot_enabled +} + /// Boot the Linux kernel without checking the PE signature. /// /// We assume that the caller has made sure that the image is safe to @@ -20,7 +88,7 @@ pub fn boot_linux_unchecked( handle: Handle, system_table: SystemTable, kernel_data: Vec, - kernel_cmdline: &CStr16, + kernel_cmdline: &[u8], initrd_data: Vec, ) -> uefi::Result<()> { let kernel = diff --git a/rust/uefi/stub/src/fat.rs b/rust/uefi/stub/src/fat.rs index 2cbc053..cce30d3 100644 --- a/rust/uefi/stub/src/fat.rs +++ b/rust/uefi/stub/src/fat.rs @@ -1,7 +1,7 @@ use alloc::vec::Vec; use uefi::{prelude::*, CString16, Result}; -use crate::common::{boot_linux_unchecked, extract_string}; +use crate::common::{boot_linux_unchecked, extract_string, get_cmdline, get_secure_boot_status}; use linux_bootloader::pe_section::pe_section; use linux_bootloader::uefi_helpers::booted_image_file; @@ -56,12 +56,12 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> Status .expect("Failed to extract configuration from binary.") }; - boot_linux_unchecked( - handle, - system_table, - config.kernel, + let secure_boot_enabled = get_secure_boot_status(system_table.runtime_services()); + let cmdline = get_cmdline( &config.cmdline, - config.initrd, - ) - .status() + system_table.boot_services(), + secure_boot_enabled, + ); + + boot_linux_unchecked(handle, system_table, config.kernel, &cmdline, config.initrd).status() } diff --git a/rust/uefi/stub/src/thin.rs b/rust/uefi/stub/src/thin.rs index bd431ea..bf91c5e 100644 --- a/rust/uefi/stub/src/thin.rs +++ b/rust/uefi/stub/src/thin.rs @@ -1,8 +1,8 @@ use log::{error, warn}; use sha2::{Digest, Sha256}; -use uefi::{fs::FileSystem, guid, prelude::*, table::runtime::VariableVendor, CString16, Result}; +use uefi::{fs::FileSystem, prelude::*, CString16, Result}; -use crate::common::{boot_linux_unchecked, extract_string}; +use crate::common::{boot_linux_unchecked, extract_string, get_cmdline, get_secure_boot_status}; use linux_bootloader::pe_section::pe_section; use linux_bootloader::uefi_helpers::booted_image_file; @@ -91,39 +91,7 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> uefi:: .expect("Failed to extract configuration from binary. Did you run lzbt?") }; - // The firmware initialized SecureBoot to 1 if performing signature checks, and 0 if it doesn't. - // Applications are not supposed to modify this variable (in particular, don't change the value from 1 to 0). - let secure_boot_enabled = system_table - .runtime_services() - .get_variable( - cstr16!("SecureBoot"), - &VariableVendor(guid!("8be4df61-93ca-11d2-aa0d-00e098032b8c")), - &mut [1], - ) - .and_then(|(value, _)| match value { - [0] => Ok(false), - [1] => Ok(true), - [v] => { - warn!( - "Unexpected value of SecureBoot variable: {v}. Performing verification anyway." - ); - Ok(true) - } - _ => Err(Status::BAD_BUFFER_SIZE.into()), - }) - .unwrap_or_else(|err| { - if err.status() == Status::NOT_FOUND { - warn!("SecureBoot variable not found. Assuming Secure Boot is not supported."); - false - } else { - warn!("Failed to read SecureBoot variable: {err}. Performing verification anyway."); - true - } - }); - - if !secure_boot_enabled { - warn!("Secure Boot is not active!"); - } + let secure_boot_enabled = get_secure_boot_status(system_table.runtime_services()); let kernel_data; let initrd_data; @@ -143,6 +111,12 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> uefi:: .expect("Failed to read initrd file into memory"); } + let cmdline = get_cmdline( + &config.cmdline, + system_table.boot_services(), + secure_boot_enabled, + ); + check_hash( &kernel_data, config.kernel_hash, @@ -156,11 +130,5 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable) -> uefi:: secure_boot_enabled, )?; - boot_linux_unchecked( - handle, - system_table, - kernel_data, - &config.cmdline, - initrd_data, - ) + boot_linux_unchecked(handle, system_table, kernel_data, &cmdline, initrd_data) }