From 52f39460424023a5601c02fec05fed6aa8f91213 Mon Sep 17 00:00:00 2001 From: Alex Franchuk Date: Wed, 7 Aug 2024 15:23:43 -0400 Subject: [PATCH 1/2] Perform checked arithmetic as a defensive measure. This also bumps the version of `goblin` to at least 0.8.2, which allows us to clean up some code. --- Cargo.toml | 2 +- src/linux/errors.rs | 3 +- src/linux/module_reader.rs | 62 ++++++++++++++++++++++++-------------- 3 files changed, 42 insertions(+), 25 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 27f90212..f7344372 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ thiserror = "1.0" [target.'cfg(unix)'.dependencies] libc = "0.2" -goblin = "0.8" +goblin = "0.8.2" memmap2 = "0.9" [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies] diff --git a/src/linux/errors.rs b/src/linux/errors.rs index f8a19cc8..86f27f36 100644 --- a/src/linux/errors.rs +++ b/src/linux/errors.rs @@ -254,10 +254,11 @@ pub enum WriterError { #[derive(Debug, Error)] pub enum ModuleReaderError { - #[error("failed to read module memory: {length} bytes at {offset}: {error}")] + #[error("failed to read module memory: {length} bytes at {offset}{}: {error}", .start_address.map(|addr| format!(" (start address: {addr})")).unwrap_or_default())] ReadModuleMemory { offset: u64, length: u64, + start_address: Option, #[source] error: nix::Error, }, diff --git a/src/linux/module_reader.rs b/src/linux/module_reader.rs index 4eac4853..446fddb4 100644 --- a/src/linux/module_reader.rs +++ b/src/linux/module_reader.rs @@ -45,32 +45,36 @@ impl<'buf> From for ProcessMemory<'buf> { impl<'buf> ProcessMemory<'buf> { #[inline] fn read(&mut self, offset: u64, length: u64) -> Result, Error> { + let error = move |start_address, error| Error::ReadModuleMemory { + start_address, + offset, + length, + error, + }; + match self { Self::Process(pr) => { - let len = std::num::NonZero::new(length as usize).ok_or_else(|| { - Error::ReadModuleMemory { - offset, - length, - error: nix::errno::Errno::EINVAL, - } - })?; + let error = |e| error(Some(pr.start_address), e); + let len = std::num::NonZero::new(length as usize) + .ok_or_else(|| error(nix::Error::EINVAL))?; + let proc_offset = pr + .start_address + .checked_add(offset) + .ok_or_else(|| error(nix::Error::EOVERFLOW))?; pr.inner - .read_to_vec((pr.start_address + offset) as _, len) + .read_to_vec(proc_offset as _, len) .map(Cow::Owned) - .map_err(|err| Error::ReadModuleMemory { - offset, - length, - error: err.source, - }) + .map_err(|err| error(err.source)) + } + Self::Slice(s) => { + let error = |e| error(None, e); + let end = offset + .checked_add(length) + .ok_or_else(|| error(nix::Error::EOVERFLOW))?; + s.get(offset as usize..end as usize) + .map(Cow::Borrowed) + .ok_or_else(|| error(nix::Error::EACCES)) } - Self::Slice(s) => s - .get(offset as usize..(offset + length) as usize) - .map(Cow::Borrowed) - .ok_or_else(|| Error::ReadModuleMemory { - offset, - length, - error: nix::Error::EACCES, - }), } } @@ -141,7 +145,7 @@ fn section_header_with_name<'sc>( Ok(None) } -/// Types which can be read from an `impl ModuleMemory`. +/// Types which can be read from ProcessMemory. pub trait ReadFromModule: Sized { fn read_from_module(module_memory: ProcessMemory<'_>) -> Result; } @@ -279,7 +283,12 @@ impl<'buf> ModuleReader<'buf> { (_, _, None) => Err(Error::NoSoNameEntry), (Some(addr), Some(size), Some(offset)) => { // If loaded in memory, the address will be altered to be absolute. - self.read_name_from_strtab(self.module_memory.absolute(addr), size, offset) + if offset < size { + self.read_name_from_strtab(self.module_memory.absolute(addr), size, offset) + } else { + log::warn!("soname strtab offset ({offset}) exceeds strtab size ({size})"); + Err(Error::NoSoNameEntry) + } } } } @@ -320,6 +329,11 @@ impl<'buf> ModuleReader<'buf> { dynstr_section_header.sh_size, name_offset, ); + } else { + log::warn!( + "soname offset ({name_offset}) exceeds dynstr section size ({})", + dynstr_section_header.sh_size + ); } } } @@ -394,6 +408,7 @@ impl<'buf> ModuleReader<'buf> { strtab_size: u64, name_offset: u64, ) -> Result { + assert!(name_offset < strtab_size); let name = self .module_memory .read(strtab_offset + name_offset, strtab_size - name_offset)?; @@ -436,6 +451,7 @@ impl<'buf> ModuleReader<'buf> { self.header.e_shoff, self.header.e_shentsize as u64 * self.header.e_shnum as u64, )?; + // Use `parse_from` rather than `parse`, which allows a 0 offset. let section_headers = elf::SectionHeader::parse_from( §ion_headers_data, 0, From 66ffdb15d597ce3f196ce7c9aec319b5ac594626 Mon Sep 17 00:00:00 2001 From: Alex Franchuk Date: Thu, 29 Aug 2024 14:29:04 -0400 Subject: [PATCH 2/2] Only use the shstrtab if it has the correct type. This avoids continuing the logic if the section header table isn't present (except for the contrived case where the random bytes happen to be the correct section type). --- src/linux/module_reader.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/linux/module_reader.rs b/src/linux/module_reader.rs index 446fddb4..fffa7514 100644 --- a/src/linux/module_reader.rs +++ b/src/linux/module_reader.rs @@ -126,7 +126,11 @@ fn section_header_with_name<'sc>( name: &[u8], module_memory: &mut ProcessMemory<'_>, ) -> Result, Error> { - let strtab_section_header = section_headers.get(strtab_index).ok_or(Error::NoStrTab)?; + let strtab_section_header = section_headers + .get(strtab_index) + .and_then(|hdr| (hdr.sh_type == elf::section_header::SHT_STRTAB).then_some(hdr)) + .ok_or(Error::NoStrTab)?; + for header in section_headers { let sh_name = header.sh_name as u64; if sh_name >= strtab_section_header.sh_size {