From f726fbb1989f18bf7f15c5c6db518d4e9de85bcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 15:59:53 +0100 Subject: [PATCH 01/10] feat: the object namespace, walked by name rather than printed `\Device\MountPointManager` is not an address, and nothing here could turn it into one: a symbol names what the linker placed, while an object is created at run time and filed under a name in a tree the object manager keeps. The debugger's answer is `!object`, an extension printing text. This is the same walk answering in values, which is what a caller reading a device's security descriptor needs -- the descriptor pointer is in the object's header, and the header is only reachable once the name has been resolved. `object_at` resolves a path, `objects_in` lists a directory, and `symbolic_link_target` reads where a link points. **Every bucket is walked rather than the one the name hashes to.** The hash is the object manager's own, over a name folded with the kernel's upcase table, and a wrong reimplementation of it does not fail -- it looks in the wrong bucket and reports that the object is not there, which is an answer a caller acts on. A directory holds tens of entries, so the saving is not worth that risk. **Not one literal offset.** `_OBJECT_HEADER` has moved between Windows versions, so every field comes from the target's own type information -- and two things that look like constants are derived rather than assumed. A pointer's width is the distance between a directory entry's two pointer fields, because a 32-bit kernel read from this host would otherwise be decoded with the host's width and every read after it would run off the end of something. The bucket count is the array's span over that width, because 37 is this build's number and not the structure's. **A cap is an error rather than a short list**, which is the same rule the security descriptor reader in the consumer follows: a directory reported shorter than it is answers "which symbolic links reach this device" wrongly, and that is the one direction a security question must not fail in. A chain that points at itself is refused rather than walked. Two subtleties the tests pin by mutation, one at a time: - The header's `SecurityDescriptor` keeps three object-manager flags in its low bits. Taken as an address it reads a descriptor three bytes into its own header and reports a DACL that is not there. - `TypeIndex` is exclusive-ored with a per-boot cookie **and with a byte of the header's own address**. Dropping either term reads a type out of the wrong table slot, which is how a device would read as a directory and be walked through. `LinkTarget` shares its storage with a callback pointer: `Callback` lands exactly on `Length` and `MaximumLength`, and `CallbackContext` lands on `Buffer`. So the string is checked before it is followed, and the test lays that union out as it really is rather than relying on a read that happens to fail. This does not work on a kernel minidump. Measured against the consumer's checked-in dump: `nt!ObpRootDirectoryObject` reads `????????`, so the walk stops at its first read and names the address rather than reporting an empty namespace. A live kernel is this code's tier. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/lib.rs | 1 + src/object.rs | 896 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 897 insertions(+) create mode 100644 src/object.rs diff --git a/src/lib.rs b/src/lib.rs index 776398f..2099ddc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -115,5 +115,6 @@ pub mod allocator; pub mod dbgeng; pub mod heap; +pub mod object; pub mod pool; mod pool_extension; diff --git a/src/object.rs b/src/object.rs new file mode 100644 index 0000000..b83314a --- /dev/null +++ b/src/object.rs @@ -0,0 +1,896 @@ +//! The kernel **object namespace**, walked by name. +//! +//! `\Device\MountPointManager` is not an address, and nothing in [`crate::dbgeng`] could turn it +//! into one: a symbol names code and data the linker placed, while an object is created at run +//! time and filed under a name in a tree the object manager keeps. The debugger's own answer to +//! this is `!object`, which is an extension command printing text; this is the same walk answering +//! in values. +//! +//! # What the walk is +//! +//! `nt!ObpRootDirectoryObject` points at the root [`_OBJECT_DIRECTORY`]. A directory is an array +//! of hash buckets, each a chain of `_OBJECT_DIRECTORY_ENTRY`, each pointing at an object **body**. +//! The name of a body is not in the body: it is in an `_OBJECT_HEADER_NAME_INFO` that sits *before* +//! the `_OBJECT_HEADER`, present only when the header's `InfoMask` says so, at a distance the +//! kernel looks up in `nt!ObpInfoMaskToOffset`. So resolving one path component means enumerating +//! a directory and reading a name out from under every object in it. +//! +//! **Every bucket is walked rather than the one the name hashes to.** The hash is the object +//! manager's own, over a case-folded name using the kernel's upcase table, and a wrong reimplementation +//! of it does not fail — it looks in the wrong bucket and reports that the object does not exist, +//! which is the answer a caller would act on. A directory holds tens of entries, so walking all of +//! them costs nothing worth having that risk for. +//! +//! # What it refuses +//! +//! Every list is capped, and a cap is an **error rather than a short list**. A namespace is data +//! this crate did not write: a corrupt chain is a cycle, and a directory reported shorter than it +//! is answers "which symbolic links point here" wrongly, which is the one thing a security question +//! must not do. +//! +//! Names are compared with an **ASCII** case fold, which is what device and directory names are in +//! practice and is stated rather than hidden: the kernel folds with its own upcase table, so a name +//! differing only outside ASCII compares unequal here where the object manager would match it. +//! +//! # Where it works +//! +//! A live kernel, and a kernel dump complete enough to carry `nt`'s data pages. It does **not** +//! work on a kernel minidump: measured against `docs/samples/081226-2187-01.dmp` in the consumer, +//! `nt!ObpRootDirectoryObject` itself reads `????????`, so the walk stops at its first read and +//! says so rather than reporting an empty namespace. + +use thiserror::Error; + +use crate::dbgeng::{DbgEngError, DebugEngine}; + +/// The most entries one directory may hold before the walk refuses it. +/// +/// `\GLOBAL??` on a busy machine holds a few thousand; this is well above that and far below a +/// chain that has looped. It bounds the whole directory rather than one bucket, because a cycle +/// can be spread across buckets as easily as kept inside one. +const MAX_ENTRIES: usize = 65_536; + +/// The most path components a name may have. `\Device\HarddiskVolume1` is two. +const MAX_COMPONENTS: usize = 32; + +/// The longest object name this reads, in bytes of UTF-16. +/// +/// `_UNICODE_STRING::Length` is a `USHORT`, so the structure's own limit is 64 KiB; a name that +/// long is not one the object manager made. +const MAX_NAME_BYTES: usize = 1024; + +/// Why a namespace walk could not answer. +#[derive(Debug, Error, PartialEq, Eq)] +pub enum ObjectError { + /// Target memory that would not read. On a kernel minidump this is the first thing that + /// happens, and it names the address so that the answer is "this target has no namespace" + /// rather than "this namespace is empty". + #[error("could not read {len} bytes of the object namespace at {at:#x}")] + Unreadable { at: u64, len: usize }, + /// A structure whose fields contradict themselves. + #[error("the object namespace is malformed: {reason}")] + Malformed { reason: &'static str }, + /// A path that is not one the object manager could have filed anything under. + #[error("{path:?} is not an object path: {reason}")] + BadPath { path: String, reason: &'static str }, + /// Every component before this one resolved, and this one is not in its directory. + #[error("{component:?} is not in {directory:?}")] + NotFound { + directory: String, + component: String, + }, + /// A component resolved to something that is not a directory, with path left to walk. + #[error("{component:?} is not a directory, so {rest:?} cannot be under it")] + NotADirectory { component: String, rest: String }, + /// A cap was reached, so what this could answer with is a **short** list. + #[error("{what} exceeded its bound of {bound}, so this list would be shorter than the truth")] + TooMany { what: &'static str, bound: usize }, +} + +/// One object, as much of it as the namespace says. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct KernelObject { + /// The object **body** — what a handle resolves to, and what `!devobj` and friends take. + pub address: u64, + /// Its name in the directory that holds it, not a path. + pub name: String, + /// The object type's name (`Device`, `SymbolicLink`, `Directory`), when the type table could + /// be read. `None` is "this walk could not say", never "untyped". + pub type_name: Option, + /// `_OBJECT_HEADER::SecurityDescriptor`, with the three low bits the object manager keeps its + /// own flags in masked off. `None` where the object carries none. + pub security_descriptor: Option, +} + +/// Where the fields this walk reads live, taken from the **target's own type information**. +/// +/// Not one literal offset, and that is the point: `_OBJECT_HEADER` has moved between Windows +/// versions and `_OBJECT_DIRECTORY`'s bucket count is a build's choice. A table of constants here +/// would decode a different build confidently and wrongly. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Layout { + /// A pointer's width on this target, **derived** from the distance between the two pointer + /// fields of a directory entry rather than assumed from the host. + pub pointer: usize, + /// How many hash buckets a directory has, derived the same way: the span from the array to the + /// field after it, over a pointer. + pub buckets: usize, + /// `_OBJECT_DIRECTORY::HashBuckets`. + pub hash_buckets: u32, + /// `_OBJECT_DIRECTORY_ENTRY::ChainLink` and `::Object`. + pub entry_chain: u32, + pub entry_object: u32, + /// `_OBJECT_HEADER::Body`, which is how far *back* a body's header is. + pub header_body: u32, + pub header_type_index: u32, + pub header_info_mask: u32, + pub header_security: u32, + /// `_OBJECT_HEADER_NAME_INFO::Name`, and the structure's size. + pub name_info_name: u32, + pub name_info_size: u32, + /// `_UNICODE_STRING::Length` and `::Buffer`. + pub unicode_length: u32, + pub unicode_buffer: u32, + /// `_OBJECT_SYMBOLIC_LINK::LinkTarget`. + pub link_target: u32, + /// `_OBJECT_TYPE::Name`. + pub type_name: u32, +} + +/// The globals the walk starts from, resolved by symbol. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Globals { + /// `nt!ObpRootDirectoryObject` — a pointer to the root directory, not the directory. + pub root: u64, + /// `nt!ObpInfoMaskToOffset` — a byte per `InfoMask` combination, saying how far before the + /// header the optional headers sit. + pub info_mask_to_offset: u64, + /// `nt!ObHeaderCookie` and `nt!ObTypeIndexTable`, which together turn a header's obfuscated + /// `TypeIndex` into a type object. Optional: a build without them still resolves names, and a + /// `type_name` of `None` is the honest answer there. + pub header_cookie: Option, + pub type_index_table: Option, +} + +/// Which optional header is wanted, as the bit the kernel's own lookup is keyed on. +const INFO_MASK_NAME: u8 = 0x02; + +/// Reads a walk's worth of target memory. A trait so the walk is testable against bytes, which is +/// the only way to test it at all: the namespace it walks lives in a running kernel. +pub trait Memory { + /// Answers `None` for anything that would not read, at any length. + fn read(&self, address: u64, len: usize) -> Option>; +} + +impl Memory for F +where + F: Fn(u64, usize) -> Option>, +{ + fn read(&self, address: u64, len: usize) -> Option> { + self(address, len) + } +} + +/// The namespace, over some memory and a layout. +pub struct Namespace<'a> { + memory: &'a dyn Memory, + layout: Layout, + globals: Globals, +} + +impl<'a> Namespace<'a> { + pub fn new(memory: &'a dyn Memory, layout: Layout, globals: Globals) -> Self { + Self { + memory, + layout, + globals, + } + } + + fn read(&self, at: u64, len: usize) -> Result, ObjectError> { + match self.memory.read(at, len) { + Some(bytes) if bytes.len() >= len => Ok(bytes), + _ => Err(ObjectError::Unreadable { at, len }), + } + } + + fn pointer_at(&self, at: u64) -> Result { + let bytes = self.read(at, self.layout.pointer)?; + Ok(match self.layout.pointer { + 4 => u64::from(u32::from_le_bytes( + bytes[..4].try_into().unwrap_or_default(), + )), + _ => u64::from_le_bytes(bytes[..8].try_into().unwrap_or_default()), + }) + } + + /// The object body every entry of a directory points at. + fn entries_of(&self, directory: u64) -> Result, ObjectError> { + let mut out = Vec::new(); + for bucket in 0..self.layout.buckets { + let at = directory + .wrapping_add(u64::from(self.layout.hash_buckets)) + .wrapping_add((bucket * self.layout.pointer) as u64); + let mut entry = self.pointer_at(at)?; + // A chain is bounded by the whole directory's bound rather than one of its own: a + // cycle inside a bucket and a cycle across buckets are the same corruption. + while entry != 0 { + if out.len() >= MAX_ENTRIES { + return Err(ObjectError::TooMany { + what: "a directory's entries", + bound: MAX_ENTRIES, + }); + } + let object = + self.pointer_at(entry.wrapping_add(u64::from(self.layout.entry_object)))?; + if object != 0 { + out.push(object); + } + entry = self.pointer_at(entry.wrapping_add(u64::from(self.layout.entry_chain)))?; + } + } + Ok(out) + } + + /// The header that belongs to an object body. + fn header_of(&self, body: u64) -> u64 { + body.wrapping_sub(u64::from(self.layout.header_body)) + } + + /// One `_UNICODE_STRING`, read from wherever it sits. + fn unicode_at(&self, at: u64) -> Result { + let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; + let bytes = self.read(at, size)?; + let length = u16::from_le_bytes( + bytes[self.layout.unicode_length as usize..][..2] + .try_into() + .unwrap_or_default(), + ) as usize; + let buffer = self.pointer_at(at.wrapping_add(u64::from(self.layout.unicode_buffer)))?; + if length == 0 || buffer == 0 { + return Ok(String::new()); + } + if !length.is_multiple_of(2) { + return Err(ObjectError::Malformed { + reason: "a UNICODE_STRING's length is not a whole number of UTF-16 units", + }); + } + if length > MAX_NAME_BYTES { + return Err(ObjectError::TooMany { + what: "an object name", + bound: MAX_NAME_BYTES, + }); + } + let raw = self.read(buffer, length)?; + Ok(utf16(&raw[..length])) + } + + /// An object's own name, which lives in an optional header before its header. + /// + /// `Ok(None)` is an object filed under no name at all, which is ordinary — most objects are + /// reached by handle and never named. + fn name_of(&self, body: u64) -> Result, ObjectError> { + let header = self.header_of(body); + let mask = self.read( + header.wrapping_add(u64::from(self.layout.header_info_mask)), + 1, + )?[0]; + if mask & INFO_MASK_NAME == 0 { + return Ok(None); + } + // The kernel's own lookup: the offsets of every optional header present *up to and + // including* the one wanted, which is what the bit and every bit below it select. + let index = mask & (INFO_MASK_NAME | (INFO_MASK_NAME - 1)); + let distance = self.read( + self.globals + .info_mask_to_offset + .wrapping_add(u64::from(index)), + 1, + )?[0]; + if u32::from(distance) < self.layout.name_info_size { + return Err(ObjectError::Malformed { + reason: "the name header's distance is shorter than the name header", + }); + } + let name_info = header.wrapping_sub(u64::from(distance)); + Ok(Some(self.unicode_at( + name_info.wrapping_add(u64::from(self.layout.name_info_name)), + )?)) + } + + /// The security descriptor an object carries, with the object manager's flag bits cleared. + fn security_of(&self, body: u64) -> Result, ObjectError> { + let at = self + .header_of(body) + .wrapping_add(u64::from(self.layout.header_security)); + // The low three bits are the object manager's own, never part of the address. + let descriptor = self.pointer_at(at)? & !0b111; + Ok((descriptor != 0).then_some(descriptor)) + } + + /// The name of an object's type, when the type table can be read. + /// + /// The index in the header is obfuscated — exclusive-ored with a per-boot cookie and with a + /// byte of the header's own address — so a build whose cookie this cannot find gets `None` + /// rather than a type read out of the wrong table slot. + fn type_of(&self, body: u64) -> Option { + let (cookie, table) = (self.globals.header_cookie?, self.globals.type_index_table?); + let header = self.header_of(body); + let raw = self + .read( + header.wrapping_add(u64::from(self.layout.header_type_index)), + 1, + ) + .ok()?[0]; + let cookie = self.read(cookie, 1).ok()?[0]; + let index = raw ^ cookie ^ ((header >> 8) as u8); + let entry = self + .pointer_at(table.wrapping_add((usize::from(index) * self.layout.pointer) as u64)) + .ok()?; + if entry == 0 { + return None; + } + let name = self + .unicode_at(entry.wrapping_add(u64::from(self.layout.type_name))) + .ok()?; + (!name.is_empty()).then_some(name) + } + + /// Everything one directory holds, named. + fn named_in(&self, directory: u64) -> Result, ObjectError> { + let mut out = Vec::new(); + for body in self.entries_of(directory)? { + let Some(name) = self.name_of(body)? else { + continue; + }; + out.push(KernelObject { + address: body, + name, + type_name: self.type_of(body), + security_descriptor: self.security_of(body)?, + }); + } + Ok(out) + } + + /// Resolves a path to the object filed under it. + pub fn object_at(&self, path: &str) -> Result { + let components = components_of(path)?; + let mut directory = self.pointer_at(self.globals.root)?; + if directory == 0 { + return Err(ObjectError::Malformed { + reason: "the root directory pointer is null", + }); + } + let mut walked = String::from("\\"); + let last = components.len() - 1; + for (at, component) in components.iter().enumerate() { + let found = self + .named_in(directory)? + .into_iter() + .find(|object| object.name.eq_ignore_ascii_case(component)) + .ok_or_else(|| ObjectError::NotFound { + directory: walked.clone(), + component: component.clone(), + })?; + if at == last { + return Ok(found); + } + // Anything with a directory under it has to *be* one, and the type is how that is + // known. A build this cannot read the type table on gets the benefit of the doubt -- + // the next read fails as an unreadable directory rather than as a wrong answer. + if found + .type_name + .as_deref() + .is_some_and(|kind| kind != "Directory") + { + return Err(ObjectError::NotADirectory { + component: component.clone(), + rest: components[at + 1..].join("\\"), + }); + } + directory = found.address; + if walked.len() > 1 { + walked.push('\\'); + } + walked.push_str(component); + } + unreachable!("a path with no components is refused above") + } + + /// Everything a directory holds. + pub fn objects_in(&self, path: &str) -> Result, ObjectError> { + let directory = match path.trim_end_matches('\\') { + "" => self.pointer_at(self.globals.root)?, + path => self.object_at(path)?.address, + }; + self.named_in(directory) + } + + /// What a symbolic link points at. + /// + /// **`LinkTarget` shares its storage with a callback pointer**, so what is there is checked + /// before it is followed rather than after: `Callback` lands exactly on `Length` and + /// `MaximumLength`, and `CallbackContext` lands on `Buffer`, so decoding without looking + /// reads a name out of whatever a context pointer happens to address. The checks are what a + /// string must satisfy and a function address need not -- a whole number of UTF-16 units, + /// within its own maximum -- and they are **not** a way to tell the two arms apart on their + /// own: filter by [`KernelObject::type_name`] first, and treat this as the second gate. + pub fn link_target(&self, link: u64) -> Result { + let at = link.wrapping_add(u64::from(self.layout.link_target)); + let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; + let bytes = self.read(at, size)?; + let field = |offset: u32| { + u16::from_le_bytes(bytes[offset as usize..][..2].try_into().unwrap_or_default()) + }; + let length = field(self.layout.unicode_length); + let maximum = field(self.layout.unicode_length + 2); + if length == 0 || !length.is_multiple_of(2) || length > maximum { + return Err(ObjectError::Malformed { + reason: "the link target is not a string this can vouch for", + }); + } + self.unicode_at(at) + } +} + +/// Splits an object path into the components a walk descends through. +fn components_of(path: &str) -> Result, ObjectError> { + let bad = |reason| ObjectError::BadPath { + path: path.to_string(), + reason, + }; + if !path.starts_with('\\') { + return Err(bad("an object path begins at the root, with a backslash")); + } + let parts: Vec = path + .split('\\') + .filter(|part| !part.is_empty()) + .map(str::to_string) + .collect(); + if parts.is_empty() { + return Err(bad( + "it names the root directory rather than an object in it", + )); + } + if parts.len() > MAX_COMPONENTS { + return Err(bad("it has more components than the namespace is deep")); + } + Ok(parts) +} + +/// UTF-16 little-endian, with anything unpaired replaced rather than refused: a name is being read +/// to show someone, and a lone surrogate in it is not a reason to lose the object. +fn utf16(bytes: &[u8]) -> String { + let (pairs, _) = bytes.as_chunks::<2>(); + let units: Vec = pairs.iter().copied().map(u16::from_le_bytes).collect(); + String::from_utf16_lossy(&units) +} + +impl DebugEngine { + /// Where the object namespace's structures are on **this** target. + /// + /// Read once per call rather than cached: the walk that follows makes tens of memory reads, so + /// a dozen type lookups beside them are not what costs, and a cache keyed on the wrong thing + /// is how a second build gets decoded with the first one's offsets. + pub fn object_layout(&self) -> Result { + // Every type below is the kernel's, so the module is the kernel's base -- asked of the + // engine rather than inferred from a symbol's address, which is inside a section and not + // the base a type lookup is scoped by. + let module = self.kernel_base()?; + let of = |type_name: &str, field: &str| -> Result { + let id = self.type_id(module, type_name)?; + self.field_offset(module, id, field) + }; + + let entry_chain = of("_OBJECT_DIRECTORY_ENTRY", "ChainLink")?; + let entry_object = of("_OBJECT_DIRECTORY_ENTRY", "Object")?; + // **A pointer's width is the target's, and it is derived rather than assumed.** These two + // fields are adjacent and the first is one pointer, so their distance is that width -- on + // a 32-bit kernel read from a 64-bit host, which is a supported target, the host's answer + // would be wrong by a factor of two and every read after it off the end of something. + let pointer = entry_object + .checked_sub(entry_chain) + .filter(|width| matches!(width, 4 | 8)) + .ok_or(DbgEngError::InvalidCommand)? as usize; + + let hash_buckets = of("_OBJECT_DIRECTORY", "HashBuckets")?; + // And the bucket count is the array's span over that width, for the same reason: 37 is + // this build's number rather than the structure's. + let buckets = (of("_OBJECT_DIRECTORY", "Lock")?.saturating_sub(hash_buckets) as usize) + .checked_div(pointer) + .filter(|count| *count > 0) + .ok_or(DbgEngError::InvalidCommand)?; + + let name_info = self.type_id(module, "_OBJECT_HEADER_NAME_INFO")?; + Ok(Layout { + pointer, + buckets, + hash_buckets, + entry_chain, + entry_object, + header_body: of("_OBJECT_HEADER", "Body")?, + header_type_index: of("_OBJECT_HEADER", "TypeIndex")?, + header_info_mask: of("_OBJECT_HEADER", "InfoMask")?, + header_security: of("_OBJECT_HEADER", "SecurityDescriptor")?, + name_info_name: self.field_offset(module, name_info, "Name")?, + name_info_size: self.type_size(module, name_info)?, + unicode_length: of("_UNICODE_STRING", "Length")?, + unicode_buffer: of("_UNICODE_STRING", "Buffer")?, + link_target: of("_OBJECT_SYMBOLIC_LINK", "LinkTarget")?, + type_name: of("_OBJECT_TYPE", "Name")?, + }) + } + + /// The globals the walk starts from. + /// + /// The two the walk cannot do without are errors; the two that only name a *type* are options, + /// because a build that renamed or inlined them still resolves paths. + pub fn object_globals(&self) -> Result { + Ok(Globals { + root: self.symbol_offset("nt!ObpRootDirectoryObject")?, + info_mask_to_offset: self.symbol_offset("nt!ObpInfoMaskToOffset")?, + header_cookie: self.symbol_offset("nt!ObHeaderCookie").ok(), + type_index_table: self.symbol_offset("nt!ObTypeIndexTable").ok(), + }) + } + + /// The object filed under a path, as `!object` would find it. + pub fn object_at(&self, path: &str) -> Result { + self.with_namespace(|namespace| namespace.object_at(path)) + } + + /// Everything a directory holds. + pub fn objects_in(&self, path: &str) -> Result, ObjectError> { + self.with_namespace(|namespace| namespace.objects_in(path)) + } + + /// What a symbolic link object points at. + pub fn symbolic_link_target(&self, link: u64) -> Result { + self.with_namespace(|namespace| namespace.link_target(link)) + } + + fn with_namespace( + &self, + answer: impl FnOnce(&Namespace<'_>) -> Result, + ) -> Result { + // A layout or a global this cannot resolve is reported as the read it would have been: + // both mean the same thing to a caller — this target does not carry a namespace to walk — + // and the symbol that failed is in the engine's own error rather than lost here. + let layout = self.object_layout().map_err(|_| ObjectError::Malformed { + reason: "this target has no type information for the object manager's structures", + })?; + let globals = self.object_globals().map_err(|_| ObjectError::Malformed { + reason: "this target does not resolve the object manager's globals", + })?; + let read = |at: u64, len: usize| self.read_memory(at, len).ok(); + answer(&Namespace::new(&read, layout, globals)) + } +} + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use super::*; + + /// The offsets measured on Windows 26100 x64, which is what the fixtures below are laid out to. + /// + /// Written out rather than derived from the builder that places the bytes: a fixture sharing + /// its arithmetic with the code under test agrees with it about a wrong offset, which is the + /// one thing a layout test cannot afford. + fn layout() -> Layout { + Layout { + pointer: 8, + buckets: 37, + hash_buckets: 0x00, + entry_chain: 0x00, + entry_object: 0x08, + header_body: 0x30, + header_type_index: 0x18, + header_info_mask: 0x1a, + header_security: 0x28, + name_info_name: 0x08, + name_info_size: 0x20, + unicode_length: 0x00, + unicode_buffer: 0x08, + link_target: 0x08, + type_name: 0x10, + } + } + + const ROOT_POINTER: u64 = 0xffff_f800_0000_1000; + const INFO_OFFSETS: u64 = 0xffff_f800_0000_2000; + const COOKIE: u64 = 0xffff_f800_0000_3000; + const TYPE_TABLE: u64 = 0xffff_f800_0000_4000; + /// The cookie this fixture's kernel booted with. + const COOKIE_VALUE: u8 = 0x5a; + + fn globals() -> Globals { + Globals { + root: ROOT_POINTER, + info_mask_to_offset: INFO_OFFSETS, + header_cookie: Some(COOKIE), + type_index_table: Some(TYPE_TABLE), + } + } + + /// A byte-addressed target, which is all the walk needs to be a walk. + #[derive(Default)] + struct Fake { + bytes: BTreeMap, + } + + impl Memory for Fake { + fn read(&self, address: u64, len: usize) -> Option> { + (0..len) + .map(|step| self.bytes.get(&(address + step as u64)).copied()) + .collect() + } + } + + impl Fake { + fn put(&mut self, at: u64, bytes: &[u8]) { + for (step, byte) in bytes.iter().enumerate() { + self.bytes.insert(at + step as u64, *byte); + } + } + + fn pointer(&mut self, at: u64, value: u64) { + self.put(at, &value.to_le_bytes()); + } + + /// A `_UNICODE_STRING` at `at`, with its characters at `buffer`. + fn string(&mut self, at: u64, buffer: u64, text: &str) { + let units: Vec = text + .encode_utf16() + .flat_map(|unit| unit.to_le_bytes()) + .collect(); + let length = units.len() as u16; + // The whole structure, padding included: the walk reads it in one go, and a fixture + // that writes only the fields it cares about leaves a hole that reads as unmapped. + self.put(at, &[0u8; 16]); + self.put(at, &length.to_le_bytes()); + self.put(at + 2, &length.to_le_bytes()); + self.pointer(at + 8, buffer); + self.put(buffer, &units); + } + + /// An object body with a header, a name, a type and a descriptor. + #[allow(clippy::too_many_arguments)] + fn object(&mut self, body: u64, name: &str, type_index: u8, security: u64) { + let header = body - 0x30; + // Name info sits `distance` before the header, and the table says how far. + self.put(header + 0x18, &[type_index]); + self.put(header + 0x1a, &[0x02]); + self.pointer(header + 0x28, security); + self.put(INFO_OFFSETS + u64::from(0x02u8 & 0x03), &[0x20]); + let name_info = header - 0x20; + self.string(name_info + 0x08, name_info + 0x1000, name); + } + + /// A directory holding these objects, one per bucket so the chains stay short. + fn directory(&mut self, at: u64, objects: &[u64]) { + for bucket in 0..37u64 { + self.pointer(at + bucket * 8, 0); + } + for (index, object) in objects.iter().enumerate() { + let entry = at + 0x2000 + (index as u64) * 0x20; + let bucket = (index as u64) % 37; + // Push onto the front of the bucket's chain. + let was = + u64::from_le_bytes(self.read(at + bucket * 8, 8).unwrap().try_into().unwrap()); + self.pointer(entry, was); + self.pointer(entry + 0x08, *object); + self.pointer(at + bucket * 8, entry); + } + } + + /// A type object whose index the header will be obfuscated against. + fn kind(&mut self, index: u8, at: u64, name: &str) { + self.pointer(TYPE_TABLE + u64::from(index) * 8, at); + self.string(at + 0x10, at + 0x1000, name); + } + } + + /// The index a header must carry for its object to read as `kind`. + fn obfuscated(kind: u8, body: u64) -> u8 { + let header = body - 0x30; + kind ^ COOKIE_VALUE ^ ((header >> 8) as u8) + } + + const ROOT: u64 = 0xffff_a000_0000_0000; + const DEVICE_DIR: u64 = 0xffff_a000_0010_0000; + const DEVICE: u64 = 0xffff_a000_0020_0000; + + fn namespace() -> Fake { + let mut fake = Fake::default(); + fake.put(COOKIE, &[COOKIE_VALUE]); + fake.pointer(ROOT_POINTER, ROOT); + fake.kind(3, 0xffff_a000_0030_0000, "Directory"); + fake.kind(4, 0xffff_a000_0031_0000, "Device"); + fake.kind(5, 0xffff_a000_0032_0000, "SymbolicLink"); + + fake.object(DEVICE_DIR, "Device", obfuscated(3, DEVICE_DIR), 0); + fake.directory(ROOT, &[DEVICE_DIR]); + + fake.object( + DEVICE, + "MountPointManager", + obfuscated(4, DEVICE), + 0xffff_b000_0000_0007, + ); + fake.directory(DEVICE_DIR, &[DEVICE]); + fake + } + + /// A path resolves to the object filed under it, with its type and its descriptor. + /// + /// The descriptor is stored with the object manager's own flag bits set, which is how a real + /// one is stored: taking the field as an address reads a security descriptor three bytes into + /// its own header and reports a DACL that is not there. + #[test] + fn a_path_resolves_to_the_object_filed_under_it() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + + let found = namespace + .object_at("\\Device\\MountPointManager") + .expect("the device is in the namespace"); + assert_eq!( + ( + found.address, + found.name.as_str(), + found.type_name.as_deref(), + found.security_descriptor + ), + ( + DEVICE, + "MountPointManager", + Some("Device"), + Some(0xffff_b000_0000_0000) + ), + "the low three bits of the descriptor field are the object manager's own" + ); + } + + /// A name is matched without regard to ASCII case, as the object manager matches it. + #[test] + fn a_name_is_matched_without_regard_to_case() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace + .object_at("\\device\\MOUNTPOINTMANAGER") + .map(|found| found.address), + Ok(DEVICE) + ); + } + + /// A component that is not there is **not found**, naming what was being looked in — and not + /// an empty answer, which reads as a namespace with nothing in it. + #[test] + fn a_missing_component_names_the_directory_it_was_not_in() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.object_at("\\Device\\Nothing"), + Err(ObjectError::NotFound { + directory: "\\Device".to_string(), + component: "Nothing".to_string(), + }) + ); + } + + /// Walking *through* something that is not a directory is refused rather than followed. + /// + /// A device's body is not a directory, and reading one as a directory reads 37 pointers out of + /// a driver's own fields and follows whatever they hold. + #[test] + fn a_leaf_is_not_walked_through() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.object_at("\\Device\\MountPointManager\\Deeper"), + Err(ObjectError::NotADirectory { + component: "MountPointManager".to_string(), + rest: "Deeper".to_string(), + }) + ); + } + + /// A directory lists what it holds, named. + #[test] + fn a_directory_lists_what_it_holds() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace + .objects_in("\\Device") + .map(|found| found.into_iter().map(|one| one.name).collect::>()), + Ok(vec!["MountPointManager".to_string()]) + ); + } + + /// A target whose namespace will not read says **which read failed**, rather than answering + /// with an empty namespace. + /// + /// This is a kernel minidump, where `nt`'s data pages are not in the file at all: measured on + /// `docs/samples/081226-2187-01.dmp`, `ObpRootDirectoryObject` itself reads `????????`. + #[test] + fn a_target_with_no_namespace_says_so_rather_than_answering_empty() { + let fake = Fake::default(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.object_at("\\Device"), + Err(ObjectError::Unreadable { + at: ROOT_POINTER, + len: 8 + }) + ); + } + + /// A chain that points at itself is refused, rather than walked until the process dies. + #[test] + fn a_looping_chain_is_refused_rather_than_walked() { + let mut fake = namespace(); + // The first bucket's entry chains to itself. + let entry = DEVICE_DIR + 0x2000; + fake.pointer(DEVICE_DIR, entry); + fake.pointer(entry, entry); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.objects_in("\\Device"), + Err(ObjectError::TooMany { + what: "a directory's entries", + bound: MAX_ENTRIES + }) + ); + } + + /// A link target is read as a string, and something that is not one is refused. + /// + /// The field shares storage with a callback pointer, so a link this walk cannot vouch for is + /// an error rather than a string decoded out of a function address. + #[test] + fn a_link_target_is_checked_before_it_is_decoded() { + let mut fake = namespace(); + const LINK: u64 = 0xffff_a000_0040_0000; + fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\MountPointManager"); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(LINK).as_deref(), + Ok("\\Device\\MountPointManager") + ); + + // The union's **other** arm, laid out as it really is: a callback address where the two + // lengths are, a context pointer where the buffer is, and all sixteen bytes mapped -- so + // what refuses this is the check rather than a read that happened to fail. + const CALLBACK: u64 = 0xffff_a000_0041_0000; + fake.pointer(CALLBACK + 0x08, 0xffff_f805_cb41_2341); + fake.pointer(CALLBACK + 0x10, 0xffff_a000_0050_0000); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(CALLBACK), + Err(ObjectError::Malformed { + reason: "the link target is not a string this can vouch for" + }), + "a code address is not a whole number of UTF-16 units" + ); + } + + /// A path that is not a path is refused before anything is read. + #[test] + fn a_path_that_is_not_one_is_refused() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert!(matches!( + namespace.object_at("Device"), + Err(ObjectError::BadPath { .. }) + )); + assert!(matches!( + namespace.object_at("\\"), + Err(ObjectError::BadPath { .. }) + )); + } +} From 03999139857251d2cb377566218c6ecdcaddede3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 16:12:59 +0100 Subject: [PATCH 02/10] fix: a fast reference is not a pointer, and a bound counts what it follows Four review findings, each pinned by breaking the rule it is for. **The security descriptor field is an `_EX_FAST_REF`.** The object manager keeps a count of outstanding fast references in the bits an aligned address leaves spare, and that count is **four** bits on a 64-bit kernel -- measured, `nt!_EX_FAST_REF::RefCnt` is `Pos 0, 4 Bits` on 26100 x64. Clearing three of them left the fourth standing on any object with eight or more live references, and the address handed back was then eight bytes into the descriptor: what a caller decodes from there is a DACL read out of the middle of a header, which is a wrong answer about who may open a device rather than a failure to answer. The mask is derived from the pointer width the layout already derives, since the count fills what the descriptor's alignment leaves. **A bound counts the links it follows, not the objects it emits.** An entry whose `Object` is null contributes nothing to the list, so a bound counting the list never reached one -- and one of those pointing at itself is a loop the walk never leaves. That is not a test that fails, it is a debugger that stops answering, and the mutation for it hangs rather than going red. **Listing a leaf is refused rather than enumerated.** The guard against walking *through* a device existed; the same guard at the *end* of a path did not, so `objects_in` on a device read a driver's own fields as thirty-seven bucket pointers and followed whatever they held. **A length with no buffer is malformed, not empty.** Answered as an empty string it becomes a symbolic link whose target is `""`, which a caller publishes as a device reachable under no name at all. One finding is **declined**, and the reason belongs here because the next round will raise it against code that by then looks deliberate: the walk was said to ask for `nt!ObpTypeIndexTable`, with the extra `p`, and to lose every type name to the `.ok()` that follows. It asks for `nt!ObTypeIndexTable` and always has. `x nt!ObpTypeIndexTable` resolves nothing on the checked-in dump; `x nt!ObTypeIndexTable` resolves to `fffff805 cc3c6800`. Nothing changed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 131 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 120 insertions(+), 11 deletions(-) diff --git a/src/object.rs b/src/object.rs index b83314a..81b2816 100644 --- a/src/object.rs +++ b/src/object.rs @@ -59,6 +59,9 @@ const MAX_COMPONENTS: usize = 32; /// long is not one the object manager made. const MAX_NAME_BYTES: usize = 1024; +/// What the object manager calls a directory's type, and the one type name this walk acts on. +const DIRECTORY: &str = "Directory"; + /// Why a namespace walk could not answer. #[derive(Debug, Error, PartialEq, Eq)] pub enum ObjectError { @@ -97,8 +100,8 @@ pub struct KernelObject { /// The object type's name (`Device`, `SymbolicLink`, `Directory`), when the type table could /// be read. `None` is "this walk could not say", never "untyped". pub type_name: Option, - /// `_OBJECT_HEADER::SecurityDescriptor`, with the three low bits the object manager keeps its - /// own flags in masked off. `None` where the object carries none. + /// `_OBJECT_HEADER::SecurityDescriptor`, with the fast-reference count masked out of it. + /// `None` where the object carries none. pub security_descriptor: Option, } @@ -207,6 +210,7 @@ impl<'a> Namespace<'a> { /// The object body every entry of a directory points at. fn entries_of(&self, directory: u64) -> Result, ObjectError> { let mut out = Vec::new(); + let mut followed = 0usize; for bucket in 0..self.layout.buckets { let at = directory .wrapping_add(u64::from(self.layout.hash_buckets)) @@ -214,8 +218,14 @@ impl<'a> Namespace<'a> { let mut entry = self.pointer_at(at)?; // A chain is bounded by the whole directory's bound rather than one of its own: a // cycle inside a bucket and a cycle across buckets are the same corruption. + // + // **The bound counts links followed, not objects found.** Counting what came out of + // the walk leaves a chain of entries whose `Object` is null unbounded -- one of them + // pointing at itself is a loop this never leaves, which on a live kernel is a + // debugger that stops answering rather than a walk that refuses. while entry != 0 { - if out.len() >= MAX_ENTRIES { + followed += 1; + if followed > MAX_ENTRIES { return Err(ObjectError::TooMany { what: "a directory's entries", bound: MAX_ENTRIES, @@ -247,9 +257,17 @@ impl<'a> Namespace<'a> { .unwrap_or_default(), ) as usize; let buffer = self.pointer_at(at.wrapping_add(u64::from(self.layout.unicode_buffer)))?; - if length == 0 || buffer == 0 { + if length == 0 { return Ok(String::new()); } + // **A length with no buffer is not an empty string**, it is a structure contradicting + // itself -- and an empty string is what a caller would publish as a symbolic link's + // target, which is worse than saying nothing. + if buffer == 0 { + return Err(ObjectError::Malformed { + reason: "a UNICODE_STRING has a length and no buffer", + }); + } if !length.is_multiple_of(2) { return Err(ObjectError::Malformed { reason: "a UNICODE_STRING's length is not a whole number of UTF-16 units", @@ -298,13 +316,24 @@ impl<'a> Namespace<'a> { )?)) } - /// The security descriptor an object carries, with the object manager's flag bits cleared. + /// The security descriptor an object carries, with the reference count cleared out of it. + /// + /// **The field is an `_EX_FAST_REF`, not a pointer**: the object manager keeps a count of + /// outstanding fast references in the bits an aligned address leaves spare, and that is + /// **four** bits on a 64-bit kernel — measured, `nt!_EX_FAST_REF::RefCnt` is `Pos 0, 4 Bits` + /// on 26100 x64. Clearing three of them leaves the fourth set on any object with eight or more + /// live references, and the address handed back is then eight bytes into the descriptor: what + /// gets decoded is a DACL read from the middle of a header, which is a wrong answer about who + /// may open a device rather than a failure to answer. + /// + /// So the mask is derived from the pointer width the layout already derived — the count fills + /// what the descriptor's alignment leaves, which is one bit more than the pointer's own. fn security_of(&self, body: u64) -> Result, ObjectError> { let at = self .header_of(body) .wrapping_add(u64::from(self.layout.header_security)); - // The low three bits are the object manager's own, never part of the address. - let descriptor = self.pointer_at(at)? & !0b111; + let counted = (2 * self.layout.pointer as u64) - 1; + let descriptor = self.pointer_at(at)? & !counted; Ok((descriptor != 0).then_some(descriptor)) } @@ -382,7 +411,7 @@ impl<'a> Namespace<'a> { if found .type_name .as_deref() - .is_some_and(|kind| kind != "Directory") + .is_some_and(|kind| kind != DIRECTORY) { return Err(ObjectError::NotADirectory { component: component.clone(), @@ -399,10 +428,28 @@ impl<'a> Namespace<'a> { } /// Everything a directory holds. + /// + /// **A path that resolves to a leaf is refused rather than enumerated.** `object_at` guards + /// walking *through* a device on the way to something else; this is the same guard at the end + /// of the path, and without it `\Device\MountPointManager` has a driver's own fields read as + /// thirty-seven bucket pointers and whatever they hold followed as chains. pub fn objects_in(&self, path: &str) -> Result, ObjectError> { let directory = match path.trim_end_matches('\\') { "" => self.pointer_at(self.globals.root)?, - path => self.object_at(path)?.address, + path => { + let found = self.object_at(path)?; + if found + .type_name + .as_deref() + .is_some_and(|kind| kind != DIRECTORY) + { + return Err(ObjectError::NotADirectory { + component: found.name, + rest: String::new(), + }); + } + found.address + } }; self.named_in(directory) } @@ -718,7 +765,7 @@ mod tests { DEVICE, "MountPointManager", obfuscated(4, DEVICE), - 0xffff_b000_0000_0007, + 0xffff_b000_0000_000f, ); fake.directory(DEVICE_DIR, &[DEVICE]); fake @@ -750,7 +797,7 @@ mod tests { Some("Device"), Some(0xffff_b000_0000_0000) ), - "the low three bits of the descriptor field are the object manager's own" + "the descriptor field is a fast reference, and the count in its low bits is not part of the address" ); } @@ -830,6 +877,68 @@ mod tests { ); } + /// A **null-object** chain is bounded too, and that is a different counter from the one that + /// bounds what comes out. + /// + /// An entry whose `Object` is null contributes nothing to the list, so a bound counting the + /// list never reaches it — and one of those pointing at itself is a loop this never leaves. On + /// a live kernel that is a debugger that stops answering, which is the failure a bound exists + /// to turn into a refusal. + #[test] + fn a_chain_of_entries_that_name_nothing_is_bounded_as_well() { + let mut fake = namespace(); + const EMPTY: u64 = DEVICE_DIR + 0x8000; + fake.pointer(DEVICE_DIR, EMPTY); + fake.pointer(EMPTY, EMPTY); + fake.pointer(EMPTY + 0x08, 0); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.objects_in("\\Device"), + Err(ObjectError::TooMany { + what: "a directory's entries", + bound: MAX_ENTRIES + }) + ); + } + + /// Listing a **leaf** is refused rather than answered. + /// + /// `object_at` guards walking *through* a device on the way to something else; this is the + /// same guard at the end of a path. Without it a driver's own fields are read as thirty-seven + /// bucket pointers and whatever they hold is followed as chains. + #[test] + fn a_leaf_is_not_listed_as_a_directory() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.objects_in("\\Device\\MountPointManager"), + Err(ObjectError::NotADirectory { + component: "MountPointManager".to_string(), + rest: String::new(), + }) + ); + } + + /// A string with a length and **no buffer** is malformed, not empty. + /// + /// Answered as an empty string it becomes a symbolic link whose target is `""`, which a + /// caller publishes as a device reachable under no name at all. + #[test] + fn a_length_with_no_buffer_is_malformed_rather_than_empty() { + let mut fake = namespace(); + const LINK: u64 = 0xffff_a000_0042_0000; + fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); + // Everything the link check looks at still holds; only the buffer is gone. + fake.pointer(LINK + 0x10, 0); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(LINK), + Err(ObjectError::Malformed { + reason: "a UNICODE_STRING has a length and no buffer" + }) + ); + } + /// A chain that points at itself is refused, rather than walked until the process dies. #[test] fn a_looping_chain_is_refused_rather_than_walked() { From b7ed9487224d5026ea7d99918ab59f7a23a14b30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 16:20:46 +0100 Subject: [PATCH 03/10] fix: a link target is checked for being a target, not for being a string The union `LinkTarget` sits in is not discriminated by the object's type: a callback-backed symbolic link is a `SymbolicLink` like any other, so the doc comment telling a caller to filter on `type_name` first was pointing at a gate that is not there. `Callback` lands exactly on `Length` and `MaximumLength` and `CallbackContext` lands on `Buffer`, so a code address whose low half happens to be an even, non-zero, in-range length gets past checks that ask only whether this is a string -- and what gets decoded is whatever the context pointer addresses. So the check is now what a **link target** is rather than what a string is: the lengths have to be a string's, and then the thing they describe has to be an object path, which begins at the root with a backslash. The test that was here covered an address the lengths caught; the case that matters is the one they do not, and it is now beside it. **The discriminating flag is deliberately not read**, and this is where the reason goes. `_OBJECT_SYMBOLIC_LINK::Flags` is what the object manager itself branches on, and which bit that is could not be measured here: local kernel debugging is not enabled on this host (`attach_kernel_local` answers `0x80004001`), and a kernel minidump carries no namespace at all. A bit taken from reading about it rather than from a target would be a rule nothing checked, which is the shape of mistake this crate keeps finding in review. Refusing a callback link cannot be wrong in the direction that matters. Reading the flag would let one be answered instead, and that is worth doing from a live kernel rather than from memory. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 57 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/src/object.rs b/src/object.rs index 81b2816..86b51f3 100644 --- a/src/object.rs +++ b/src/object.rs @@ -456,13 +456,23 @@ impl<'a> Namespace<'a> { /// What a symbolic link points at. /// - /// **`LinkTarget` shares its storage with a callback pointer**, so what is there is checked - /// before it is followed rather than after: `Callback` lands exactly on `Length` and - /// `MaximumLength`, and `CallbackContext` lands on `Buffer`, so decoding without looking - /// reads a name out of whatever a context pointer happens to address. The checks are what a - /// string must satisfy and a function address need not -- a whole number of UTF-16 units, - /// within its own maximum -- and they are **not** a way to tell the two arms apart on their - /// own: filter by [`KernelObject::type_name`] first, and treat this as the second gate. + /// **`LinkTarget` shares its storage with a callback pointer**, and the type does not tell + /// the two apart: a callback-backed link is a `SymbolicLink` like any other. `Callback` lands + /// exactly on `Length` and `MaximumLength`, `CallbackContext` lands on `Buffer`, so decoding + /// without looking reads a name out of whatever a context pointer happens to address. + /// + /// So what is checked is what a **link target** is rather than what a string is. The lengths + /// have to be a string's, and then the thing they describe has to be an object path — which + /// begins at the root, with a backslash. A code address whose low halves happen to pass for + /// lengths gets that far and no further, because what its context points at does not begin + /// with one. + /// + /// **The discriminating flag is deliberately not read.** `_OBJECT_SYMBOLIC_LINK::Flags` is + /// what the object manager itself branches on, and which bit that is could not be measured on + /// this bench: local kernel debugging is not enabled here, a minidump carries no namespace, + /// and a bit guessed from reading about it would be a rule nothing checked. Refusing a + /// callback link is the answer that cannot be wrong in the direction that matters; reading the + /// flag would let one be *answered*, and that is worth doing from a live kernel. pub fn link_target(&self, link: u64) -> Result { let at = link.wrapping_add(u64::from(self.layout.link_target)); let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; @@ -477,7 +487,13 @@ impl<'a> Namespace<'a> { reason: "the link target is not a string this can vouch for", }); } - self.unicode_at(at) + let target = self.unicode_at(at)?; + if !target.starts_with('\\') { + return Err(ObjectError::Malformed { + reason: "the link target is not an object path, so this is not a target", + }); + } + Ok(target) } } @@ -986,6 +1002,31 @@ mod tests { }), "a code address is not a whole number of UTF-16 units" ); + + // **And the case the lengths do not catch**, which is the one that matters: a callback + // address whose low half is an even, non-zero, in-range length. Everything a string must + // satisfy holds, the context pointer is mapped, and what it addresses decodes perfectly + // well -- as text that is not an object path. Nothing about the *type* would have stopped + // this: a callback-backed link is a `SymbolicLink` like any other. + const PASSES: u64 = 0xffff_a000_0043_0000; + const CONTEXT: u64 = 0xffff_a000_0051_0000; + fake.pointer(PASSES + 0x08, 0xffff_f805_cb41_0010); + fake.pointer(PASSES + 0x10, CONTEXT); + fake.put( + CONTEXT, + &"HeapFree" + .encode_utf16() + .flat_map(u16::to_le_bytes) + .collect::>(), + ); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(PASSES), + Err(ObjectError::Malformed { + reason: "the link target is not an object path, so this is not a target" + }), + "the lengths passed, and what they described was not a target" + ); } /// A path that is not a path is refused before anything is read. From 419320414db08231837e061c48e45dac4b3e6a28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 16:30:33 +0100 Subject: [PATCH 04/10] fix: the callback arm is a bit the kernel tests, and a guard that cannot tell says no Three findings, and the first sent me to read the kernel rather than argue with the reviewer. **The link union's discriminator is `Flags & 10h`**, measured out of `nt!ObpParseSymbolicLinkEx` on 26100 x64: it loads `Flags`, tests `10h`, and on that branch calls through the pointer at `+0x08` with the context at `+0x10`; with the bit clear it reads `+0x08` as the `_UNICODE_STRING`. That is now read first, so a callback-backed link is *answered* -- it has no target -- rather than refused for failing a string check. Worth recording why this was measured rather than looked up: the neighbouring bits are all something else. `2h` asks whether the token is sandboxed, `8h` masks an access mask, `1h` is a silo check. `2h` is the one a reader would try first, and it is the wrong one. The content checks stay, because they are cheap and they are what the kernel itself does two blocks earlier -- `cmp word ptr [rax],5Ch`, the same backslash. They are evidence now rather than the answer. **A guard that cannot tell says no.** Both directory guards read `type_name.is_some_and(...)`, which passes an object whose type could not be read -- and the guard exists to stop a device's own fields being read as thirty-seven bucket pointers, so passing on doubt is passing on exactly the case it is for. Both now require `Some("Directory")`. That makes the type load-bearing, so `ObHeaderCookie` and `ObTypeIndexTable` stop being optional globals: a walk that cannot name a type cannot make the guarantee, and saying so at `object_globals` is better than a guard that silently opens. **A string is checked against its own maximum.** `Length` past `MaximumLength` is a torn structure, and reading the length alone takes whatever follows into a name the walk then matches paths against -- which resolves some *other* object rather than failing to resolve this one. Each of the three is pinned by breaking it, one at a time. The `maximum` mutation also found a bug in my own harness: restoring a rule whose broken form is the empty string prepends it to the file rather than putting it back, which `cargo fmt` caught as a parse error at line 1. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 170 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 139 insertions(+), 31 deletions(-) diff --git a/src/object.rs b/src/object.rs index 86b51f3..33ea16a 100644 --- a/src/object.rs +++ b/src/object.rs @@ -62,6 +62,16 @@ const MAX_NAME_BYTES: usize = 1024; /// What the object manager calls a directory's type, and the one type name this walk acts on. const DIRECTORY: &str = "Directory"; +/// The bit in `_OBJECT_SYMBOLIC_LINK::Flags` that says the object's callback arm is live. +/// +/// **Read out of the kernel rather than out of a document.** `nt!ObpParseSymbolicLinkEx` on +/// 26100 x64 loads `Flags`, tests `10h`, and on that branch calls through the pointer at `+0x08` +/// with the context at `+0x10`; with the bit clear it takes `+0x08` as the `_UNICODE_STRING` it +/// is in the other arm. The neighbouring bits are all something else -- `2h` asks whether the +/// token is sandboxed, `8h` masks an access mask, `1h` is a silo check -- which is why this was +/// measured rather than guessed: the first bit anyone would have tried is the sandbox one. +const SYMBOLIC_LINK_CALLBACK: u32 = 0x10; + /// Why a namespace walk could not answer. #[derive(Debug, Error, PartialEq, Eq)] pub enum ObjectError { @@ -134,8 +144,9 @@ pub struct Layout { /// `_UNICODE_STRING::Length` and `::Buffer`. pub unicode_length: u32, pub unicode_buffer: u32, - /// `_OBJECT_SYMBOLIC_LINK::LinkTarget`. + /// `_OBJECT_SYMBOLIC_LINK::LinkTarget`, and the `Flags` that say whether it is live. pub link_target: u32, + pub link_flags: u32, /// `_OBJECT_TYPE::Name`. pub type_name: u32, } @@ -151,8 +162,8 @@ pub struct Globals { /// `nt!ObHeaderCookie` and `nt!ObTypeIndexTable`, which together turn a header's obfuscated /// `TypeIndex` into a type object. Optional: a build without them still resolves names, and a /// `type_name` of `None` is the honest answer there. - pub header_cookie: Option, - pub type_index_table: Option, + pub header_cookie: u64, + pub type_index_table: u64, } /// Which optional header is wanted, as the bit the kernel's own lookup is keyed on. @@ -251,11 +262,18 @@ impl<'a> Namespace<'a> { fn unicode_at(&self, at: u64) -> Result { let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; let bytes = self.read(at, size)?; - let length = u16::from_le_bytes( - bytes[self.layout.unicode_length as usize..][..2] - .try_into() - .unwrap_or_default(), - ) as usize; + let field = |offset: u32| { + u16::from_le_bytes(bytes[offset as usize..][..2].try_into().unwrap_or_default()) + }; + let length = field(self.layout.unicode_length) as usize; + // **A string that is longer than its own buffer is torn**, and reading `Length` alone + // takes whatever follows into a name the walk then matches paths against -- which + // resolves some other object, rather than failing to resolve this one. + if length > field(self.layout.unicode_length + 2) as usize { + return Err(ObjectError::Malformed { + reason: "a UNICODE_STRING is longer than its own maximum", + }); + } let buffer = self.pointer_at(at.wrapping_add(u64::from(self.layout.unicode_buffer)))?; if length == 0 { return Ok(String::new()); @@ -343,7 +361,7 @@ impl<'a> Namespace<'a> { /// byte of the header's own address — so a build whose cookie this cannot find gets `None` /// rather than a type read out of the wrong table slot. fn type_of(&self, body: u64) -> Option { - let (cookie, table) = (self.globals.header_cookie?, self.globals.type_index_table?); + let (cookie, table) = (self.globals.header_cookie, self.globals.type_index_table); let header = self.header_of(body); let raw = self .read( @@ -406,13 +424,10 @@ impl<'a> Namespace<'a> { return Ok(found); } // Anything with a directory under it has to *be* one, and the type is how that is - // known. A build this cannot read the type table on gets the benefit of the doubt -- - // the next read fails as an unreadable directory rather than as a wrong answer. - if found - .type_name - .as_deref() - .is_some_and(|kind| kind != DIRECTORY) - { + // known. **Fail closed**: an object whose type could not be read is not one to walk + // through on the chance that it is a directory, because what that reads is a device's + // own fields as bucket pointers. + if found.type_name.as_deref() != Some(DIRECTORY) { return Err(ObjectError::NotADirectory { component: component.clone(), rest: components[at + 1..].join("\\"), @@ -438,11 +453,7 @@ impl<'a> Namespace<'a> { "" => self.pointer_at(self.globals.root)?, path => { let found = self.object_at(path)?; - if found - .type_name - .as_deref() - .is_some_and(|kind| kind != DIRECTORY) - { + if found.type_name.as_deref() != Some(DIRECTORY) { return Err(ObjectError::NotADirectory { component: found.name, rest: String::new(), @@ -467,13 +478,22 @@ impl<'a> Namespace<'a> { /// lengths gets that far and no further, because what its context points at does not begin /// with one. /// - /// **The discriminating flag is deliberately not read.** `_OBJECT_SYMBOLIC_LINK::Flags` is - /// what the object manager itself branches on, and which bit that is could not be measured on - /// this bench: local kernel debugging is not enabled here, a minidump carries no namespace, - /// and a bit guessed from reading about it would be a rule nothing checked. Refusing a - /// callback link is the answer that cannot be wrong in the direction that matters; reading the - /// flag would let one be *answered*, and that is worth doing from a live kernel. + /// **And the discriminator is read first**, because the checks above are evidence and the flag + /// is the answer: [`SYMBOLIC_LINK_CALLBACK`] is the bit the object manager itself branches on, + /// taken out of `nt!ObpParseSymbolicLinkEx` rather than out of a document. A link whose + /// callback arm is live has no target to read, and saying so is not the same as failing to + /// decode one. pub fn link_target(&self, link: u64) -> Result { + let flags = self.read( + link.wrapping_add(u64::from(self.layout.link_flags)), + size_of::(), + )?; + let flags = u32::from_le_bytes(flags[..4].try_into().unwrap_or_default()); + if flags & SYMBOLIC_LINK_CALLBACK != 0 { + return Err(ObjectError::Malformed { + reason: "this link resolves through a callback, so it has no target to read", + }); + } let at = link.wrapping_add(u64::from(self.layout.link_target)); let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; let bytes = self.read(at, size)?; @@ -581,6 +601,7 @@ impl DebugEngine { unicode_length: of("_UNICODE_STRING", "Length")?, unicode_buffer: of("_UNICODE_STRING", "Buffer")?, link_target: of("_OBJECT_SYMBOLIC_LINK", "LinkTarget")?, + link_flags: of("_OBJECT_SYMBOLIC_LINK", "Flags")?, type_name: of("_OBJECT_TYPE", "Name")?, }) } @@ -593,8 +614,8 @@ impl DebugEngine { Ok(Globals { root: self.symbol_offset("nt!ObpRootDirectoryObject")?, info_mask_to_offset: self.symbol_offset("nt!ObpInfoMaskToOffset")?, - header_cookie: self.symbol_offset("nt!ObHeaderCookie").ok(), - type_index_table: self.symbol_offset("nt!ObTypeIndexTable").ok(), + header_cookie: self.symbol_offset("nt!ObHeaderCookie")?, + type_index_table: self.symbol_offset("nt!ObTypeIndexTable")?, }) } @@ -658,6 +679,7 @@ mod tests { unicode_length: 0x00, unicode_buffer: 0x08, link_target: 0x08, + link_flags: 0x1c, type_name: 0x10, } } @@ -673,8 +695,8 @@ mod tests { Globals { root: ROOT_POINTER, info_mask_to_offset: INFO_OFFSETS, - header_cookie: Some(COOKIE), - type_index_table: Some(TYPE_TABLE), + header_cookie: COOKIE, + type_index_table: TYPE_TABLE, } } @@ -749,6 +771,11 @@ mod tests { } } + /// The flags a symbolic link carries, which say which arm of its union is live. + fn flags(&mut self, link: u64, flags: u32) { + self.put(link + 0x1c, &flags.to_le_bytes()); + } + /// A type object whose index the header will be obfuscated against. fn kind(&mut self, index: u8, at: u64, name: &str) { self.pointer(TYPE_TABLE + u64::from(index) * 8, at); @@ -943,6 +970,7 @@ mod tests { fn a_length_with_no_buffer_is_malformed_rather_than_empty() { let mut fake = namespace(); const LINK: u64 = 0xffff_a000_0042_0000; + fake.flags(LINK, 0); fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); // Everything the link check looks at still holds; only the buffer is gone. fake.pointer(LINK + 0x10, 0); @@ -981,6 +1009,7 @@ mod tests { fn a_link_target_is_checked_before_it_is_decoded() { let mut fake = namespace(); const LINK: u64 = 0xffff_a000_0040_0000; + fake.flags(LINK, 0); fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\MountPointManager"); let namespace = Namespace::new(&fake, layout(), globals()); assert_eq!( @@ -992,6 +1021,7 @@ mod tests { // lengths are, a context pointer where the buffer is, and all sixteen bytes mapped -- so // what refuses this is the check rather than a read that happened to fail. const CALLBACK: u64 = 0xffff_a000_0041_0000; + fake.flags(CALLBACK, 0); fake.pointer(CALLBACK + 0x08, 0xffff_f805_cb41_2341); fake.pointer(CALLBACK + 0x10, 0xffff_a000_0050_0000); let namespace = Namespace::new(&fake, layout(), globals()); @@ -1010,6 +1040,7 @@ mod tests { // this: a callback-backed link is a `SymbolicLink` like any other. const PASSES: u64 = 0xffff_a000_0043_0000; const CONTEXT: u64 = 0xffff_a000_0051_0000; + fake.flags(PASSES, 0); fake.pointer(PASSES + 0x08, 0xffff_f805_cb41_0010); fake.pointer(PASSES + 0x10, CONTEXT); fake.put( @@ -1029,6 +1060,83 @@ mod tests { ); } + /// A link whose **callback** arm is live has no target, and says so. + /// + /// This is the case the content checks cannot reach: the lengths are a string's, the buffer + /// reads, and what it holds is a path. Only the flag the object manager itself branches on + /// separates the two arms, which is why it is read first. + #[test] + fn a_callback_link_has_no_target_to_read() { + let mut fake = namespace(); + const LINK: u64 = 0xffff_a000_0044_0000; + // A target that would decode perfectly well, and a flag saying it is not the live arm. + fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); + fake.flags(LINK, 0); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(LINK).as_deref(), + Ok("\\Device\\X"), + "with the bit clear the union holds the target" + ); + + fake.flags(LINK, SYMBOLIC_LINK_CALLBACK); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(LINK), + Err(ObjectError::Malformed { + reason: "this link resolves through a callback, so it has no target to read" + }), + "and with it set the same bytes are a callback and a context" + ); + } + + /// An object whose **type could not be read** is not walked through or listed. + /// + /// The guard is what stops a device's own fields being read as bucket pointers, and a guard + /// that passes when it cannot tell is not one. Here the type table slot is empty, which is + /// every way that read can fail rolled into one. + #[test] + fn an_object_of_unknown_type_is_not_treated_as_a_directory() { + let mut fake = namespace(); + // The slot the Device directory's own type index selects, emptied. + fake.pointer(TYPE_TABLE + 3 * 8, 0); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.object_at("\\Device\\MountPointManager"), + Err(ObjectError::NotADirectory { + component: "Device".to_string(), + rest: "MountPointManager".to_string(), + }), + "an unreadable type is not a directory" + ); + assert!( + matches!( + namespace.objects_in("\\Device"), + Err(ObjectError::NotADirectory { .. }) + ), + "and listing it is the same refusal" + ); + } + + /// A name longer than its own maximum is **torn**, and taking its length would read past the + /// buffer into whatever follows -- which the walk then matches a path against, resolving some + /// other object rather than failing to resolve this one. + #[test] + fn a_name_longer_than_its_own_maximum_is_refused() { + let mut fake = namespace(); + let header = DEVICE - 0x30; + let name_info = header - 0x20; + // Length past MaximumLength, with the buffer left as it was. + fake.put(name_info + 0x08, &200u16.to_le_bytes()); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.objects_in("\\Device"), + Err(ObjectError::Malformed { + reason: "a UNICODE_STRING is longer than its own maximum" + }) + ); + } + /// A path that is not a path is refused before anything is read. #[test] fn a_path_that_is_not_one_is_refused() { From 1a5d8719a6e1fefdb5506f3982b39b2e0a263a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 16:40:04 +0100 Subject: [PATCH 05/10] fix: a refusal says which of the two it is, and a rendering is not an identity Two findings, and the first is a round trip: the previous round made the type globals required so the directory guards could fail closed, and this one asks for them back because resolving a one-component path, listing the root and reading a link target need no type at all. Both rounds were right about different things, and neither position was the answer. What was wrong underneath is that the guard had one refusal for two facts. An object this walk could not type is not an object that *is* something else, and reporting a directory as `NotADirectory` because the type table would not read is a lie the caller would act on. So `Untyped` is its own variant, the globals go back to optional, and the guards still fail closed -- saying which of the two it is. A target that cannot name types now answers everything that needs no type and refuses, accurately, the one thing that does. **And a rendering is not an identity.** An unpaired surrogate is a legal object name and an illegal `String`, so `from_utf16_lossy` turns it into a replacement character: two different names render alike, and a caller asking for the replacement character reaches an object whose name contains no such character. For a walk whose answer feeds a security question, that is a device answering under a name that is not its own -- and it is a name a driver chooses, not one it is stuck with. The name is still rendered, because an object nobody can list is worse than one nobody can resolve by name, and `KernelObject::exact_name` says which it is. `object_at` will not match an object it cannot reproduce exactly. A link target that is not exactly its own bytes is refused outright, since there is no listing to preserve there. This round's harness refuses a mutation that maps a rule to the empty string, which is how the last one corrupted the file while restoring. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 217 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 185 insertions(+), 32 deletions(-) diff --git a/src/object.rs b/src/object.rs index 33ea16a..452b232 100644 --- a/src/object.rs +++ b/src/object.rs @@ -95,6 +95,14 @@ pub enum ObjectError { /// A component resolved to something that is not a directory, with path left to walk. #[error("{component:?} is not a directory, so {rest:?} cannot be under it")] NotADirectory { component: String, rest: String }, + /// A component could not be **typed**, so this walk will not treat it as a directory. + /// + /// Distinct from [`Self::NotADirectory`], and the distinction is the whole reason this variant + /// exists: that one says an object *is* something else, which for a directory this could not + /// name would be a lie. The walk still refuses, because a guard that passes on doubt passes on + /// exactly the case it is for -- but it refuses saying it could not tell. + #[error("{component:?} could not be typed, so this walk will not descend through it")] + Untyped { component: String }, /// A cap was reached, so what this could answer with is a **short** list. #[error("{what} exceeded its bound of {bound}, so this list would be shorter than the truth")] TooMany { what: &'static str, bound: usize }, @@ -107,6 +115,14 @@ pub struct KernelObject { pub address: u64, /// Its name in the directory that holds it, not a path. pub name: String, + /// Whether [`Self::name`] **is** that name, or only shows it. + /// + /// A name is a counted run of UTF-16 units, which an unpaired surrogate makes legal as a name + /// and illegal as text. Such a name is rendered with replacements so the object still lists, + /// and this says the rendering is not an identity: [`Namespace::object_at`] will not resolve to + /// an object whose name it cannot reproduce exactly, because two such names render alike and a + /// caller would get whichever came first. + pub exact_name: bool, /// The object type's name (`Device`, `SymbolicLink`, `Directory`), when the type table could /// be read. `None` is "this walk could not say", never "untyped". pub type_name: Option, @@ -162,8 +178,8 @@ pub struct Globals { /// `nt!ObHeaderCookie` and `nt!ObTypeIndexTable`, which together turn a header's obfuscated /// `TypeIndex` into a type object. Optional: a build without them still resolves names, and a /// `type_name` of `None` is the honest answer there. - pub header_cookie: u64, - pub type_index_table: u64, + pub header_cookie: Option, + pub type_index_table: Option, } /// Which optional header is wanted, as the bit the kernel's own lookup is keyed on. @@ -259,7 +275,7 @@ impl<'a> Namespace<'a> { } /// One `_UNICODE_STRING`, read from wherever it sits. - fn unicode_at(&self, at: u64) -> Result { + fn unicode_at(&self, at: u64) -> Result<(String, bool), ObjectError> { let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; let bytes = self.read(at, size)?; let field = |offset: u32| { @@ -276,7 +292,7 @@ impl<'a> Namespace<'a> { } let buffer = self.pointer_at(at.wrapping_add(u64::from(self.layout.unicode_buffer)))?; if length == 0 { - return Ok(String::new()); + return Ok((String::new(), true)); } // **A length with no buffer is not an empty string**, it is a structure contradicting // itself -- and an empty string is what a caller would publish as a symbolic link's @@ -305,7 +321,7 @@ impl<'a> Namespace<'a> { /// /// `Ok(None)` is an object filed under no name at all, which is ordinary — most objects are /// reached by handle and never named. - fn name_of(&self, body: u64) -> Result, ObjectError> { + fn name_of(&self, body: u64) -> Result, ObjectError> { let header = self.header_of(body); let mask = self.read( header.wrapping_add(u64::from(self.layout.header_info_mask)), @@ -361,7 +377,7 @@ impl<'a> Namespace<'a> { /// byte of the header's own address — so a build whose cookie this cannot find gets `None` /// rather than a type read out of the wrong table slot. fn type_of(&self, body: u64) -> Option { - let (cookie, table) = (self.globals.header_cookie, self.globals.type_index_table); + let (cookie, table) = (self.globals.header_cookie?, self.globals.type_index_table?); let header = self.header_of(body); let raw = self .read( @@ -377,7 +393,7 @@ impl<'a> Namespace<'a> { if entry == 0 { return None; } - let name = self + let (name, _) = self .unicode_at(entry.wrapping_add(u64::from(self.layout.type_name))) .ok()?; (!name.is_empty()).then_some(name) @@ -387,12 +403,13 @@ impl<'a> Namespace<'a> { fn named_in(&self, directory: u64) -> Result, ObjectError> { let mut out = Vec::new(); for body in self.entries_of(directory)? { - let Some(name) = self.name_of(body)? else { + let Some((name, exact_name)) = self.name_of(body)? else { continue; }; out.push(KernelObject { address: body, name, + exact_name, type_name: self.type_of(body), security_descriptor: self.security_of(body)?, }); @@ -415,7 +432,7 @@ impl<'a> Namespace<'a> { let found = self .named_in(directory)? .into_iter() - .find(|object| object.name.eq_ignore_ascii_case(component)) + .find(|object| object.exact_name && object.name.eq_ignore_ascii_case(component)) .ok_or_else(|| ObjectError::NotFound { directory: walked.clone(), component: component.clone(), @@ -427,11 +444,19 @@ impl<'a> Namespace<'a> { // known. **Fail closed**: an object whose type could not be read is not one to walk // through on the chance that it is a directory, because what that reads is a device's // own fields as bucket pointers. - if found.type_name.as_deref() != Some(DIRECTORY) { - return Err(ObjectError::NotADirectory { - component: component.clone(), - rest: components[at + 1..].join("\\"), - }); + match found.type_name.as_deref() { + Some(DIRECTORY) => {} + Some(_) => { + return Err(ObjectError::NotADirectory { + component: component.clone(), + rest: components[at + 1..].join("\\"), + }); + } + None => { + return Err(ObjectError::Untyped { + component: component.clone(), + }); + } } directory = found.address; if walked.len() > 1 { @@ -453,11 +478,19 @@ impl<'a> Namespace<'a> { "" => self.pointer_at(self.globals.root)?, path => { let found = self.object_at(path)?; - if found.type_name.as_deref() != Some(DIRECTORY) { - return Err(ObjectError::NotADirectory { - component: found.name, - rest: String::new(), - }); + match found.type_name.as_deref() { + Some(DIRECTORY) => {} + Some(_) => { + return Err(ObjectError::NotADirectory { + component: found.name, + rest: String::new(), + }); + } + None => { + return Err(ObjectError::Untyped { + component: found.name, + }); + } } found.address } @@ -507,7 +540,12 @@ impl<'a> Namespace<'a> { reason: "the link target is not a string this can vouch for", }); } - let target = self.unicode_at(at)?; + let (target, exact) = self.unicode_at(at)?; + if !exact { + return Err(ObjectError::Malformed { + reason: "the link target is not text, so it is not a path to follow", + }); + } if !target.starts_with('\\') { return Err(ObjectError::Malformed { reason: "the link target is not an object path, so this is not a target", @@ -542,12 +580,22 @@ fn components_of(path: &str) -> Result, ObjectError> { Ok(parts) } -/// UTF-16 little-endian, with anything unpaired replaced rather than refused: a name is being read -/// to show someone, and a lone surrogate in it is not a reason to lose the object. -fn utf16(bytes: &[u8]) -> String { +/// UTF-16 little-endian, and whether what came back **is** the name or only shows it. +/// +/// A name the object manager holds is a counted run of UTF-16 units, which is not the same thing +/// as text: an unpaired surrogate is a legal name and not a legal `String`. Replacing one keeps the +/// object listable, and the flag is what stops that rendering being used as an identity -- two +/// different names with a lone surrogate each render alike, so a walk matching on the rendering +/// resolves whichever came first, and a caller asking for the replacement character resolves an +/// object whose name has no such character in it. For a security question that is a device +/// answering under a name that is not its own. +fn utf16(bytes: &[u8]) -> (String, bool) { let (pairs, _) = bytes.as_chunks::<2>(); let units: Vec = pairs.iter().copied().map(u16::from_le_bytes).collect(); - String::from_utf16_lossy(&units) + match String::from_utf16(&units) { + Ok(exact) => (exact, true), + Err(_) => (String::from_utf16_lossy(&units), false), + } } impl DebugEngine { @@ -614,8 +662,8 @@ impl DebugEngine { Ok(Globals { root: self.symbol_offset("nt!ObpRootDirectoryObject")?, info_mask_to_offset: self.symbol_offset("nt!ObpInfoMaskToOffset")?, - header_cookie: self.symbol_offset("nt!ObHeaderCookie")?, - type_index_table: self.symbol_offset("nt!ObTypeIndexTable")?, + header_cookie: self.symbol_offset("nt!ObHeaderCookie").ok(), + type_index_table: self.symbol_offset("nt!ObTypeIndexTable").ok(), }) } @@ -695,8 +743,8 @@ mod tests { Globals { root: ROOT_POINTER, info_mask_to_offset: INFO_OFFSETS, - header_cookie: COOKIE, - type_index_table: TYPE_TABLE, + header_cookie: Some(COOKIE), + type_index_table: Some(TYPE_TABLE), } } @@ -771,6 +819,24 @@ mod tests { } } + /// An object whose name is written as raw UTF-16 units rather than as text, which is + /// what the object manager actually holds. + fn units_named(&mut self, body: u64, units: &[u16], type_index: u8) { + let header = body - 0x30; + self.put(header + 0x18, &[type_index]); + self.put(header + 0x1a, &[0x02]); + self.pointer(header + 0x28, 0); + self.put(INFO_OFFSETS + u64::from(0x02u8 & 0x03), &[0x20]); + let name_info = header - 0x20; + let bytes: Vec = units.iter().flat_map(|unit| unit.to_le_bytes()).collect(); + let length = bytes.len() as u16; + self.put(name_info + 0x08, &[0u8; 16]); + self.put(name_info + 0x08, &length.to_le_bytes()); + self.put(name_info + 0x0a, &length.to_le_bytes()); + self.pointer(name_info + 0x10, name_info + 0x1000); + self.put(name_info + 0x1000, &bytes); + } + /// The flags a symbolic link carries, which say which arm of its union is live. fn flags(&mut self, link: u64, flags: u32) { self.put(link + 0x1c, &flags.to_le_bytes()); @@ -1103,16 +1169,15 @@ mod tests { let namespace = Namespace::new(&fake, layout(), globals()); assert_eq!( namespace.object_at("\\Device\\MountPointManager"), - Err(ObjectError::NotADirectory { + Err(ObjectError::Untyped { component: "Device".to_string(), - rest: "MountPointManager".to_string(), }), - "an unreadable type is not a directory" + "it refuses, and says it could not tell rather than that this is something else" ); assert!( matches!( namespace.objects_in("\\Device"), - Err(ObjectError::NotADirectory { .. }) + Err(ObjectError::Untyped { .. }) ), "and listing it is the same refusal" ); @@ -1137,6 +1202,94 @@ mod tests { ); } + /// A name that is **not text** lists, and does not resolve. + /// + /// An unpaired surrogate is a legal object name and an illegal `String`. Rendering it with + /// replacements keeps the object visible, which is what a listing is for; matching on that + /// rendering would let two different names answer to one query, and would let a caller asking + /// for the replacement character reach an object whose name has no such character in it. For a + /// device that is answering under a name that is not its own. + #[test] + fn a_name_that_is_not_text_lists_but_does_not_resolve() { + let mut fake = namespace(); + const ODD: u64 = 0xffff_a000_0060_0000; + const OTHER: u64 = 0xffff_a000_0061_0000; + // Two different names, each with a lone high surrogate, which render identically. + fake.units_named(ODD, &[0x41, 0xd800, 0x42], obfuscated(4, ODD)); + fake.units_named(OTHER, &[0x41, 0xdbff, 0x42], obfuscated(4, OTHER)); + fake.directory(DEVICE_DIR, &[DEVICE, ODD, OTHER]); + let namespace = Namespace::new(&fake, layout(), globals()); + + let listed = namespace + .objects_in("\\Device") + .expect("the directory lists"); + let rendered: Vec<&str> = listed + .iter() + .filter(|one| !one.exact_name) + .map(|one| one.name.as_str()) + .collect(); + assert_eq!( + rendered.len(), + 2, + "both are listed, so neither object is lost: {listed:?}" + ); + assert_eq!( + rendered[0], rendered[1], + "and they render alike, which is what makes the rendering useless as an identity" + ); + + assert!( + matches!( + namespace.object_at(&format!("\\Device\\{}", rendered[0])), + Err(ObjectError::NotFound { .. }) + ), + "so neither answers to it" + ); + assert_eq!( + namespace + .object_at("\\Device\\MountPointManager") + .map(|found| found.address), + Ok(DEVICE), + "and the ordinary name beside them still resolves" + ); + } + + /// A target that cannot name **types** still answers everything that does not need one. + /// + /// Requiring the type globals was the previous round's answer and was too broad: resolving a + /// one-component path, listing the root, and reading a link target all need no type at all. The + /// guards still fail closed -- they just say [`ObjectError::Untyped`] when they cannot tell. + #[test] + fn a_target_that_cannot_name_types_still_answers_what_needs_none() { + let fake = namespace(); + let untyped = Globals { + header_cookie: None, + type_index_table: None, + ..globals() + }; + let namespace = Namespace::new(&fake, layout(), untyped); + + assert_eq!( + namespace.object_at("\\Device").map(|found| found.address), + Ok(DEVICE_DIR), + "one component needs no type" + ); + assert_eq!( + namespace + .objects_in("\\") + .map(|found| found.into_iter().map(|one| one.name).collect::>()), + Ok(vec!["Device".to_string()]), + "nor does listing the root" + ); + assert_eq!( + namespace.object_at("\\Device\\MountPointManager"), + Err(ObjectError::Untyped { + component: "Device".to_string() + }), + "and descending is refused, saying which of the two it is" + ); + } + /// A path that is not a path is refused before anything is read. #[test] fn a_path_that_is_not_one_is_refused() { From 9d0cf8baa2852acffc70adbdb049b08aff0f53ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 16:51:41 +0100 Subject: [PATCH 06/10] fix: an operation asks for the globals it reads, and a target is bounded as a path Two findings, and the second is the third round on one seam: an operation requiring more than it uses. Round three made the type globals required so the directory guards could fail closed; round three's own finding took them back for the operations that never descend; this one points out that reading a symbolic link still needed the root pointer and the optional-header table, neither of which it touches. So the seam goes rather than the symptom. **Every global is optional at resolution and required at use.** `Globals` records what the target has; `needs` asks for one where it is read and names it in the refusal. Walking a path wants the root, reading a name wants the offsets table, naming a type wants the cookie and the table, and reading a link wants none of them -- which it now demonstrably does, on a target that resolves nothing at all. The layout is deliberately *not* treated this way, and the comment says so, so the next round has the answer rather than the question: every type in it comes out of one PDB, so a partial answer there is not something a real target produces, while a global is a symbol that a build can rename. **And a link target is bounded as a path rather than as a name.** It was sharing `MAX_NAME_BYTES`, which is sized for one path *component* -- so an ordinary long target was refused, with a message about object names, which is the tell that the bound belonged to something else. One note on method. The first mutation I wrote for the globals rule passed, and it deserved to: it added a requirement inside a helper the operation under test never calls, so it changed a path the test does not take. The rule is "reading a link needs none of these", and the mutation that states it puts the requirement in `link_target` itself. That one fails, which is what makes the test worth having. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 132 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 112 insertions(+), 20 deletions(-) diff --git a/src/object.rs b/src/object.rs index 452b232..e6be679 100644 --- a/src/object.rs +++ b/src/object.rs @@ -53,12 +53,21 @@ const MAX_ENTRIES: usize = 65_536; /// The most path components a name may have. `\Device\HarddiskVolume1` is two. const MAX_COMPONENTS: usize = 32; -/// The longest object name this reads, in bytes of UTF-16. +/// The longest object **name** this reads, in bytes of UTF-16. /// /// `_UNICODE_STRING::Length` is a `USHORT`, so the structure's own limit is 64 KiB; a name that -/// long is not one the object manager made. +/// long is not one the object manager made. A name is one component and not a path -- the longest +/// in `\\Device` on an ordinary machine is a few dozen bytes. const MAX_NAME_BYTES: usize = 1024; +/// The longest **link target**, which is a different quantity and needs a bound of its own. +/// +/// A target is a whole path where a name is one component of one, so holding it to the name bound +/// refuses a link that is perfectly ordinary, with a message about object names. This is the +/// structure's own limit rounded down to something a path can reach: `UNICODE_STRING` counts bytes +/// in a `USHORT`, and `MAX_PATH` twice over in UTF-16 is well inside it. +const MAX_TARGET_BYTES: usize = 32_768; + /// What the object manager calls a directory's type, and the one type name this walk acts on. const DIRECTORY: &str = "Directory"; @@ -72,6 +81,10 @@ const DIRECTORY: &str = "Directory"; /// measured rather than guessed: the first bit anyone would have tried is the sandbox one. const SYMBOLIC_LINK_CALLBACK: u32 = 0x10; +/// The globals a walk names when it has to say which one is missing. +const ROOT_SYMBOL: &str = "nt!ObpRootDirectoryObject"; +const OFFSETS_SYMBOL: &str = "nt!ObpInfoMaskToOffset"; + /// Why a namespace walk could not answer. #[derive(Debug, Error, PartialEq, Eq)] pub enum ObjectError { @@ -103,6 +116,13 @@ pub enum ObjectError { /// exactly the case it is for -- but it refuses saying it could not tell. #[error("{component:?} could not be typed, so this walk will not descend through it")] Untyped { component: String }, + /// A global this operation needs is not one the target resolves. + /// + /// Per operation rather than per walk: reading a symbolic link needs none of the namespace's + /// globals, and taking it away because the root pointer was renamed would refuse something this + /// target can perfectly well answer. + #[error("this target does not resolve {what}, which this operation reads")] + Unavailable { what: &'static str }, /// A cap was reached, so what this could answer with is a **short** list. #[error("{what} exceeded its bound of {bound}, so this list would be shorter than the truth")] TooMany { what: &'static str, bound: usize }, @@ -171,10 +191,10 @@ pub struct Layout { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Globals { /// `nt!ObpRootDirectoryObject` — a pointer to the root directory, not the directory. - pub root: u64, + pub root: Option, /// `nt!ObpInfoMaskToOffset` — a byte per `InfoMask` combination, saying how far before the /// header the optional headers sit. - pub info_mask_to_offset: u64, + pub info_mask_to_offset: Option, /// `nt!ObHeaderCookie` and `nt!ObTypeIndexTable`, which together turn a header's obfuscated /// `TypeIndex` into a type object. Optional: a build without them still resolves names, and a /// `type_name` of `None` is the honest answer there. @@ -217,6 +237,17 @@ impl<'a> Namespace<'a> { } } + /// One global this operation cannot do without. + /// + /// **Asked for where it is used rather than where the set is built**, because the operations + /// here need different subsets and a walk that resolved the union of them would take reading a + /// symbolic link away from a target that merely renamed the root pointer. The layout is not + /// treated this way and deliberately: every type in it comes out of one PDB, so a partial + /// answer there is not a thing a real target produces. + fn needs(&self, global: Option, what: &'static str) -> Result { + global.ok_or(ObjectError::Unavailable { what }) + } + fn read(&self, at: u64, len: usize) -> Result, ObjectError> { match self.memory.read(at, len) { Some(bytes) if bytes.len() >= len => Ok(bytes), @@ -275,7 +306,12 @@ impl<'a> Namespace<'a> { } /// One `_UNICODE_STRING`, read from wherever it sits. - fn unicode_at(&self, at: u64) -> Result<(String, bool), ObjectError> { + fn unicode_at( + &self, + at: u64, + bound: usize, + what: &'static str, + ) -> Result<(String, bool), ObjectError> { let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; let bytes = self.read(at, size)?; let field = |offset: u32| { @@ -307,11 +343,8 @@ impl<'a> Namespace<'a> { reason: "a UNICODE_STRING's length is not a whole number of UTF-16 units", }); } - if length > MAX_NAME_BYTES { - return Err(ObjectError::TooMany { - what: "an object name", - bound: MAX_NAME_BYTES, - }); + if length > bound { + return Err(ObjectError::TooMany { what, bound }); } let raw = self.read(buffer, length)?; Ok(utf16(&raw[..length])) @@ -334,8 +367,7 @@ impl<'a> Namespace<'a> { // including* the one wanted, which is what the bit and every bit below it select. let index = mask & (INFO_MASK_NAME | (INFO_MASK_NAME - 1)); let distance = self.read( - self.globals - .info_mask_to_offset + self.needs(self.globals.info_mask_to_offset, OFFSETS_SYMBOL)? .wrapping_add(u64::from(index)), 1, )?[0]; @@ -347,6 +379,8 @@ impl<'a> Namespace<'a> { let name_info = header.wrapping_sub(u64::from(distance)); Ok(Some(self.unicode_at( name_info.wrapping_add(u64::from(self.layout.name_info_name)), + MAX_NAME_BYTES, + "an object name", )?)) } @@ -394,7 +428,11 @@ impl<'a> Namespace<'a> { return None; } let (name, _) = self - .unicode_at(entry.wrapping_add(u64::from(self.layout.type_name))) + .unicode_at( + entry.wrapping_add(u64::from(self.layout.type_name)), + MAX_NAME_BYTES, + "a type name", + ) .ok()?; (!name.is_empty()).then_some(name) } @@ -420,7 +458,7 @@ impl<'a> Namespace<'a> { /// Resolves a path to the object filed under it. pub fn object_at(&self, path: &str) -> Result { let components = components_of(path)?; - let mut directory = self.pointer_at(self.globals.root)?; + let mut directory = self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?; if directory == 0 { return Err(ObjectError::Malformed { reason: "the root directory pointer is null", @@ -475,7 +513,7 @@ impl<'a> Namespace<'a> { /// thirty-seven bucket pointers and whatever they hold followed as chains. pub fn objects_in(&self, path: &str) -> Result, ObjectError> { let directory = match path.trim_end_matches('\\') { - "" => self.pointer_at(self.globals.root)?, + "" => self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?, path => { let found = self.object_at(path)?; match found.type_name.as_deref() { @@ -540,7 +578,7 @@ impl<'a> Namespace<'a> { reason: "the link target is not a string this can vouch for", }); } - let (target, exact) = self.unicode_at(at)?; + let (target, exact) = self.unicode_at(at, MAX_TARGET_BYTES, "a link target")?; if !exact { return Err(ObjectError::Malformed { reason: "the link target is not text, so it is not a path to follow", @@ -660,8 +698,8 @@ impl DebugEngine { /// because a build that renamed or inlined them still resolves paths. pub fn object_globals(&self) -> Result { Ok(Globals { - root: self.symbol_offset("nt!ObpRootDirectoryObject")?, - info_mask_to_offset: self.symbol_offset("nt!ObpInfoMaskToOffset")?, + root: self.symbol_offset("nt!ObpRootDirectoryObject").ok(), + info_mask_to_offset: self.symbol_offset("nt!ObpInfoMaskToOffset").ok(), header_cookie: self.symbol_offset("nt!ObHeaderCookie").ok(), type_index_table: self.symbol_offset("nt!ObTypeIndexTable").ok(), }) @@ -741,8 +779,8 @@ mod tests { fn globals() -> Globals { Globals { - root: ROOT_POINTER, - info_mask_to_offset: INFO_OFFSETS, + root: Some(ROOT_POINTER), + info_mask_to_offset: Some(INFO_OFFSETS), header_cookie: Some(COOKIE), type_index_table: Some(TYPE_TABLE), } @@ -1290,6 +1328,60 @@ mod tests { ); } + /// A link is read on a target that resolves **none** of the namespace's globals. + /// + /// Reading one needs the symbolic link's own layout and nothing else: not the root pointer, not + /// the optional-header offsets, not the type table. Requiring the set would take an answer this + /// target can give away because of a symbol it never reads -- which is the third round of + /// findings this seam produced, and why the globals are now asked for one at a time. + #[test] + fn a_link_is_read_with_none_of_the_namespaces_globals() { + let mut fake = namespace(); + const LINK: u64 = 0xffff_a000_0045_0000; + fake.flags(LINK, 0); + fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\MountPointManager"); + let nothing = Globals { + root: None, + info_mask_to_offset: None, + header_cookie: None, + type_index_table: None, + }; + let namespace = Namespace::new(&fake, layout(), nothing); + + assert_eq!( + namespace.link_target(LINK).as_deref(), + Ok("\\Device\\MountPointManager"), + "the link reads, because it needs none of them" + ); + assert_eq!( + namespace.object_at("\\Device"), + Err(ObjectError::Unavailable { + what: "nt!ObpRootDirectoryObject" + }), + "and a walk says which one it wanted" + ); + } + + /// A link target is bounded as a **path**, not as a name. + /// + /// A name is one component and a target is a whole path, so holding the second to the first's + /// bound refuses an ordinary link -- with a message about object names, which is the tell. + #[test] + fn a_link_target_is_bounded_as_a_path_rather_than_as_a_name() { + let mut fake = namespace(); + const LINK: u64 = 0xffff_a000_0046_0000; + // Longer than a name may be, and far inside what a path may be. + let long = format!("\\Device\\{}", "D".repeat(MAX_NAME_BYTES)); + fake.flags(LINK, 0); + fake.string(LINK + 0x08, LINK + 0x1000, &long); + let namespace = Namespace::new(&fake, layout(), globals()); + assert_eq!( + namespace.link_target(LINK).as_deref(), + Ok(long.as_str()), + "a target this long is a path, not a name that got out of hand" + ); + } + /// A path that is not a path is refused before anything is read. #[test] fn a_path_that_is_not_one_is_refused() { From eed6699de9a2e38c984fc3a6b853bf09efbc2599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 17:02:41 +0100 Subject: [PATCH 07/10] fix: the flag is the discriminator, so the content rule goes, and no layout panics Two findings, and the first retires something I defended one round ago. **A target is returned whatever it looks like.** This required a leading backslash, which refuses `\KnownDlls\KnownDllPath` -- a real link whose target is the DOS path `C:\Windows\System32`. That rule was a stand-in for a discriminator, and it stopped being worth anything the moment there was a flag to ask: once `Flags` says the union holds a `UNICODE_STRING`, what is in it *is* the target and this has no business second-guessing it. What remains is structural -- a whole number of UTF-16 units, within its own maximum, addressing a buffer that reads. The justification I gave for keeping it was also wrong, and the correction is in the prose so the next reader has it: `ObpParseSymbolicLinkEx` does test a leading backslash two blocks before the flag, and it is testing the **remaining name** being parsed rather than the target. Reading one as the other is how a real link came to be refused. **And nothing panics on a layout this crate did not build.** `Layout` is public and so is `Namespace::new`, so every offset in it is a caller's to fill in and every one is an index into bytes this walk read. A pointer width of two had two bytes read and eight taken; a string field placed past the end of its structure is the same fault by another route. Both were a panic inside calls whose whole contract is that they return an error. The width is now refused rather than treated as eight, and field reads are bounds-checked against what was actually read -- so the class goes rather than the instance the finding named. Both mutations for that rule panic when backed out rather than going red, which is the defect stating itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 137 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 91 insertions(+), 46 deletions(-) diff --git a/src/object.rs b/src/object.rs index e6be679..330c9d7 100644 --- a/src/object.rs +++ b/src/object.rs @@ -255,14 +255,39 @@ impl<'a> Namespace<'a> { } } + /// A pointer, at whichever of the two widths this target uses. + /// + /// **Any other width is refused rather than treated as eight.** [`Layout`] is public and its + /// fields are a caller's to fill in, so a width of two would have this read two bytes and then + /// take eight of them — a panic, inside calls whose whole contract is that they return an + /// error. Nothing here trusts a layout to be one this crate built. fn pointer_at(&self, at: u64) -> Result { let bytes = self.read(at, self.layout.pointer)?; - Ok(match self.layout.pointer { - 4 => u64::from(u32::from_le_bytes( + match self.layout.pointer { + 4 => Ok(u64::from(u32::from_le_bytes( bytes[..4].try_into().unwrap_or_default(), + ))), + 8 => Ok(u64::from_le_bytes( + bytes[..8].try_into().unwrap_or_default(), )), - _ => u64::from_le_bytes(bytes[..8].try_into().unwrap_or_default()), - }) + _ => Err(ObjectError::Malformed { + reason: "a pointer on this target is neither four bytes nor eight", + }), + } + } + + /// Two bytes out of a structure this walk read, at an offset a [`Layout`] gave it. + /// + /// Bounds-checked for the same reason as the widths above: the offsets are public fields, and + /// one past the end of what was read is a panic where this owes an error. + fn field_at(bytes: &[u8], offset: u32) -> Result { + bytes + .get(offset as usize..) + .and_then(|rest| rest.first_chunk::<2>()) + .map(|pair| u16::from_le_bytes(*pair)) + .ok_or(ObjectError::Malformed { + reason: "a field sits outside the structure this layout describes", + }) } /// The object body every entry of a directory points at. @@ -314,14 +339,11 @@ impl<'a> Namespace<'a> { ) -> Result<(String, bool), ObjectError> { let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; let bytes = self.read(at, size)?; - let field = |offset: u32| { - u16::from_le_bytes(bytes[offset as usize..][..2].try_into().unwrap_or_default()) - }; - let length = field(self.layout.unicode_length) as usize; + let length = Self::field_at(&bytes, self.layout.unicode_length)? as usize; // **A string that is longer than its own buffer is torn**, and reading `Length` alone // takes whatever follows into a name the walk then matches paths against -- which // resolves some other object, rather than failing to resolve this one. - if length > field(self.layout.unicode_length + 2) as usize { + if length > Self::field_at(&bytes, self.layout.unicode_length + 2)? as usize { return Err(ObjectError::Malformed { reason: "a UNICODE_STRING is longer than its own maximum", }); @@ -543,11 +565,14 @@ impl<'a> Namespace<'a> { /// exactly on `Length` and `MaximumLength`, `CallbackContext` lands on `Buffer`, so decoding /// without looking reads a name out of whatever a context pointer happens to address. /// - /// So what is checked is what a **link target** is rather than what a string is. The lengths - /// have to be a string's, and then the thing they describe has to be an object path — which - /// begins at the root, with a backslash. A code address whose low halves happen to pass for - /// lengths gets that far and no further, because what its context points at does not begin - /// with one. + /// What remains after the flag is **structural** and nothing more: a whole number of UTF-16 + /// units, within its own maximum, addressing a buffer that reads. A target is deliberately not + /// held to looking like an object path, and the shape of that mistake is worth recording -- + /// this did require one to begin with a backslash, which refuses `\\KnownDlls\\KnownDllPath`, + /// whose target is the DOS path `C:\\Windows\\System32`. The kernel does test a leading + /// backslash two blocks earlier in that routine, and it is testing the **remaining name** being + /// parsed rather than the target; reading one as the other is how a real link came to be + /// refused. /// /// **And the discriminator is read first**, because the checks above are evidence and the flag /// is the answer: [`SYMBOLIC_LINK_CALLBACK`] is the bit the object manager itself branches on, @@ -568,11 +593,8 @@ impl<'a> Namespace<'a> { let at = link.wrapping_add(u64::from(self.layout.link_target)); let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; let bytes = self.read(at, size)?; - let field = |offset: u32| { - u16::from_le_bytes(bytes[offset as usize..][..2].try_into().unwrap_or_default()) - }; - let length = field(self.layout.unicode_length); - let maximum = field(self.layout.unicode_length + 2); + let length = Self::field_at(&bytes, self.layout.unicode_length)?; + let maximum = Self::field_at(&bytes, self.layout.unicode_length + 2)?; if length == 0 || !length.is_multiple_of(2) || length > maximum { return Err(ObjectError::Malformed { reason: "the link target is not a string this can vouch for", @@ -584,11 +606,6 @@ impl<'a> Namespace<'a> { reason: "the link target is not text, so it is not a path to follow", }); } - if !target.starts_with('\\') { - return Err(ObjectError::Malformed { - reason: "the link target is not an object path, so this is not a target", - }); - } Ok(target) } } @@ -1137,30 +1154,18 @@ mod tests { "a code address is not a whole number of UTF-16 units" ); - // **And the case the lengths do not catch**, which is the one that matters: a callback - // address whose low half is an even, non-zero, in-range length. Everything a string must - // satisfy holds, the context pointer is mapped, and what it addresses decodes perfectly - // well -- as text that is not an object path. Nothing about the *type* would have stopped - // this: a callback-backed link is a `SymbolicLink` like any other. - const PASSES: u64 = 0xffff_a000_0043_0000; - const CONTEXT: u64 = 0xffff_a000_0051_0000; - fake.flags(PASSES, 0); - fake.pointer(PASSES + 0x08, 0xffff_f805_cb41_0010); - fake.pointer(PASSES + 0x10, CONTEXT); - fake.put( - CONTEXT, - &"HeapFree" - .encode_utf16() - .flat_map(u16::to_le_bytes) - .collect::>(), - ); + // **A target that is not an object path at all**, which is ordinary rather than + // suspicious: `\\KnownDlls\\KnownDllPath` points at a DOS path. This walk used to require a + // leading backslash and refused exactly that link -- a content rule standing in for the + // flag, and wrong as soon as there was a flag to ask. + const DOS: u64 = 0xffff_a000_0043_0000; + fake.flags(DOS, 0); + fake.string(DOS + 0x08, DOS + 0x1000, "C:\\Windows\\System32"); let namespace = Namespace::new(&fake, layout(), globals()); assert_eq!( - namespace.link_target(PASSES), - Err(ObjectError::Malformed { - reason: "the link target is not an object path, so this is not a target" - }), - "the lengths passed, and what they described was not a target" + namespace.link_target(DOS).as_deref(), + Ok("C:\\Windows\\System32"), + "the flag said this is a target, so what it holds is the answer" ); } @@ -1382,6 +1387,46 @@ mod tests { ); } + /// A layout this crate did not build is **refused**, never panicked on. + /// + /// [`Layout`] is public and so is [`Namespace::new`], so its fields are a caller's to fill in + /// -- and every one of them is an index into bytes this walk read. A pointer width of two has + /// two bytes read and eight taken; an offset past the end of a structure is the same fault by + /// another route. Both are a panic inside calls whose whole contract is that they return an + /// error, so both are errors. + #[test] + fn a_layout_this_crate_did_not_build_is_refused_rather_than_panicked_on() { + let fake = namespace(); + + let narrow = Layout { + pointer: 2, + ..layout() + }; + assert_eq!( + Namespace::new(&fake, narrow, globals()).object_at("\\Device"), + Err(ObjectError::Malformed { + reason: "a pointer on this target is neither four bytes nor eight" + }) + ); + + // A string field placed past the end of the structure the walk reads for it. The link + // itself is whole, so what refuses this is the offset rather than a read that failed. + let mut fake = fake; + const LINK: u64 = 0xffff_a000_0047_0000; + fake.flags(LINK, 0); + fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); + let adrift = Layout { + unicode_length: 0x40, + ..layout() + }; + assert_eq!( + Namespace::new(&fake, adrift, globals()).link_target(LINK), + Err(ObjectError::Malformed { + reason: "a field sits outside the structure this layout describes" + }) + ); + } + /// A path that is not a path is refused before anything is read. #[test] fn a_path_that_is_not_one_is_refused() { From 1bb493d52258af1678446c9d5e1e1679815c2cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 17:17:40 +0100 Subject: [PATCH 08/10] fix: the layout is checked once, where it is built into a walk Two more findings on the same seam, which is the signal to stop answering them one at a time. The size arithmetic in `link_target` overflows before the width check it would eventually reach, and a bucket count of zero turns a full directory into an empty list while a huge one keeps the walk reading -- and before those, a width of two had two bytes read and eight taken, and a field offset past the end of its structure was taken from whatever followed. Four defects, one cause: `Layout` is public, so its fields are a caller's to fill in, and every read defended itself against the field it happened to use. So the check moves to `Namespace::new`, which now returns a `Result`. Two kinds of field, and the difference is worth stating rather than checking everything: a **width** or a **count** is what the walk sizes reads and loops from, so a wrong one panics or runs away and is refused. An **offset into a structure the walk reads whole** has to be inside what it reads, or the read succeeds and the field comes from past its end. An offset that is only added to an address is left alone, deliberately -- a wrong one reads somewhere else, which comes back as `Unreadable` naming the address, and there is nothing here to compare it against. The walk below can now rely on the layout, and there is no next field to find. Each of the four checks is pinned by disabling it alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 198 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 149 insertions(+), 49 deletions(-) diff --git a/src/object.rs b/src/object.rs index 330c9d7..bab7bce 100644 --- a/src/object.rs +++ b/src/object.rs @@ -68,6 +68,12 @@ const MAX_NAME_BYTES: usize = 1024; /// in a `USHORT`, and `MAX_PATH` twice over in UTF-16 is well inside it. const MAX_TARGET_BYTES: usize = 32_768; +/// The most hash buckets a directory may claim. +/// +/// Windows uses 37 and has for a long time. This is far above that and far below a count that +/// would have the walk read for minutes off a [`Layout`] nobody checked. +const MAX_BUCKETS: usize = 1024; + /// What the object manager calls a directory's type, and the one type name this walk acts on. const DIRECTORY: &str = "Directory"; @@ -187,6 +193,36 @@ pub struct Layout { pub type_name: u32, } +impl Layout { + /// Whether this describes a target's structures, or merely has the right field names. + /// + /// Two kinds of check, and the second is the one worth naming. A **width** and a **count** are + /// what the walk sizes reads and loops from, so a wrong one panics or runs away. An **offset** + /// into a structure the walk reads whole has to be inside what it reads, or the read succeeds + /// and the field is taken from past its end. Offsets that are only added to an address are not + /// checked: a wrong one reads somewhere else, which comes back as [`ObjectError::Unreadable`] + /// naming the address, and there is nothing this could compare it against anyway. + fn check(&self) -> Result<(), ObjectError> { + let bad = |reason| Err(ObjectError::Malformed { reason }); + if !matches!(self.pointer, 4 | 8) { + return bad("a pointer on this target is neither four bytes nor eight"); + } + if self.buckets == 0 || self.buckets > MAX_BUCKETS { + return bad("a directory's bucket count is not one a directory has"); + } + // A `UNICODE_STRING` is read whole: its two lengths, then its buffer. + let unicode = (self.unicode_buffer as usize) + self.pointer; + if (self.unicode_length as usize) + 4 > unicode { + return bad("a UNICODE_STRING's lengths sit outside the structure"); + } + // And the name header is read as far as the string inside it. + if (self.name_info_name as usize) + unicode > self.name_info_size as usize { + return bad("a name header's string sits outside the name header"); + } + Ok(()) + } +} + /// The globals the walk starts from, resolved by symbol. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Globals { @@ -229,12 +265,27 @@ pub struct Namespace<'a> { } impl<'a> Namespace<'a> { - pub fn new(memory: &'a dyn Memory, layout: Layout, globals: Globals) -> Self { - Self { + /// A walk over some memory, once the layout has been checked. + /// + /// **The check is here and not in each read**, which is the third answer this seam has had and + /// the one that ends it. [`Layout`] is public, so its fields are a caller's to fill in, and + /// every one of them is either an index into bytes this walk read or a count it loops on -- a + /// width of two had two bytes read and eight taken, a field past the end of its structure was + /// the same fault by another route, a bucket count of zero made an empty directory out of a + /// full one, and `usize::MAX` overflowed the size arithmetic before any of the guards those + /// produced could run. Defending each read found one more of these every round. Checking the + /// layout once means the walk below can rely on it, and there is no next one. + pub fn new( + memory: &'a dyn Memory, + layout: Layout, + globals: Globals, + ) -> Result { + layout.check()?; + Ok(Self { memory, layout, globals, - } + }) } /// One global this operation cannot do without. @@ -751,7 +802,7 @@ impl DebugEngine { reason: "this target does not resolve the object manager's globals", })?; let read = |at: u64, len: usize| self.read_memory(at, len).ok(); - answer(&Namespace::new(&read, layout, globals)) + answer(&Namespace::new(&read, layout, globals)?) } } @@ -943,7 +994,8 @@ mod tests { #[test] fn a_path_resolves_to_the_object_filed_under_it() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); let found = namespace .object_at("\\Device\\MountPointManager") @@ -969,7 +1021,8 @@ mod tests { #[test] fn a_name_is_matched_without_regard_to_case() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace .object_at("\\device\\MOUNTPOINTMANAGER") @@ -983,7 +1036,8 @@ mod tests { #[test] fn a_missing_component_names_the_directory_it_was_not_in() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.object_at("\\Device\\Nothing"), Err(ObjectError::NotFound { @@ -1000,7 +1054,8 @@ mod tests { #[test] fn a_leaf_is_not_walked_through() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.object_at("\\Device\\MountPointManager\\Deeper"), Err(ObjectError::NotADirectory { @@ -1014,7 +1069,8 @@ mod tests { #[test] fn a_directory_lists_what_it_holds() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace .objects_in("\\Device") @@ -1031,7 +1087,8 @@ mod tests { #[test] fn a_target_with_no_namespace_says_so_rather_than_answering_empty() { let fake = Fake::default(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.object_at("\\Device"), Err(ObjectError::Unreadable { @@ -1055,7 +1112,8 @@ mod tests { fake.pointer(DEVICE_DIR, EMPTY); fake.pointer(EMPTY, EMPTY); fake.pointer(EMPTY + 0x08, 0); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.objects_in("\\Device"), Err(ObjectError::TooMany { @@ -1073,7 +1131,8 @@ mod tests { #[test] fn a_leaf_is_not_listed_as_a_directory() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.objects_in("\\Device\\MountPointManager"), Err(ObjectError::NotADirectory { @@ -1095,7 +1154,8 @@ mod tests { fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); // Everything the link check looks at still holds; only the buffer is gone. fake.pointer(LINK + 0x10, 0); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(LINK), Err(ObjectError::Malformed { @@ -1112,7 +1172,8 @@ mod tests { let entry = DEVICE_DIR + 0x2000; fake.pointer(DEVICE_DIR, entry); fake.pointer(entry, entry); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.objects_in("\\Device"), Err(ObjectError::TooMany { @@ -1132,7 +1193,8 @@ mod tests { const LINK: u64 = 0xffff_a000_0040_0000; fake.flags(LINK, 0); fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\MountPointManager"); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(LINK).as_deref(), Ok("\\Device\\MountPointManager") @@ -1145,7 +1207,8 @@ mod tests { fake.flags(CALLBACK, 0); fake.pointer(CALLBACK + 0x08, 0xffff_f805_cb41_2341); fake.pointer(CALLBACK + 0x10, 0xffff_a000_0050_0000); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(CALLBACK), Err(ObjectError::Malformed { @@ -1161,7 +1224,8 @@ mod tests { const DOS: u64 = 0xffff_a000_0043_0000; fake.flags(DOS, 0); fake.string(DOS + 0x08, DOS + 0x1000, "C:\\Windows\\System32"); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(DOS).as_deref(), Ok("C:\\Windows\\System32"), @@ -1181,7 +1245,8 @@ mod tests { // A target that would decode perfectly well, and a flag saying it is not the live arm. fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); fake.flags(LINK, 0); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(LINK).as_deref(), Ok("\\Device\\X"), @@ -1189,7 +1254,8 @@ mod tests { ); fake.flags(LINK, SYMBOLIC_LINK_CALLBACK); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(LINK), Err(ObjectError::Malformed { @@ -1209,7 +1275,8 @@ mod tests { let mut fake = namespace(); // The slot the Device directory's own type index selects, emptied. fake.pointer(TYPE_TABLE + 3 * 8, 0); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.object_at("\\Device\\MountPointManager"), Err(ObjectError::Untyped { @@ -1236,7 +1303,8 @@ mod tests { let name_info = header - 0x20; // Length past MaximumLength, with the buffer left as it was. fake.put(name_info + 0x08, &200u16.to_le_bytes()); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.objects_in("\\Device"), Err(ObjectError::Malformed { @@ -1261,7 +1329,8 @@ mod tests { fake.units_named(ODD, &[0x41, 0xd800, 0x42], obfuscated(4, ODD)); fake.units_named(OTHER, &[0x41, 0xdbff, 0x42], obfuscated(4, OTHER)); fake.directory(DEVICE_DIR, &[DEVICE, ODD, OTHER]); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); let listed = namespace .objects_in("\\Device") @@ -1310,7 +1379,8 @@ mod tests { type_index_table: None, ..globals() }; - let namespace = Namespace::new(&fake, layout(), untyped); + let namespace = Namespace::new(&fake, layout(), untyped) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.object_at("\\Device").map(|found| found.address), @@ -1351,7 +1421,8 @@ mod tests { header_cookie: None, type_index_table: None, }; - let namespace = Namespace::new(&fake, layout(), nothing); + let namespace = Namespace::new(&fake, layout(), nothing) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(LINK).as_deref(), @@ -1379,7 +1450,8 @@ mod tests { let long = format!("\\Device\\{}", "D".repeat(MAX_NAME_BYTES)); fake.flags(LINK, 0); fake.string(LINK + 0x08, LINK + 0x1000, &long); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert_eq!( namespace.link_target(LINK).as_deref(), Ok(long.as_str()), @@ -1397,41 +1469,69 @@ mod tests { #[test] fn a_layout_this_crate_did_not_build_is_refused_rather_than_panicked_on() { let fake = namespace(); - - let narrow = Layout { - pointer: 2, - ..layout() + let refused = |layout: Layout| match Namespace::new(&fake, layout, globals()) { + Err(ObjectError::Malformed { reason }) => reason, + other => panic!("a layout that is not one was accepted: {:?}", other.is_ok()), }; + + // Every one of these was a separate defect before the layout was checked in one place: a + // width of two had two bytes read and eight taken, `usize::MAX` overflowed the size + // arithmetic before any read happened, no buckets made a full directory look empty, too + // many kept it reading, and a field past the end of its structure was taken from whatever + // followed. assert_eq!( - Namespace::new(&fake, narrow, globals()).object_at("\\Device"), - Err(ObjectError::Malformed { - reason: "a pointer on this target is neither four bytes nor eight" - }) + refused(Layout { + pointer: 2, + ..layout() + }), + "a pointer on this target is neither four bytes nor eight" ); - - // A string field placed past the end of the structure the walk reads for it. The link - // itself is whole, so what refuses this is the offset rather than a read that failed. - let mut fake = fake; - const LINK: u64 = 0xffff_a000_0047_0000; - fake.flags(LINK, 0); - fake.string(LINK + 0x08, LINK + 0x1000, "\\Device\\X"); - let adrift = Layout { - unicode_length: 0x40, - ..layout() - }; assert_eq!( - Namespace::new(&fake, adrift, globals()).link_target(LINK), - Err(ObjectError::Malformed { - reason: "a field sits outside the structure this layout describes" - }) + refused(Layout { + pointer: usize::MAX, + ..layout() + }), + "a pointer on this target is neither four bytes nor eight" ); + assert_eq!( + refused(Layout { + buckets: 0, + ..layout() + }), + "a directory's bucket count is not one a directory has" + ); + assert_eq!( + refused(Layout { + buckets: MAX_BUCKETS + 1, + ..layout() + }), + "a directory's bucket count is not one a directory has" + ); + assert_eq!( + refused(Layout { + unicode_length: 0x40, + ..layout() + }), + "a UNICODE_STRING's lengths sit outside the structure" + ); + assert_eq!( + refused(Layout { + name_info_name: 0x40, + ..layout() + }), + "a name header's string sits outside the name header" + ); + + // And the one this crate builds is accepted, so the check is not refusing everything. + assert!(Namespace::new(&fake, layout(), globals()).is_ok()); } /// A path that is not a path is refused before anything is read. #[test] fn a_path_that_is_not_one_is_refused() { let fake = namespace(); - let namespace = Namespace::new(&fake, layout(), globals()); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); assert!(matches!( namespace.object_at("Device"), Err(ObjectError::BadPath { .. }) From 6660564b7ed94a43ee8bd117198cd45b62a03091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 17:26:14 +0100 Subject: [PATCH 09/10] fix: a relation is not a magnitude, and the root is one path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, the first of which is a gap in the check I wrote last round rather than a new seam: it verified that the offsets are *ordered* the way a real layout's are and never that they are *small*. A buffer offset of nearly four gigabytes satisfies every ordering and then has the walk ask the target for a read that size, which is an allocation failure rather than a refusal — and on a 32-bit host the sums that check the ordering overflow before they are compared. So every offset is bounded before any of them is added to another. These describe structures of tens of bytes: `_OBJECT_HEADER` is 0x30, a name header 0x20, a `UNICODE_STRING` 0x10. Anything near a `u32`'s range is not one of them. **And the root is `\` and nothing else is.** `objects_in` trimmed trailing backslashes before deciding, which made the empty string the same value as the root — so a caller whose argument went missing got a successful listing of a directory it never asked about. `object_at` had always refused that, so one question had two answers depending on which door it came through. Both are pinned by disabling them alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/object.rs b/src/object.rs index bab7bce..e8193c9 100644 --- a/src/object.rs +++ b/src/object.rs @@ -68,6 +68,15 @@ const MAX_NAME_BYTES: usize = 1024; /// in a `USHORT`, and `MAX_PATH` twice over in UTF-16 is well inside it. const MAX_TARGET_BYTES: usize = 32_768; +/// The most any structure this walk reads may be, in bytes. +/// +/// Every one of them is tens of bytes: `_OBJECT_HEADER` is 0x30, a name header 0x20, a +/// `UNICODE_STRING` 0x10. A bound on the *relation* between offsets is not a bound on their size -- +/// a buffer offset of nearly four gigabytes satisfies every ordering this checks and then asks the +/// target for a read that large, which is an allocation failure rather than a refusal. So the +/// magnitudes are bounded too, and every sum that reaches this is checked. +const MAX_STRUCTURE: usize = 4096; + /// The most hash buckets a directory may claim. /// /// Windows uses 37 and has for a long time. This is far above that and far below a count that @@ -210,6 +219,29 @@ impl Layout { if self.buckets == 0 || self.buckets > MAX_BUCKETS { return bad("a directory's bucket count is not one a directory has"); } + // **Every offset is small before any of them is added to another.** These describe + // structures of tens of bytes, so anything near a `u32`'s range is not one of them -- and + // checking that first is what stops a sum overflowing on a 32-bit host and what stops a + // read being asked for in gigabytes. + let fields = [ + self.hash_buckets, + self.entry_chain, + self.entry_object, + self.header_body, + self.header_type_index, + self.header_info_mask, + self.header_security, + self.name_info_name, + self.name_info_size, + self.unicode_length, + self.unicode_buffer, + self.link_target, + self.link_flags, + self.type_name, + ]; + if fields.iter().any(|offset| *offset as usize > MAX_STRUCTURE) { + return bad("a field sits further into its structure than any of these reach"); + } // A `UNICODE_STRING` is read whole: its two lengths, then its buffer. let unicode = (self.unicode_buffer as usize) + self.pointer; if (self.unicode_length as usize) + 4 > unicode { @@ -585,6 +617,16 @@ impl<'a> Namespace<'a> { /// of the path, and without it `\Device\MountPointManager` has a driver's own fields read as /// thirty-seven bucket pointers and whatever they hold followed as chains. pub fn objects_in(&self, path: &str) -> Result, ObjectError> { + // **The root is `\\`, and nothing else is.** Trimming first made the empty string the + // same value, so a caller whose argument was missing listed the root and got a successful + // answer about a directory it never asked for. `object_at` has always refused that; this + // refuses it the same way rather than having two answers to one question. + if !path.starts_with('\\') { + return Err(ObjectError::BadPath { + path: path.to_string(), + reason: "an object path begins at the root, with a backslash", + }); + } let directory = match path.trim_end_matches('\\') { "" => self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?, path => { @@ -1521,11 +1563,56 @@ mod tests { }), "a name header's string sits outside the name header" ); + // **A relation is not a magnitude**, which is the gap the checks above left: these offsets + // are ordered exactly as a real layout's are, and describe a structure of nearly four + // gigabytes. What that reaches is a read asked for in gigabytes, which is an allocation + // failure rather than a refusal. + assert_eq!( + refused(Layout { + pointer: 4, + unicode_buffer: u32::MAX - 4, + name_info_name: 0, + name_info_size: u32::MAX, + ..layout() + }), + "a field sits further into its structure than any of these reach" + ); // And the one this crate builds is accepted, so the check is not refusing everything. assert!(Namespace::new(&fake, layout(), globals()).is_ok()); } + /// Listing with **no path at all** is refused, not answered about the root. + /// + /// `\\` is the root and nothing else is. Trimming first made the empty string the same + /// value, so an argument that went missing came back as a successful listing of a directory + /// nobody asked about -- and `object_at` had always refused it, so one question had two + /// answers. + #[test] + fn listing_with_no_path_is_refused_rather_than_answered_about_the_root() { + let fake = namespace(); + let namespace = Namespace::new(&fake, layout(), globals()) + .expect("the fixture layout is one this crate builds"); + assert!( + matches!(namespace.objects_in(""), Err(ObjectError::BadPath { .. })), + "an empty path is not the root" + ); + assert!( + matches!( + namespace.objects_in("Device"), + Err(ObjectError::BadPath { .. }) + ), + "and neither is one that does not begin at it" + ); + assert_eq!( + namespace + .objects_in("\\") + .map(|found| found.into_iter().map(|one| one.name).collect::>()), + Ok(vec!["Device".to_string()]), + "while the root itself still lists" + ); + } + /// A path that is not a path is refused before anything is read. #[test] fn a_path_that_is_not_one_is_refused() { From 75ce3527b4862b0d2bcbf08d452f131f52319d59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Carvalho?= Date: Sat, 12 Sep 2026 17:32:13 +0100 Subject: [PATCH 10/10] fix: one path parser, and an empty component is refused rather than dropped Third disagreement between two path parsers, so the second one goes. `objects_in` had its own handling and it was lenient in a way `object_at` was not: the empty string listed the root, then a path of nothing but separators listed the root, because trimming trailing backslashes turns both into the same value. Each was fixed where it was found. The parser they were supposed to share was lenient too -- it *filtered* empty components rather than refusing them, so `\Device\X` quietly became `\Device\X` and `\` quietly became the root. So `components_of` refuses an empty component, answers the root as a path with no components in it, and both callers ask it. `object_at` refuses that answer because the root is a directory rather than an object in one; `objects_in` is the call that means it. One trailing separator stays a caller's convenience, which is the one piece of leniency that was deliberate. The test now walks every spelling that used to reach the root by a different route, and asserts the two calls agree on each. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf --- src/object.rs | 91 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 32 deletions(-) diff --git a/src/object.rs b/src/object.rs index e8193c9..70ac020 100644 --- a/src/object.rs +++ b/src/object.rs @@ -563,6 +563,14 @@ impl<'a> Namespace<'a> { /// Resolves a path to the object filed under it. pub fn object_at(&self, path: &str) -> Result { let components = components_of(path)?; + // The root is a directory rather than an object in one, so there is nothing here to + // resolve. `objects_in` is the call that answers about it. + if components.is_empty() { + return Err(ObjectError::BadPath { + path: path.to_string(), + reason: "it names the root directory rather than an object in it", + }); + } let mut directory = self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?; if directory == 0 { return Err(ObjectError::Malformed { @@ -617,19 +625,13 @@ impl<'a> Namespace<'a> { /// of the path, and without it `\Device\MountPointManager` has a driver's own fields read as /// thirty-seven bucket pointers and whatever they hold followed as chains. pub fn objects_in(&self, path: &str) -> Result, ObjectError> { - // **The root is `\\`, and nothing else is.** Trimming first made the empty string the - // same value, so a caller whose argument was missing listed the root and got a successful - // answer about a directory it never asked for. `object_at` has always refused that; this - // refuses it the same way rather than having two answers to one question. - if !path.starts_with('\\') { - return Err(ObjectError::BadPath { - path: path.to_string(), - reason: "an object path begins at the root, with a backslash", - }); - } - let directory = match path.trim_end_matches('\\') { - "" => self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?, - path => { + // **The same parser `object_at` uses**, which is the point rather than a tidy-up: this + // had its own, and the two disagreed twice -- an empty argument listed the root, and so + // did a path of nothing but separators, both of which `object_at` refused. One question + // with two answers depending on which door it came through. + let directory = match components_of(path)?.is_empty() { + true => self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?, + false => { let found = self.object_at(path)?; match found.type_name.as_deref() { Some(DIRECTORY) => {} @@ -712,15 +714,19 @@ fn components_of(path: &str) -> Result, ObjectError> { if !path.starts_with('\\') { return Err(bad("an object path begins at the root, with a backslash")); } - let parts: Vec = path - .split('\\') - .filter(|part| !part.is_empty()) - .map(str::to_string) - .collect(); - if parts.is_empty() { - return Err(bad( - "it names the root directory rather than an object in it", - )); + // The root, which is the one path with no components in it. + if path == "\\" { + return Ok(Vec::new()); + } + // One trailing separator is a caller's convenience and is dropped. **An empty component + // anywhere else is refused rather than dropped**, which is where the leniency here used to + // be: filtering them turned `\\Device\\\\X` into `\\Device\\X` and `\\\\` into the root, so a path + // that is not one quietly became a path that is -- and a listing answered about a directory + // nobody named. + let body = path.strip_suffix('\\').unwrap_or(path); + let parts: Vec = body[1..].split('\\').map(str::to_string).collect(); + if parts.iter().any(String::is_empty) { + return Err(bad("it has a component with no name in it")); } if parts.len() > MAX_COMPONENTS { return Err(bad("it has more components than the namespace is deep")); @@ -1593,23 +1599,44 @@ mod tests { let fake = namespace(); let namespace = Namespace::new(&fake, layout(), globals()) .expect("the fixture layout is one this crate builds"); - assert!( - matches!(namespace.objects_in(""), Err(ObjectError::BadPath { .. })), - "an empty path is not the root" - ); + + // **Every one of these used to list the root**, each through a different hole in a parser + // `objects_in` kept for itself: nothing at all, a path that never begins at the root, and + // a path of separators with no name between them. `object_at` refused all three. + for path in ["", "Device", "\\\\", "\\\\\\"] { + assert!( + matches!(namespace.objects_in(path), Err(ObjectError::BadPath { .. })), + "{path:?} is not the root" + ); + assert!( + matches!(namespace.object_at(path), Err(ObjectError::BadPath { .. })), + "{path:?} is not an object either, and the two agree now" + ); + } + + // Nor is a path whose components are not all named. assert!( matches!( - namespace.objects_in("Device"), + namespace.object_at("\\Device\\\\MountPointManager"), Err(ObjectError::BadPath { .. }) ), - "and neither is one that does not begin at it" + "an empty component is refused rather than dropped" ); - assert_eq!( + + let root = |path: &str| { namespace - .objects_in("\\") - .map(|found| found.into_iter().map(|one| one.name).collect::>()), + .objects_in(path) + .map(|found| found.into_iter().map(|one| one.name).collect::>()) + }; + assert_eq!( + root("\\"), Ok(vec!["Device".to_string()]), - "while the root itself still lists" + "while the root itself lists" + ); + assert_eq!( + namespace.objects_in("\\Device\\").map(|found| found.len()), + Ok(1), + "and one trailing separator is still a caller's convenience" ); }