From e8a137ea97dbe10f01e48f0d4b507380fedf8fe7 Mon Sep 17 00:00:00 2001 From: Francisco Javier Honduvilla Coto Date: Wed, 8 Jan 2025 15:37:07 +0000 Subject: [PATCH] feedback --- Cargo.lock | 1 + Cargo.toml | 2 + lightswitch-object/Cargo.toml | 2 +- src/unwind_info/pages.rs | 3 +- src/unwind_info/persist.rs | 123 +++++++++++++++++++++++++++------- src/unwind_info/types.rs | 5 +- 6 files changed, 110 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 22f720c..ba47d70 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1380,6 +1380,7 @@ dependencies = [ "prost", "rand", "reqwest", + "ring", "rstest", "tempfile", "thiserror 2.0.3", diff --git a/Cargo.toml b/Cargo.toml index 6503fd9..297efe1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ rand = "0.8.5" # workspace build dependencies have to be defined here libbpf-cargo = { version = "0.25.0-beta.0" } glob = "0.3.1" +ring = "0.17.8" [dependencies] gimli = "0.31.1" @@ -62,6 +63,7 @@ errno = { workspace = true } procfs = { workspace = true } nix = { workspace = true, features = ["user"] } parking_lot = { version = "0.12.3", features = ["deadlock_detection"] } +ring = { workspace = true } [dev-dependencies] assert_cmd = { version = "2.0.16" } diff --git a/lightswitch-object/Cargo.toml b/lightswitch-object/Cargo.toml index 3ce2673..9bbc92b 100644 --- a/lightswitch-object/Cargo.toml +++ b/lightswitch-object/Cargo.toml @@ -8,7 +8,7 @@ repository = "https://github.com/javierhonduco/lightswitch" [dependencies] data-encoding = "2.6.0" -ring = "0.17.8" +ring = { workspace = true } memmap2 = { workspace = true } object = { workspace = true } anyhow = { workspace = true } diff --git a/src/unwind_info/pages.rs b/src/unwind_info/pages.rs index 881aef1..03d50b0 100644 --- a/src/unwind_info/pages.rs +++ b/src/unwind_info/pages.rs @@ -130,7 +130,8 @@ mod tests { let search_here = &unwind_info[(found.index as usize)..(found.len as usize)]; let found_row = search_here.iter().find(|el| el.pc == pc).unwrap(); // And that the high and low bits were done ok - assert_eq!((found_row.pc & low_bits_mask) + pc_high, found_row.pc); + let pc = found_row.pc; + assert_eq!((pc & low_bits_mask) + pc_high, pc); } } } diff --git a/src/unwind_info/persist.rs b/src/unwind_info/persist.rs index 7c7cf82..98dec09 100644 --- a/src/unwind_info/persist.rs +++ b/src/unwind_info/persist.rs @@ -1,5 +1,9 @@ #![allow(dead_code)] use plain::Plain; +use ring::digest::{Context, SHA256}; +use std::io::Read; +use std::io::Seek; +use std::io::SeekFrom; use std::io::Write; use std::path::Path; use std::path::PathBuf; @@ -9,18 +13,30 @@ use crate::unwind_info::types::CompactUnwindRow; // To identify this binary file type. const MAGIC_NUMBER: u32 = 0x1357531; -// Any changes to the ABI must bump the version. +// Any changes to the ABI / digest must bump the version. const VERSION: u32 = 1; +type UnwindInformationDigest = u64; + #[derive(Debug, Default)] -#[repr(C)] +#[repr(C, packed)] struct Header { magic: u32, version: u32, - unwind_info_len: usize, + // To ensure that the unwind information we are reading is not + // corrupted in any way we compute a hash of the unwind information + // that is checked on the read path. + unwind_info_digest: UnwindInformationDigest, + unwind_info_len: u64, } +/// SAFETY: Using packed C representation, which plain needs, and there is +/// the extra safety layer of the unwind information digest checked in the +/// read path, in case the data is corrupted. unsafe impl Plain for Header {} +/// SAFETY: Using packed C representation, which plain needs, and there is +/// the extra safety layer of the unwind information digest checked in the +/// read path, in case the data is corrupted. unsafe impl Plain for CompactUnwindRow {} /// Writes compact information to a given writer. @@ -35,11 +51,14 @@ impl Writer { } } - fn write(self, writer: &mut impl Write) -> anyhow::Result<()> { + fn write(self, writer: &mut W) -> anyhow::Result<()> { let unwind_info = self.read_unwind_info()?; - self.write_header(writer, unwind_info.len())?; - self.write_unwind_info(writer, &unwind_info)?; - + // Write dummy header. + self.write_header(writer, 0, None)?; + let digest = self.write_unwind_info(writer, &unwind_info)?; + // Write real header. + writer.seek(SeekFrom::Start(0))?; + self.write_header(writer, unwind_info.len(), Some(digest))?; Ok(()) } @@ -47,11 +66,17 @@ impl Writer { compact_unwind_info(&self.executable_path.to_string_lossy()) } - fn write_header(&self, writer: &mut impl Write, unwind_info_len: usize) -> anyhow::Result<()> { + fn write_header( + &self, + writer: &mut impl Write, + unwind_info_len: usize, + digest: Option, + ) -> anyhow::Result<()> { let header = Header { magic: MAGIC_NUMBER, version: VERSION, - unwind_info_len, + unwind_info_digest: digest.unwrap_or(0), + unwind_info_len: unwind_info_len.try_into()?, }; writer.write_all(unsafe { plain::as_bytes(&header) })?; Ok(()) @@ -61,12 +86,19 @@ impl Writer { &self, writer: &mut impl Write, unwind_info: &[CompactUnwindRow], - ) -> anyhow::Result<()> { + ) -> anyhow::Result { + let mut context = Context::new(&SHA256); + for unwind_row in unwind_info { - writer.write_all(unsafe { plain::as_bytes(unwind_row) })?; + let unwind_row_data = unsafe { plain::as_bytes(unwind_row) }; + context.update(unwind_row_data); + writer.write_all(unwind_row_data)?; } - Ok(()) + let mut buffer = [0; 8]; + let _ = context.finish().as_ref().read(&mut buffer)?; + + Ok(u64::from_ne_bytes(buffer)) } } @@ -76,10 +108,14 @@ pub enum ReaderError { MagicNumber, #[error("version is not compatible")] Version, - #[error("error parsing raw bytes: {0}")] - ParsingError(String), + #[error("generic error: {0}")] + Generic(String), #[error("index out of range")] OutOfRange, + #[error("could not convert between types")] + SizeConversion, + #[error("digest does not match")] + Digest, } /// Reads compact information of a bytes slice. @@ -99,7 +135,7 @@ impl<'a> Reader<'a> { let mut header = Header::default(); let header_data = data.get(0..header_size).ok_or(ReaderError::OutOfRange)?; plain::copy_from_bytes(&mut header, header_data) - .map_err(|e| ReaderError::ParsingError(format!("{:?}", e)))?; + .map_err(|e| ReaderError::Generic(format!("{:?}", e)))?; if header.magic != MAGIC_NUMBER { return Err(ReaderError::MagicNumber); @@ -115,41 +151,59 @@ impl<'a> Reader<'a> { pub fn unwind_info(self) -> Result, ReaderError> { let header_size = std::mem::size_of::
(); let unwind_row_size = std::mem::size_of::(); - let unwind_info_len = self.header.unwind_info_len; + let unwind_info_len: usize = self + .header + .unwind_info_len + .try_into() + .map_err(|_| ReaderError::SizeConversion)?; let mut unwind_info = Vec::with_capacity(unwind_info_len); let mut unwind_row = CompactUnwindRow::default(); let unwind_info_data = &self.data[header_size..]; + let mut context = Context::new(&SHA256); for i in 0..unwind_info_len { let step = i * unwind_row_size; let unwind_row_data = unwind_info_data .get(step..step + unwind_row_size) .ok_or(ReaderError::OutOfRange)?; + context.update(unwind_row_data); plain::copy_from_bytes(&mut unwind_row, unwind_row_data) - .map_err(|e| ReaderError::ParsingError(format!("{:?}", e)))?; + .map_err(|e| ReaderError::Generic(format!("{:?}", e)))?; unwind_info.push(unwind_row); } + let mut buffer = [0; 8]; + let _ = context + .finish() + .as_ref() + .read(&mut buffer) + .map_err(|e| ReaderError::Generic(e.to_string())); + let digest = u64::from_ne_bytes(buffer); + + if self.header.unwind_info_digest != digest { + return Err(ReaderError::Digest); + } + Ok(unwind_info) } } #[cfg(test)] mod tests { + use std::io::Cursor; use std::path::PathBuf; use super::*; #[test] fn test_write_and_read_unwind_info() { - let mut buffer = Vec::new(); + let mut buffer = Cursor::new(Vec::new()); let path = PathBuf::from("/proc/self/exe"); let writer = Writer::new(&path); assert!(writer.write(&mut buffer).is_ok()); - let reader = Reader::new(&buffer); - assert!(reader.is_ok()); + let reader = Reader::new(&buffer.get_ref()[..]); let unwind_info = reader.unwrap().unwind_info(); assert!(unwind_info.is_ok()); let unwind_info = unwind_info.unwrap(); @@ -163,7 +217,9 @@ mod tests { magic: 0xBAD, ..Default::default() }; - buffer.write_all(unsafe { plain::as_bytes(&header) }).unwrap(); + buffer + .write_all(unsafe { plain::as_bytes(&header) }) + .unwrap(); assert!(matches!( Reader::new(&buffer), Err(ReaderError::MagicNumber) @@ -178,10 +234,28 @@ mod tests { magic: MAGIC_NUMBER, ..Default::default() }; - buffer.write_all(unsafe { plain::as_bytes(&header) }).unwrap(); + buffer + .write_all(unsafe { plain::as_bytes(&header) }) + .unwrap(); assert!(matches!(Reader::new(&buffer), Err(ReaderError::Version))); } + #[test] + fn test_corrupt_unwind_info() { + let mut buffer: Cursor> = Cursor::new(Vec::new()); + let path = PathBuf::from("/proc/self/exe"); + let writer = Writer::new(&path); + assert!(writer.write(&mut buffer).is_ok()); + + // Corrupt unwind info. + buffer.seek(SeekFrom::End(-10)).unwrap(); + buffer.write_all(&[0, 0, 0, 0, 0, 0, 0]).unwrap(); + + let reader = Reader::new(&buffer.get_ref()[..]); + let unwind_info = reader.unwrap().unwind_info(); + assert!(matches!(unwind_info, Err(ReaderError::Digest))); + } + #[test] fn test_header_too_small() { let buffer = Vec::new(); @@ -195,8 +269,11 @@ mod tests { version: VERSION, magic: MAGIC_NUMBER, unwind_info_len: 4, + unwind_info_digest: 0x0, }; - buffer.write_all(unsafe { plain::as_bytes(&header) }).unwrap(); + buffer + .write_all(unsafe { plain::as_bytes(&header) }) + .unwrap(); assert!(matches!( Reader::new(&buffer).unwrap().unwind_info(), Err(ReaderError::OutOfRange) diff --git a/src/unwind_info/types.rs b/src/unwind_info/types.rs index 68fd6c4..f27d42a 100644 --- a/src/unwind_info/types.rs +++ b/src/unwind_info/types.rs @@ -1,5 +1,8 @@ use lazy_static::lazy_static; +// Important: Any changes to the structures below must bump the file +// version in unwind_info/persist.rs + #[repr(u8)] #[derive(Debug, Default, Copy, Clone, PartialEq)] pub enum CfaType { @@ -33,7 +36,7 @@ pub enum PltType { } #[derive(Debug, Default, Copy, Clone, PartialEq)] -#[repr(C)] +#[repr(C, packed)] pub struct CompactUnwindRow { pub pc: u64, pub cfa_type: CfaType,