Merge pull request #232 from alois31/insecure-boot

stub: improve handling of insecure boot

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))

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<Boot>,
-        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(),
             );
         }
 

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<CString16> {
     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<u8> {
+    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::<LoadedImage>(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<Boot>,
     kernel_data: Vec<u8>,
-    kernel_cmdline: &CStr16,
+    kernel_cmdline: &[u8],
     initrd_data: Vec<u8>,
 ) -> uefi::Result<()> {
     let kernel =

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<Boot>) -> Status
         .expect("Failed to extract configuration from binary.")
     };
 
-    boot_linux_unchecked(
+    let secure_boot_enabled = get_secure_boot_status(system_table.runtime_services());
-        handle,
+    let cmdline = get_cmdline(
-        system_table,
-        config.kernel,
         &config.cmdline,
-        config.initrd,
+        system_table.boot_services(),
-    )
+        secure_boot_enabled,
-    .status()
+    );
+
+    boot_linux_unchecked(handle, system_table, config.kernel, &cmdline, config.initrd).status()
 }

rust/uefi/stub/src/main.rs

@@ -77,7 +77,7 @@ fn main(handle: Handle, mut system_table: SystemTable<Boot>) -> Status {
 
     #[cfg(feature = "thin")]
     {
-        status = thin::boot_linux(handle, system_table)
+        status = thin::boot_linux(handle, system_table).status()
     }
 
     status

rust/uefi/stub/src/thin.rs

@@ -1,12 +1,10 @@
-use alloc::vec::Vec;
+use log::{error, warn};
-use log::warn;
 use sha2::{Digest, Sha256};
-use uefi::fs::FileSystem;
+use uefi::{fs::FileSystem, prelude::*, CString16, Result};
-use uefi::{prelude::*, proto::loaded_image::LoadedImage, CStr16, 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::{linux_loader::InitrdLoader, uefi_helpers::booted_image_file};
+use linux_bootloader::uefi_helpers::booted_image_file;
 
 type Hash = sha2::digest::Output<Sha256>;
 
@@ -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
+/// In case of a mismatch:
-/// broken...), because the Lanzaboote tool does not sign the kernel.
+/// * 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.
-/// In essence, we can use this routine to detect whether Secure Boot
+fn check_hash(data: &[u8], expected_hash: Hash, name: &str, secure_boot: bool) -> uefi::Result<()> {
-/// is actually enabled.
+    let hash_correct = Sha256::digest(data) == expected_hash;
-fn boot_linux_uefi(
+    if !hash_correct {
-    handle: Handle,
+        if secure_boot {
-    system_table: SystemTable<Boot>,
+            error!("{name} hash does not match!");
-    kernel_data: Vec<u8>,
+            return Err(Status::SECURITY_VIOLATION.into());
-    kernel_cmdline: &CStr16,
+        } else {
-    initrd_data: Vec<u8>,
+            warn!("{name} hash does not match! Continuing anyway.");
-) -> 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::<LoadedImage>(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(),
-        );
     }
-
+    Ok(())
-    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()
 }
 
-pub fn boot_linux(handle: Handle, mut system_table: SystemTable<Boot>) -> Status {
+pub fn boot_linux(handle: Handle, mut system_table: SystemTable<Boot>) -> uefi::Result<()> {
     uefi_services::init(&mut system_table).unwrap();
 
     // SAFETY: We get a slice that represents our currently running
@@ -121,6 +91,8 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable<Boot>) -> Status
         .expect("Failed to extract configuration from binary. Did you run lzbt?")
     };
 
+    let secure_boot_enabled = get_secure_boot_status(system_table.runtime_services());
+
     let kernel_data;
     let initrd_data;
 
@@ -139,57 +111,24 @@ pub fn boot_linux(handle: Handle, mut system_table: SystemTable<Boot>) -> Status
             .expect("Failed to read initrd file into memory");
     }
 
-    let is_kernel_hash_correct = Sha256::digest(&kernel_data) == config.kernel_hash;
+    let cmdline = get_cmdline(
-    let is_initrd_hash_correct = Sha256::digest(&initrd_data) == config.initrd_hash;
+        &config.cmdline,
+        system_table.boot_services(),
+        secure_boot_enabled,
+    );
 
-    if !is_kernel_hash_correct {
+    check_hash(
-        warn!("Hash mismatch for kernel!");
+        &kernel_data,
-    }
+        config.kernel_hash,
+        "Kernel",
+        secure_boot_enabled,
+    )?;
+    check_hash(
+        &initrd_data,
+        config.initrd_hash,
+        "Initrd",
+        secure_boot_enabled,
+    )?;
 
-    if !is_initrd_hash_correct {
+    boot_linux_unchecked(handle, system_table, kernel_data, &cmdline, initrd_data)
-        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()
-    }
 }