From 39944dc97e054582c1351e354bf55d46cb241a31 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sun, 8 Feb 2026 18:01:33 -0500 Subject: [PATCH 01/15] feat: implement Close, Read, and QueryInfo handlers for E2E file read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the server-side handlers needed for a complete file read flow: Create → QueryInfo → Read → Close. Protocol types: - Add accessor methods to SMBCloseRequest, SMBReadRequest, SMBQueryInfoRequest - Add constructors for SMBCloseResponse, SMBReadResponse, SMBQueryInfoResponse - Make close::flags and query_info::info_type modules public Trait extensions: - Add read_data() to ResourceHandle trait with implementations for SMBFileSystemHandle (seeks + reads) and SMBIPCHandle (stub) - Add read_data() to Open trait, delegating to the underlying handle - Add open_table_mut() to Session trait for close cleanup - Add remove_open() to Server trait for global open table cleanup - Fix SMBOpen::inner() todo!() → return None (terminal handler) Handlers (tree_connect.rs): - handle_close: returns file metadata if POSTQUERY_ATTRIB flag set, removes open from server (outer) then session (inner) tables - handle_read: reads data from the open's underlying handle, enforces minimum_count, returns SMBReadResponse - handle_query_info: supports FileBasicInformation (class 4), FileStandardInformation (class 5), FileNetworkOpenInformation (class 34) per MS-FSCC; returns InvalidInfoClass for unsupported types - Fix lock ordering in handle_create: server write before session write Lock ordering: all handlers acquire locks outer→inner (server → connection → session → open), readers before writers. NTStatus: add FileClosed, EndOfFile, InvalidInfoClass, InvalidDeviceRequest Tests: - 13 unit tests for request accessor and response constructor round-trips - 3 integration tests (smbclient E2E: file read, dir listing, missing file) --- smb-core/src/nt_status.rs | 4 + smb/src/protocol/body/close/mod.rs | 130 ++++++++++++++- smb/src/protocol/body/query_info/mod.rs | 94 ++++++++++- smb/src/protocol/body/read/mod.rs | 97 ++++++++++++ smb/src/server/mod.rs | 5 + smb/src/server/open.rs | 7 +- smb/src/server/session.rs | 5 + smb/src/server/share/file_system.rs | 17 ++ smb/src/server/share/ipc.rs | 6 + smb/src/server/share/mod.rs | 5 + smb/src/server/tree_connect.rs | 198 ++++++++++++++++++++++- smb/tests/smbclient.rs | 200 ++++++++++++++++++++++++ 12 files changed, 760 insertions(+), 8 deletions(-) diff --git a/smb-core/src/nt_status.rs b/smb-core/src/nt_status.rs index 9c6e60d..86a8fdc 100644 --- a/smb-core/src/nt_status.rs +++ b/smb-core/src/nt_status.rs @@ -19,6 +19,10 @@ pub enum NTStatus { UserSessionDeleted = 0xC0000203, NetworkSessionExpired = 0xC000035C, FileNotAvailable = 0xC0000467, + FileClosed = 0xC0000128, + EndOfFile = 0xC0000011, + InvalidInfoClass = 0xC0000003, + InvalidDeviceRequest = 0xC0000010, UnknownError = 0xFFFFFFFF, } diff --git a/smb/src/protocol/body/close/mod.rs b/smb/src/protocol/body/close/mod.rs index 0b08bb6..5d4d34a 100644 --- a/smb/src/protocol/body/close/mod.rs +++ b/smb/src/protocol/body/close/mod.rs @@ -9,7 +9,7 @@ use crate::protocol::body::create::file_attributes::SMBFileAttributes; use crate::protocol::body::create::file_id::SMBFileId; use crate::protocol::body::filetime::FileTime; -mod flags; +pub mod flags; #[derive( Debug, PartialEq, Eq, SMBByteSize, SMBToBytes, SMBFromBytes, Serialize, Deserialize, Clone, @@ -24,6 +24,16 @@ pub struct SMBCloseRequest { file_id: SMBFileId, } +impl SMBCloseRequest { + pub fn flags(&self) -> SMBCloseFlags { + self.flags + } + + pub fn file_id(&self) -> &SMBFileId { + &self.file_id + } +} + #[derive( Debug, PartialEq, Eq, SMBByteSize, SMBToBytes, SMBFromBytes, Serialize, Deserialize, Clone, )] @@ -48,3 +58,121 @@ pub struct SMBCloseResponse { #[smb_direct(start(fixed = 56))] file_attributes: SMBFileAttributes, } + +impl SMBCloseResponse { + pub fn from_metadata( + metadata: &crate::server::share::SMBFileMetadata, + attributes: SMBFileAttributes, + ) -> Self { + Self { + flags: SMBCloseFlags::POSTQUERY_ATTRIB, + reserved: PhantomData, + creation_time: metadata.creation_time.clone(), + last_access_time: metadata.last_access_time.clone(), + last_write_time: metadata.last_write_time.clone(), + change_time: metadata.last_modification_time.clone(), + allocation_size: metadata.allocated_size, + end_of_file: metadata.actual_size, + file_attributes: attributes, + } + } + + pub fn empty() -> Self { + Self { + flags: SMBCloseFlags::empty(), + reserved: PhantomData, + creation_time: FileTime::zero(), + last_access_time: FileTime::zero(), + last_write_time: FileTime::zero(), + change_time: FileTime::zero(), + allocation_size: 0, + end_of_file: 0, + file_attributes: SMBFileAttributes::empty(), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; + + #[test] + fn close_response_empty_has_zero_fields() { + let resp = SMBCloseResponse::empty(); + assert_eq!(resp.flags, SMBCloseFlags::empty()); + assert_eq!(resp.allocation_size, 0); + assert_eq!(resp.end_of_file, 0); + assert_eq!(resp.file_attributes, SMBFileAttributes::empty()); + } + + #[test] + fn close_response_empty_serialization_round_trip() { + let resp = SMBCloseResponse::empty(); + let bytes = resp.smb_to_bytes(); + assert_eq!(bytes.len(), resp.smb_byte_size()); + let (_, parsed) = SMBCloseResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn close_response_from_metadata_sets_postquery_flag() { + use crate::server::share::SMBFileMetadata; + let metadata = SMBFileMetadata { + creation_time: FileTime::from_unix(1700000000), + last_access_time: FileTime::from_unix(1700000100), + last_write_time: FileTime::from_unix(1700000200), + last_modification_time: FileTime::from_unix(1700000300), + allocated_size: 4096, + actual_size: 1024, + }; + let resp = SMBCloseResponse::from_metadata(&metadata, SMBFileAttributes::NORMAL); + assert!(resp.flags.contains(SMBCloseFlags::POSTQUERY_ATTRIB)); + assert_eq!(resp.allocation_size, 4096); + assert_eq!(resp.end_of_file, 1024); + assert_eq!(resp.file_attributes, SMBFileAttributes::NORMAL); + } + + #[test] + fn close_response_from_metadata_serialization_round_trip() { + use crate::server::share::SMBFileMetadata; + let metadata = SMBFileMetadata { + creation_time: FileTime::from_unix(1700000000), + last_access_time: FileTime::from_unix(1700000100), + last_write_time: FileTime::from_unix(1700000200), + last_modification_time: FileTime::from_unix(1700000300), + allocated_size: 8192, + actual_size: 2048, + }; + let resp = SMBCloseResponse::from_metadata(&metadata, SMBFileAttributes::ARCHIVE); + let bytes = resp.smb_to_bytes(); + assert_eq!(bytes.len(), resp.smb_byte_size()); + let (_, parsed) = SMBCloseResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn close_request_accessors() { + let file_id = SMBFileId { + persistent: 42, + volatile: 99, + }; + let bytes = { + let mut buf = Vec::new(); + // struct_size (u16) = 24 + buf.extend_from_slice(&24u16.to_le_bytes()); + // flags (u16) = POSTQUERY_ATTRIB = 0x0001 + buf.extend_from_slice(&1u16.to_le_bytes()); + // reserved (4 bytes) + buf.extend_from_slice(&[0u8; 4]); + // file_id: persistent (u64) + volatile (u64) + buf.extend_from_slice(&42u64.to_le_bytes()); + buf.extend_from_slice(&99u64.to_le_bytes()); + buf + }; + let (_, req) = SMBCloseRequest::smb_from_bytes(&bytes).unwrap(); + assert_eq!(req.file_id().persistent, file_id.persistent); + assert_eq!(req.file_id().volatile, file_id.volatile); + assert!(req.flags().contains(SMBCloseFlags::POSTQUERY_ATTRIB)); + } +} diff --git a/smb/src/protocol/body/query_info/mod.rs b/smb/src/protocol/body/query_info/mod.rs index b66b2af..7b10436 100644 --- a/smb/src/protocol/body/query_info/mod.rs +++ b/smb/src/protocol/body/query_info/mod.rs @@ -10,7 +10,7 @@ use crate::protocol::body::query_info::info_type::SMBInfoType; use crate::protocol::body::query_info::security_information::SMBSecurityInformation; mod flags; -mod info_type; +pub mod info_type; mod security_information; #[derive( @@ -39,6 +39,24 @@ pub struct SMBQueryInfoRequest { buffer: Vec, } +impl SMBQueryInfoRequest { + pub fn info_type(&self) -> SMBInfoType { + self.info_type + } + + pub fn file_info_class(&self) -> u8 { + self.file_info_class + } + + pub fn output_buffer_length(&self) -> u32 { + self.output_buffer_length + } + + pub fn file_id(&self) -> &SMBFileId { + &self.file_id + } +} + #[derive( Debug, PartialEq, Eq, SMBByteSize, SMBToBytes, SMBFromBytes, Serialize, Deserialize, Clone, )] @@ -54,3 +72,77 @@ pub struct SMBQueryInfoResponse { )] data: Vec, } + +impl SMBQueryInfoResponse { + pub fn new(data: Vec) -> Self { + Self { + reserved: PhantomData, + data, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; + + #[test] + fn query_info_response_new_sets_data() { + let data = vec![1, 2, 3, 4, 5, 6, 7, 8]; + let resp = SMBQueryInfoResponse::new(data.clone()); + assert_eq!(resp.data, data); + } + + #[test] + fn query_info_response_serialization_round_trip() { + let resp = SMBQueryInfoResponse::new(vec![0xAA; 40]); + let bytes = resp.smb_to_bytes(); + assert_eq!(bytes.len(), resp.smb_byte_size()); + let (_, parsed) = SMBQueryInfoResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn query_info_response_empty_data_round_trip() { + let resp = SMBQueryInfoResponse::new(vec![]); + let bytes = resp.smb_to_bytes(); + let (_, parsed) = SMBQueryInfoResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn query_info_request_accessors() { + let bytes = { + let mut buf = Vec::new(); + // struct_size (u16) = 41 + buf.extend_from_slice(&41u16.to_le_bytes()); + // info_type (u8) = 0 (File) + buf.push(0); + // file_info_class (u8) = 4 (FileBasicInformation) + buf.push(4); + // output_buffer_length (u32) = 4096 + buf.extend_from_slice(&4096u32.to_le_bytes()); + // input_buffer_offset (u16) = 0 + buf.extend_from_slice(&0u16.to_le_bytes()); + // reserved (u16) = 0 + buf.extend_from_slice(&0u16.to_le_bytes()); + // input_buffer_length (u32) = 0 + buf.extend_from_slice(&0u32.to_le_bytes()); + // additional_information (u32) = 0 + buf.extend_from_slice(&0u32.to_le_bytes()); + // flags (u32) = 0 + buf.extend_from_slice(&0u32.to_le_bytes()); + // file_id: persistent (u64) + volatile (u64) + buf.extend_from_slice(&55u64.to_le_bytes()); + buf.extend_from_slice(&77u64.to_le_bytes()); + buf + }; + let (_, req) = SMBQueryInfoRequest::smb_from_bytes(&bytes).unwrap(); + assert_eq!(req.info_type(), SMBInfoType::File); + assert_eq!(req.file_info_class(), 4); + assert_eq!(req.output_buffer_length(), 4096); + assert_eq!(req.file_id().persistent, 55); + assert_eq!(req.file_id().volatile, 77); + } +} diff --git a/smb/src/protocol/body/read/mod.rs b/smb/src/protocol/body/read/mod.rs index 14ea887..166f40f 100644 --- a/smb/src/protocol/body/read/mod.rs +++ b/smb/src/protocol/body/read/mod.rs @@ -37,6 +37,24 @@ pub struct SMBReadRequest { channel_information: Vec, } +impl SMBReadRequest { + pub fn file_id(&self) -> &SMBFileId { + &self.file_id + } + + pub fn read_length(&self) -> u32 { + self.read_length + } + + pub fn read_offset(&self) -> u64 { + self.read_offset + } + + pub fn minimum_count(&self) -> u32 { + self.minimum_count + } +} + #[derive( Debug, PartialEq, Eq, SMBByteSize, SMBToBytes, SMBFromBytes, Serialize, Deserialize, Clone, )] @@ -55,3 +73,82 @@ pub struct SMBReadResponse { )] data: Vec, } + +impl SMBReadResponse { + pub fn new(data: Vec, data_remaining: u32) -> Self { + Self { + reserved: PhantomData, + data_remaining, + flags: SMBReadResponseFlags::None, + data, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; + + #[test] + fn read_response_new_sets_fields() { + let data = vec![0xDE, 0xAD, 0xBE, 0xEF]; + let resp = SMBReadResponse::new(data.clone(), 100); + assert_eq!(resp.data, data); + assert_eq!(resp.data_remaining, 100); + assert_eq!(resp.flags, SMBReadResponseFlags::None); + } + + #[test] + fn read_response_serialization_round_trip() { + let resp = SMBReadResponse::new(vec![1, 2, 3, 4, 5], 0); + let bytes = resp.smb_to_bytes(); + assert_eq!(bytes.len(), resp.smb_byte_size()); + let (_, parsed) = SMBReadResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn read_response_empty_data() { + let resp = SMBReadResponse::new(vec![], 0); + let bytes = resp.smb_to_bytes(); + let (_, parsed) = SMBReadResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn read_request_accessors() { + let bytes = { + let mut buf = Vec::new(); + // struct_size (u16) = 49 + buf.extend_from_slice(&49u16.to_le_bytes()); + // padding (u8) + buf.push(0); + // flags (u8) = 0 + buf.push(0); + // read_length (u32) = 1024 + buf.extend_from_slice(&1024u32.to_le_bytes()); + // read_offset (u64) = 512 + buf.extend_from_slice(&512u64.to_le_bytes()); + // file_id: persistent (u64) + volatile (u64) + buf.extend_from_slice(&10u64.to_le_bytes()); + buf.extend_from_slice(&20u64.to_le_bytes()); + // minimum_count (u32) = 256 + buf.extend_from_slice(&256u32.to_le_bytes()); + // channel (u32) = 0 + buf.extend_from_slice(&0u32.to_le_bytes()); + // remaining_bytes (u32) = 0 + buf.extend_from_slice(&0u32.to_le_bytes()); + // channel_info_offset (u16) = 0, channel_info_length (u16) = 0 + buf.extend_from_slice(&0u16.to_le_bytes()); + buf.extend_from_slice(&0u16.to_le_bytes()); + buf + }; + let (_, req) = SMBReadRequest::smb_from_bytes(&bytes).unwrap(); + assert_eq!(req.read_length(), 1024); + assert_eq!(req.read_offset(), 512); + assert_eq!(req.minimum_count(), 256); + assert_eq!(req.file_id().persistent, 10); + assert_eq!(req.file_id().volatile, 20); + } +} diff --git a/smb/src/server/mod.rs b/smb/src/server/mod.rs index 18a3d08..35677d9 100644 --- a/smb/src/server/mod.rs +++ b/smb/src/server/mod.rs @@ -57,6 +57,7 @@ pub trait Server: Send + Sync { fn shares(&self) -> &HashMap>; fn opens(&self) -> &HashMap>>; fn add_open(&mut self, open: Arc>) -> impl Future; + fn remove_open(&mut self, global_id: u32); fn sessions(&self) -> &HashMap>>; fn sessions_mut(&mut self) -> &mut HashMap>>; fn guid(&self) -> Uuid; @@ -228,6 +229,10 @@ impl< 0 } + fn remove_open(&mut self, global_id: u32) { + self.open_table.remove(&global_id); + } + fn sessions(&self) -> &HashMap>> { &self.session_table } diff --git a/smb/src/server/open.rs b/smb/src/server/open.rs index 4b32141..7002ca7 100644 --- a/smb/src/server/open.rs +++ b/smb/src/server/open.rs @@ -32,6 +32,7 @@ pub trait Open: Send + Sync { fn file_attributes(&self) -> SMBFileAttributes; fn file_id(&self) -> SMBFileId; fn file_metadata(&self) -> SMBResult; + fn read_data(&mut self, offset: u64, length: u32) -> SMBResult>; } pub struct SMBOpen { @@ -149,6 +150,10 @@ impl Open for SMBOpen { fn file_metadata(&self) -> SMBResult { self.underlying.metadata() } + + fn read_data(&mut self, offset: u64, length: u32) -> SMBResult> { + self.underlying.read_data(offset, length) + } } // TODO: From MS-FSCC section 2.6 @@ -234,7 +239,7 @@ impl SMBLockedMessageHandlerBase for Arc> { type Inner = (); async fn inner(&self, _message: &SMBMessageType) -> Option { - todo!() + None } } diff --git a/smb/src/server/session.rs b/smb/src/server/session.rs index 5bf7865..5b7213a 100644 --- a/smb/src/server/session.rs +++ b/smb/src/server/session.rs @@ -63,6 +63,7 @@ pub trait Session: Send + Sync { fn provider(&self) -> &Arc; fn encrypt_data(&self) -> bool; fn open_table(&self) -> &HashMap>>; + fn open_table_mut(&mut self) -> &mut HashMap>>; fn add_open(&mut self, open: Arc>) -> impl Future; fn set_previous_file_id(&mut self, file_id: SMBFileId); fn signing_key(&self) -> &[u8]; @@ -397,6 +398,10 @@ impl> Session &self.open_table } + fn open_table_mut(&mut self) -> &mut HashMap>> { + &mut self.open_table + } + async fn add_open(&mut self, open: Arc>) { let id = Self::get_next_map_id(&self.open_table); let mut open_wr = open.write().await; diff --git a/smb/src/server/share/file_system.rs b/smb/src/server/share/file_system.rs index ab0a9a1..5563843 100644 --- a/smb/src/server/share/file_system.rs +++ b/smb/src/server/share/file_system.rs @@ -2,6 +2,7 @@ use std::any::Any; use std::fmt::{Debug, Formatter}; use std::fs; use std::fs::{File, OpenOptions, ReadDir}; +use std::io::{Read, Seek, SeekFrom}; use std::marker::PhantomData; use std::time::{SystemTime, UNIX_EPOCH}; @@ -103,6 +104,22 @@ impl ResourceHandle for SMBFileSystemHandle { actual_size: metadata.len(), }) } + + fn read_data(&mut self, offset: u64, length: u32) -> SMBResult> { + match &mut self.resource { + SMBFileSystemResourceHandle::File(file) => { + file.seek(SeekFrom::Start(offset)) + .map_err(SMBError::io_error)?; + let mut buf = vec![0u8; length as usize]; + let bytes_read = file.read(&mut buf).map_err(SMBError::io_error)?; + buf.truncate(bytes_read); + Ok(buf) + } + SMBFileSystemResourceHandle::Directory(_) => Err(SMBError::response_error( + smb_core::nt_status::NTStatus::InvalidDeviceRequest, + )), + } + } } impl SMBFileSystemResourceHandle { diff --git a/smb/src/server/share/ipc.rs b/smb/src/server/share/ipc.rs index 9579e8e..aaaff28 100644 --- a/smb/src/server/share/ipc.rs +++ b/smb/src/server/share/ipc.rs @@ -45,6 +45,12 @@ impl ResourceHandle for SMBIPCHandle { actual_size: 0, }) } + + fn read_data(&mut self, _offset: u64, _length: u32) -> SMBResult> { + Err(SMBError::response_error( + smb_core::nt_status::NTStatus::InvalidDeviceRequest, + )) + } } impl From for Box { diff --git a/smb/src/server/share/mod.rs b/smb/src/server/share/mod.rs index 17d7af4..70a1573 100644 --- a/smb/src/server/share/mod.rs +++ b/smb/src/server/share/mod.rs @@ -24,6 +24,7 @@ pub trait ResourceHandle: Send + Sync { fn is_directory(&self) -> bool; fn path(&self) -> &str; fn metadata(&self) -> SMBResult; + fn read_data(&mut self, offset: u64, length: u32) -> SMBResult>; } pub struct SMBFileMetadata { @@ -55,6 +56,10 @@ impl ResourceHandle for Box { fn metadata(&self) -> SMBResult { H::metadata(self) } + + fn read_data(&mut self, offset: u64, length: u32) -> SMBResult> { + H::read_data(self, offset, length) + } } pub trait SharedResource: Send + Sync { diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 992e352..079f8c3 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -7,10 +7,16 @@ use tokio::sync::RwLock; use smb_core::SMBResult; use smb_core::error::SMBError; use smb_core::logging::{debug, trace}; +use smb_core::nt_status::NTStatus; use crate::protocol::body::SMBBody; +use crate::protocol::body::close::{SMBCloseRequest, SMBCloseResponse}; +use crate::protocol::body::create::file_id::SMBFileId; use crate::protocol::body::create::{SMBCreateRequest, SMBCreateResponse}; use crate::protocol::body::filetime::FileTime; +use crate::protocol::body::query_info::info_type::SMBInfoType; +use crate::protocol::body::query_info::{SMBQueryInfoRequest, SMBQueryInfoResponse}; +use crate::protocol::body::read::{SMBReadRequest, SMBReadResponse}; use crate::protocol::body::tree_connect::access_mask::SMBAccessMask; use crate::protocol::header::SMBSyncHeader; use crate::protocol::message::SMBMessage; @@ -54,6 +60,73 @@ impl SMBTreeConnect { } } +impl SMBTreeConnect { + fn get_session(&self) -> SMBResult>> { + self.session + .upgrade() + .ok_or(SMBError::server_error("No Session Found")) + } + + async fn find_open(&self, file_id: &SMBFileId) -> SMBResult>> { + let session = self.get_session()?; + let session_rd = session.read().await; + session_rd + .open_table() + .get(&file_id.volatile) + .cloned() + .ok_or(SMBError::response_error(NTStatus::FileClosed)) + } + + /// Build a FILE_BASIC_INFORMATION buffer (MS-FSCC 2.4.7) from open metadata. + fn build_file_basic_info(open: &S::Open) -> SMBResult> { + let metadata = open.file_metadata()?; + let mut buf = Vec::with_capacity(40); + buf.extend_from_slice(&metadata.creation_time.as_bytes()); // CreationTime (8) + buf.extend_from_slice(&metadata.last_access_time.as_bytes()); // LastAccessTime (8) + buf.extend_from_slice(&metadata.last_write_time.as_bytes()); // LastWriteTime (8) + buf.extend_from_slice(&metadata.last_modification_time.as_bytes()); // ChangeTime (8) + buf.extend_from_slice(&(open.file_attributes().bits() as u32).to_le_bytes()); // FileAttributes (4) + buf.extend_from_slice(&[0u8; 4]); // Reserved (4) + Ok(buf) + } + + /// Build a FILE_STANDARD_INFORMATION buffer (MS-FSCC 2.4.41) from open metadata. + fn build_file_standard_info(open: &S::Open) -> SMBResult> { + let metadata = open.file_metadata()?; + let mut buf = Vec::with_capacity(24); + buf.extend_from_slice(&metadata.allocated_size.to_le_bytes()); // AllocationSize (8) + buf.extend_from_slice(&metadata.actual_size.to_le_bytes()); // EndOfFile (8) + buf.extend_from_slice(&1u32.to_le_bytes()); // NumberOfLinks (4) + buf.push(0); // DeletePending (1) + buf.push( + if open.file_attributes().contains( + crate::protocol::body::create::file_attributes::SMBFileAttributes::DIRECTORY, + ) { + 1 + } else { + 0 + }, + ); // Directory (1) + buf.extend_from_slice(&[0u8; 2]); // Reserved (2) + Ok(buf) + } + + /// Build a FILE_NETWORK_OPEN_INFORMATION buffer (MS-FSCC 2.4.29) from open metadata. + fn build_file_network_open_info(open: &S::Open) -> SMBResult> { + let metadata = open.file_metadata()?; + let mut buf = Vec::with_capacity(56); + buf.extend_from_slice(&metadata.creation_time.as_bytes()); // CreationTime (8) + buf.extend_from_slice(&metadata.last_access_time.as_bytes()); // LastAccessTime (8) + buf.extend_from_slice(&metadata.last_write_time.as_bytes()); // LastWriteTime (8) + buf.extend_from_slice(&metadata.last_modification_time.as_bytes()); // ChangeTime (8) + buf.extend_from_slice(&metadata.allocated_size.to_le_bytes()); // AllocationSize (8) + buf.extend_from_slice(&metadata.actual_size.to_le_bytes()); // EndOfFile (8) + buf.extend_from_slice(&(open.file_attributes().bits() as u32).to_le_bytes()); // FileAttributes (4) + buf.extend_from_slice(&[0u8; 4]); // Reserved (4) + Ok(buf) + } +} + impl SMBLockedMessageHandlerBase for Arc> { type Inner = Arc>; @@ -71,15 +144,13 @@ impl SMBLockedMessageHandlerBase for Arc> { let open_raw = Open::init(handle, message); let response = SMBBody::CreateResponse(SMBCreateResponse::for_open::(&open_raw)?); let open = Arc::new(RwLock::new(open_raw)); - let session = self - .session - .upgrade() - .ok_or(SMBError::server_error("No Session Found"))?; - session.write().await.add_open(open.clone()).await; + let session = self.get_session()?; + // Register with server first (outermost), then session (inner) let server = session.upper().await?.upper().await?; { server.write().await.add_open(open.clone()).await; } + session.write().await.add_open(open.clone()).await; { let file_id = open.read().await.file_id(); session.write().await.set_previous_file_id(file_id); @@ -96,6 +167,123 @@ impl SMBLockedMessageHandlerBase for Arc> { ); Ok(SMBHandlerState::Finished(SMBMessage::new(header, response))) } + + async fn handle_close( + &mut self, + header: &SMBSyncHeader, + message: &SMBCloseRequest, + ) -> SMBResult> { + debug!(file_id = ?message.file_id(), "handling close request"); + + // Phase 1: Read open data (session_rd → open_rd, outer before inner) + let session = self.get_session()?; + let open = { + let session_rd = session.read().await; + session_rd + .open_table() + .get(&message.file_id().volatile) + .cloned() + .ok_or(SMBError::response_error(NTStatus::FileClosed))? + }; + let (response, file_id) = { + let open_rd = open.read().await; + let response = if message + .flags() + .contains(crate::protocol::body::close::flags::SMBCloseFlags::POSTQUERY_ATTRIB) + { + let metadata = open_rd.file_metadata()?; + SMBCloseResponse::from_metadata(&metadata, open_rd.file_attributes()) + } else { + SMBCloseResponse::empty() + }; + (response, open_rd.file_id()) + }; + + // Phase 2: Cleanup — acquire locks outer to inner (server_wr, then session_wr) + // Server write first (outermost) + if let Ok(conn) = session.upper().await { + if let Ok(server) = conn.upper().await { + server.write().await.remove_open(file_id.volatile as u32); + } + } + // Session write second (inner relative to server) + { + let mut session_wr = session.write().await; + session_wr.open_table_mut().remove(&file_id.volatile); + } + + debug!(file_id = ?file_id, "close completed"); + let header = header.create_response_header(0, header.session_id, header.tree_id); + Ok(SMBHandlerState::Finished(SMBMessage::new( + header, + SMBBody::CloseResponse(response), + ))) + } + + async fn handle_read( + &mut self, + header: &SMBSyncHeader, + message: &SMBReadRequest, + ) -> SMBResult> { + debug!(file_id = ?message.file_id(), offset = message.read_offset(), length = message.read_length(), "handling read request"); + let open = self.find_open(message.file_id()).await?; + let mut open_wr = open.write().await; + let data = open_wr.read_data(message.read_offset(), message.read_length())?; + drop(open_wr); + + if data.len() < message.minimum_count() as usize { + return Err(SMBError::response_error(NTStatus::EndOfFile)); + } + + debug!(bytes_read = data.len(), "read completed"); + trace!(data_len = data.len(), "read response data"); + let response = SMBReadResponse::new(data, 0); + let header = header.create_response_header(0, header.session_id, header.tree_id); + Ok(SMBHandlerState::Finished(SMBMessage::new( + header, + SMBBody::ReadResponse(response), + ))) + } + + async fn handle_query_info( + &mut self, + header: &SMBSyncHeader, + message: &SMBQueryInfoRequest, + ) -> SMBResult> { + debug!(file_id = ?message.file_id(), info_type = ?message.info_type(), class = message.file_info_class(), "handling query_info request"); + let open = self.find_open(message.file_id()).await?; + let open_rd = open.read().await; + + let data = match message.info_type() { + SMBInfoType::File => { + // MS-FSCC file information classes + match message.file_info_class() { + 4 => SMBTreeConnect::::build_file_basic_info(&*open_rd)?, // FileBasicInformation + 5 => SMBTreeConnect::::build_file_standard_info(&*open_rd)?, // FileStandardInformation + 34 => SMBTreeConnect::::build_file_network_open_info(&*open_rd)?, // FileNetworkOpenInformation + _ => { + debug!( + class = message.file_info_class(), + "unsupported file info class" + ); + return Err(SMBError::response_error(NTStatus::InvalidInfoClass)); + } + } + } + _ => { + debug!(info_type = ?message.info_type(), "unsupported info type"); + return Err(SMBError::response_error(NTStatus::InvalidInfoClass)); + } + }; + + debug!(data_len = data.len(), "query_info completed"); + let response = SMBQueryInfoResponse::new(data); + let header = header.create_response_header(0, header.session_id, header.tree_id); + Ok(SMBHandlerState::Finished(SMBMessage::new( + header, + SMBBody::QueryInfoResponse(response), + ))) + } } impl SMBLockedMessageHandler for Arc> {} diff --git a/smb/tests/smbclient.rs b/smb/tests/smbclient.rs index 0f70bfb..17b825e 100644 --- a/smb/tests/smbclient.rs +++ b/smb/tests/smbclient.rs @@ -274,6 +274,206 @@ fn tree_connect_nonexistent_share() { server.kill().ok(); } +// --------------------------------------------------------------------------- +// File Read Tests +// --------------------------------------------------------------------------- + +/// Verify that smbclient can read a file from the share. +/// +/// Expected: The server handles Create, Read, QueryInfo, and Close +/// without crashing. smbclient should be able to retrieve file contents. +#[test] +#[ignore] +fn file_read_does_not_crash_server() { + use std::io::Write; + + let port = free_port(); + + // Create a temp file in the server's working directory for the share to serve + let tmp_dir = std::env::temp_dir().join(format!("smb_test_{}", port)); + std::fs::create_dir_all(&tmp_dir).expect("Failed to create temp dir"); + let test_file = tmp_dir.join("testfile.txt"); + { + let mut f = std::fs::File::create(&test_file).expect("Failed to create test file"); + f.write_all(b"hello from smb server") + .expect("Failed to write test file"); + } + + // Start server with the temp dir as working directory + let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); + let mut server = std::process::Command::new(server_bin) + .env("SMB_PORT", port.to_string()) + .current_dir(&tmp_dir) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("Failed to spawn SMB server binary"); + + // Wait for server to start + let addr = format!("127.0.0.1:{}", port); + for _ in 0..50 { + if std::net::TcpStream::connect(&addr).is_ok() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + + let download_path = tmp_dir.join("downloaded.txt"); + let port_str = port.to_string(); + let download_str = download_path.to_str().unwrap().to_string(); + let get_cmd = format!("get testfile.txt {}", download_str); + let (success, _stdout, stderr) = run_smbclient(&[ + &format!("//127.0.0.1/test"), + "-p", + &port_str, + "-U", + "tejasmehta%password", + "-m", + "SMB2", + "-c", + &get_cmd, + ]); + + // Server should not crash + std::thread::sleep(Duration::from_millis(200)); + let status = server.try_wait().expect("Failed to check server status"); + assert!( + status.is_none(), + "Server should still be running after file read. stderr: {}", + stderr + ); + + // Verify the file was downloaded and contents match + assert!(success, "smbclient get should succeed. stderr: {}", stderr); + let downloaded = std::fs::read(&download_path).expect("Downloaded file should exist"); + assert_eq!( + downloaded, b"hello from smb server", + "Downloaded file contents should match the original" + ); + + server.kill().ok(); + let _ = std::fs::remove_dir_all(&tmp_dir); +} + +/// Verify that smbclient can list files (which triggers QueryInfo). +#[test] +#[ignore] +fn directory_listing_does_not_crash_server() { + use std::io::Write; + + let port = free_port(); + + let tmp_dir = std::env::temp_dir().join(format!("smb_test_ls_{}", port)); + std::fs::create_dir_all(&tmp_dir).expect("Failed to create temp dir"); + let test_file = tmp_dir.join("listing_test.txt"); + { + let mut f = std::fs::File::create(&test_file).expect("Failed to create test file"); + f.write_all(b"test content") + .expect("Failed to write test file"); + } + + let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); + let mut server = std::process::Command::new(server_bin) + .env("SMB_PORT", port.to_string()) + .current_dir(&tmp_dir) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("Failed to spawn SMB server binary"); + + let addr = format!("127.0.0.1:{}", port); + for _ in 0..50 { + if std::net::TcpStream::connect(&addr).is_ok() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + + let port_str = port.to_string(); + let (_success, _stdout, stderr) = run_smbclient(&[ + &format!("//127.0.0.1/test"), + "-p", + &port_str, + "-U", + "tejasmehta%password", + "-m", + "SMB2", + "-c", + "ls", + ]); + + // Server should not crash + std::thread::sleep(Duration::from_millis(200)); + let status = server.try_wait().expect("Failed to check server status"); + assert!( + status.is_none(), + "Server should still be running after directory listing. stderr: {}", + stderr + ); + + server.kill().ok(); + let _ = std::fs::remove_dir_all(&tmp_dir); +} + +/// Verify that reading a nonexistent file returns an error without crashing. +#[test] +#[ignore] +fn read_nonexistent_file_returns_error() { + let port = free_port(); + + let tmp_dir = std::env::temp_dir().join(format!("smb_test_nofile_{}", port)); + std::fs::create_dir_all(&tmp_dir).expect("Failed to create temp dir"); + + let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); + let mut server = std::process::Command::new(server_bin) + .env("SMB_PORT", port.to_string()) + .current_dir(&tmp_dir) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("Failed to spawn SMB server binary"); + + let addr = format!("127.0.0.1:{}", port); + for _ in 0..50 { + if std::net::TcpStream::connect(&addr).is_ok() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + + let port_str = port.to_string(); + let (success, _stdout, stderr) = run_smbclient(&[ + &format!("//127.0.0.1/test"), + "-p", + &port_str, + "-U", + "tejasmehta%password", + "-m", + "SMB2", + "-c", + "get nonexistent_file.txt /dev/null", + ]); + + // Should fail (file doesn't exist) + assert!( + !success || stderr.contains("NT_STATUS_"), + "Reading nonexistent file should fail. stderr: {}", + stderr + ); + + // Server should not crash + std::thread::sleep(Duration::from_millis(200)); + let status = server.try_wait().expect("Failed to check server status"); + assert!( + status.is_none(), + "Server should still be running after failed file read. stderr: {}", + stderr + ); + + server.kill().ok(); + let _ = std::fs::remove_dir_all(&tmp_dir); +} + // --------------------------------------------------------------------------- // Echo Tests // --------------------------------------------------------------------------- From f2fe29671f11a1629fe47110531c00942b45fd46 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Mon, 9 Feb 2026 23:45:26 -0500 Subject: [PATCH 02/15] fix: resolve file read integration test failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix SMBCreateRequest NameOffset subtract (68→64): offset is relative to SMB2 header start (64 bytes), not 68. This caused 4-byte misalignment producing leading NUL chars and filename truncation. - Fix SMBInfoType enum values to match MS-SMB2 spec: File=1, Filesystem=2, Security=3, Quota=4 (was incorrectly starting at 0). - Fix SMBQueryInfoResponse StructureSize: 9 per MS-SMB2 2.2.38, not 17. - Add FileAllInformation (class 18) handler in QueryInfo dispatch per MS-FSCC 2.4.2 (composite of Basic+Standard+Internal+EA+Access+Position+ Mode+Alignment+Name). - Fix tree_id in TreeConnect response header (use dynamic ID, not hardcoded 1). - Add SMB_SHARE_PATH env var support in main.rs for configurable share root. - Sanitize file paths in SMBFileSystemShare::handle_create: strip trailing NUL terminators and convert backslashes to forward slashes. - Improve integration test diagnostics: capture smbclient stdout/stderr, verify file contents in file_read test. All 66 unit tests and 10 integration tests pass. --- smb/src/main.rs | 7 ++- smb/src/protocol/body/create/mod.rs | 2 +- smb/src/protocol/body/query_info/info_type.rs | 8 ++-- smb/src/protocol/body/query_info/mod.rs | 6 +-- smb/src/server/session.rs | 2 +- smb/src/server/share/file_system.rs | 5 ++- smb/src/server/tree_connect.rs | 44 +++++++++++++++++++ smb/tests/smbclient.rs | 26 ++++++----- 8 files changed, 79 insertions(+), 21 deletions(-) diff --git a/smb/src/main.rs b/smb/src/main.rs index 0a2b652..e44f41a 100644 --- a/smb/src/main.rs +++ b/smb/src/main.rs @@ -47,7 +47,12 @@ async fn main() -> SMBResult<()> { .unencrypted_access(true) .require_message_signing(false) .encrypt_data(false) - .add_fs_share("test".into(), "".into(), file_allowed, get_file_perms) + .add_fs_share( + "test".into(), + std::env::var("SMB_SHARE_PATH").unwrap_or_default(), + file_allowed, + get_file_perms, + ) .add_ipc_share() .auth_provider(NTLMAuthProvider::new( vec![ diff --git a/smb/src/protocol/body/create/mod.rs b/smb/src/protocol/body/create/mod.rs index a7c79ef..d868a70 100644 --- a/smb/src/protocol/body/create/mod.rs +++ b/smb/src/protocol/body/create/mod.rs @@ -65,7 +65,7 @@ pub struct SMBCreateRequest { create_options: SMBCreateOptions, #[smb_string( order = 0, - start(inner(start = 44, num_type = "u16", subtract = 68)), + start(inner(start = 44, num_type = "u16", subtract = 64)), length(inner(start = 46, num_type = "u16")), underlying = "u16" )] diff --git a/smb/src/protocol/body/query_info/info_type.rs b/smb/src/protocol/body/query_info/info_type.rs index a23d556..00f8ffc 100644 --- a/smb/src/protocol/body/query_info/info_type.rs +++ b/smb/src/protocol/body/query_info/info_type.rs @@ -20,8 +20,8 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; Deserialize, )] pub enum SMBInfoType { - File, - Filesystem, - Security, - Quota, + File = 1, + Filesystem = 2, + Security = 3, + Quota = 4, } diff --git a/smb/src/protocol/body/query_info/mod.rs b/smb/src/protocol/body/query_info/mod.rs index 7b10436..7b8cbd8 100644 --- a/smb/src/protocol/body/query_info/mod.rs +++ b/smb/src/protocol/body/query_info/mod.rs @@ -60,7 +60,7 @@ impl SMBQueryInfoRequest { #[derive( Debug, PartialEq, Eq, SMBByteSize, SMBToBytes, SMBFromBytes, Serialize, Deserialize, Clone, )] -#[smb_byte_tag(value = 17)] +#[smb_byte_tag(value = 9)] pub struct SMBQueryInfoResponse { #[smb_skip(start = 2, length = 6)] reserved: PhantomData>, @@ -117,8 +117,8 @@ mod tests { let mut buf = Vec::new(); // struct_size (u16) = 41 buf.extend_from_slice(&41u16.to_le_bytes()); - // info_type (u8) = 0 (File) - buf.push(0); + // info_type (u8) = 1 (File) per MS-SMB2 + buf.push(1); // file_info_class (u8) = 4 (FileBasicInformation) buf.push(4); // output_buffer_length (u32) = 4096 diff --git a/smb/src/server/session.rs b/smb/src/server/session.rs index 5b7213a..889b0b6 100644 --- a/smb/src/server/session.rs +++ b/smb/src/server/session.rs @@ -290,7 +290,7 @@ impl>> SMBLockedMessageHandlerBase share.clone(), response.access_mask().clone(), ); - let header = SMBSyncHeader::create_response_header(header, 0, self_rd.id(), 1); + let header = SMBSyncHeader::create_response_header(&header, 0, self_rd.id(), tree_id); drop(self_rd); let mut self_wr = self.write().await; self_wr diff --git a/smb/src/server/share/file_system.rs b/smb/src/server/share/file_system.rs index 5563843..50c2de1 100644 --- a/smb/src/server/share/file_system.rs +++ b/smb/src/server/share/file_system.rs @@ -197,7 +197,10 @@ impl< disposition: SMBCreateDisposition, directory: bool, ) -> SMBResult { - let path = format!("{}/{}", self.local_path, path); + // Sanitize: strip NUL terminators from UTF-16LE wire encoding, + // convert Windows backslashes to forward slashes + let sanitized = path.trim_end_matches('\0').replace('\\', "/"); + let path = format!("{}/{}", self.local_path, sanitized); let resource = match directory { true => SMBFileSystemResourceHandle::directory(&path), false => SMBFileSystemResourceHandle::file(&path, disposition), diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 079f8c3..6f38d97 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -125,6 +125,49 @@ impl SMBTreeConnect { buf.extend_from_slice(&[0u8; 4]); // Reserved (4) Ok(buf) } + + /// Build a FILE_ALL_INFORMATION buffer (MS-FSCC 2.4.2) from open metadata. + /// Composite of Basic + Standard + Internal + EA + Access + Position + Mode + Alignment + Name. + fn build_file_all_info(open: &S::Open) -> SMBResult> { + let metadata = open.file_metadata()?; + let is_dir = open + .file_attributes() + .contains(crate::protocol::body::create::file_attributes::SMBFileAttributes::DIRECTORY); + let mut buf = Vec::with_capacity(104); + // FileBasicInformation (40 bytes) + buf.extend_from_slice(&metadata.creation_time.as_bytes()); + buf.extend_from_slice(&metadata.last_access_time.as_bytes()); + buf.extend_from_slice(&metadata.last_write_time.as_bytes()); + buf.extend_from_slice(&metadata.last_modification_time.as_bytes()); + buf.extend_from_slice(&(open.file_attributes().bits() as u32).to_le_bytes()); + buf.extend_from_slice(&[0u8; 4]); // Reserved + // FileStandardInformation (24 bytes) + buf.extend_from_slice(&metadata.allocated_size.to_le_bytes()); + buf.extend_from_slice(&metadata.actual_size.to_le_bytes()); + buf.extend_from_slice(&1u32.to_le_bytes()); // NumberOfLinks + buf.push(0); // DeletePending + buf.push(if is_dir { 1 } else { 0 }); // Directory + buf.extend_from_slice(&[0u8; 2]); // Reserved + // FileInternalInformation (8 bytes) + buf.extend_from_slice(&0u64.to_le_bytes()); // IndexNumber + // FileEaInformation (4 bytes) + buf.extend_from_slice(&0u32.to_le_bytes()); // EaSize + // FileAccessInformation (4 bytes) + buf.extend_from_slice(&0x001f01ffu32.to_le_bytes()); // AccessFlags (GENERIC_ALL) + // FilePositionInformation (8 bytes) + buf.extend_from_slice(&0u64.to_le_bytes()); // CurrentByteOffset + // FileModeInformation (4 bytes) + buf.extend_from_slice(&0u32.to_le_bytes()); // Mode + // FileAlignmentInformation (4 bytes) + buf.extend_from_slice(&0u32.to_le_bytes()); // AlignmentRequirement + // FileNameInformation (variable) + let name = open.file_name(); + let name_utf16: Vec = name.encode_utf16().collect(); + let name_bytes: Vec = name_utf16.iter().flat_map(|c| c.to_le_bytes()).collect(); + buf.extend_from_slice(&(name_bytes.len() as u32).to_le_bytes()); // FileNameLength + buf.extend_from_slice(&name_bytes); // FileName + Ok(buf) + } } impl SMBLockedMessageHandlerBase for Arc> { @@ -260,6 +303,7 @@ impl SMBLockedMessageHandlerBase for Arc> { match message.file_info_class() { 4 => SMBTreeConnect::::build_file_basic_info(&*open_rd)?, // FileBasicInformation 5 => SMBTreeConnect::::build_file_standard_info(&*open_rd)?, // FileStandardInformation + 18 => SMBTreeConnect::::build_file_all_info(&*open_rd)?, // FileAllInformation 34 => SMBTreeConnect::::build_file_network_open_info(&*open_rd)?, // FileNetworkOpenInformation _ => { debug!( diff --git a/smb/tests/smbclient.rs b/smb/tests/smbclient.rs index 17b825e..a936aa3 100644 --- a/smb/tests/smbclient.rs +++ b/smb/tests/smbclient.rs @@ -299,11 +299,11 @@ fn file_read_does_not_crash_server() { .expect("Failed to write test file"); } - // Start server with the temp dir as working directory + // Start server with the share path pointing to our temp dir let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); let mut server = std::process::Command::new(server_bin) .env("SMB_PORT", port.to_string()) - .current_dir(&tmp_dir) + .env("SMB_SHARE_PATH", tmp_dir.to_str().unwrap()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() @@ -322,7 +322,7 @@ fn file_read_does_not_crash_server() { let port_str = port.to_string(); let download_str = download_path.to_str().unwrap().to_string(); let get_cmd = format!("get testfile.txt {}", download_str); - let (success, _stdout, stderr) = run_smbclient(&[ + let (success, stdout, stderr) = run_smbclient(&[ &format!("//127.0.0.1/test"), "-p", &port_str, @@ -339,12 +339,17 @@ fn file_read_does_not_crash_server() { let status = server.try_wait().expect("Failed to check server status"); assert!( status.is_none(), - "Server should still be running after file read. stderr: {}", + "Server should still be running after file read. stdout: {} stderr: {}", + stdout, stderr ); // Verify the file was downloaded and contents match - assert!(success, "smbclient get should succeed. stderr: {}", stderr); + assert!( + success, + "smbclient get should succeed. stdout: {} stderr: {}", + stdout, stderr + ); let downloaded = std::fs::read(&download_path).expect("Downloaded file should exist"); assert_eq!( downloaded, b"hello from smb server", @@ -375,7 +380,7 @@ fn directory_listing_does_not_crash_server() { let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); let mut server = std::process::Command::new(server_bin) .env("SMB_PORT", port.to_string()) - .current_dir(&tmp_dir) + .env("SMB_SHARE_PATH", tmp_dir.to_str().unwrap()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() @@ -427,7 +432,7 @@ fn read_nonexistent_file_returns_error() { let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); let mut server = std::process::Command::new(server_bin) .env("SMB_PORT", port.to_string()) - .current_dir(&tmp_dir) + .env("SMB_SHARE_PATH", tmp_dir.to_str().unwrap()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() @@ -442,7 +447,7 @@ fn read_nonexistent_file_returns_error() { } let port_str = port.to_string(); - let (success, _stdout, stderr) = run_smbclient(&[ + let (success, stdout, stderr) = run_smbclient(&[ &format!("//127.0.0.1/test"), "-p", &port_str, @@ -456,8 +461,9 @@ fn read_nonexistent_file_returns_error() { // Should fail (file doesn't exist) assert!( - !success || stderr.contains("NT_STATUS_"), - "Reading nonexistent file should fail. stderr: {}", + !success || stdout.contains("NT_STATUS_") || stderr.contains("NT_STATUS_"), + "Reading nonexistent file should fail. stdout: {} stderr: {}", + stdout, stderr ); From 09ea9220a92450f1c168f24504352bbdca60c3e2 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Tue, 10 Feb 2026 00:01:00 -0500 Subject: [PATCH 03/15] refactor: replace raw Vec info buffers with typed MS-FSCC structs Introduce protocol::body::file_info module with proper typed structs for MS-FSCC file information classes: - FileBasicInformation (2.4.7) - FileStandardInformation (2.4.41) - FileInternalInformation (2.4.20) - FileEaInformation (2.4.12) - FileAccessInformation (2.4.1) - FilePositionInformation (2.4.35) - FileModeInformation (2.4.26) - FileAlignmentInformation (2.4.3) - FileNameInformation (2.4.28) - FileNetworkOpenInformation (2.4.29) - FileAllInformation (2.4.2) All types use SMBFromBytes/SMBToBytes/SMBByteSize derive macros for automatic serialization, replacing manual Vec byte-pushing in tree_connect.rs build_file_* methods. Includes 14 unit tests (round-trip serialization + size assertions). All 80 unit tests and 10 integration tests pass. --- smb/src/protocol/body/file_info/mod.rs | 416 +++++++++++++++++++++++++ smb/src/protocol/body/mod.rs | 1 + smb/src/server/tree_connect.rs | 150 ++++----- 3 files changed, 482 insertions(+), 85 deletions(-) create mode 100644 smb/src/protocol/body/file_info/mod.rs diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs new file mode 100644 index 0000000..cd68ddd --- /dev/null +++ b/smb/src/protocol/body/file_info/mod.rs @@ -0,0 +1,416 @@ +//! MS-FSCC File Information Classes +//! +//! Typed representations of the file information structures defined in +//! [MS-FSCC] sections 2.4.x, used in QueryInfo / SetInfo responses. + +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +use crate::protocol::body::create::file_attributes::SMBFileAttributes; +use crate::protocol::body::filetime::FileTime; + +/// FILE_BASIC_INFORMATION (MS-FSCC 2.4.7) — 40 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileBasicInformation { + #[smb_direct(start(fixed = 0))] + pub creation_time: FileTime, + #[smb_direct(start(fixed = 8))] + pub last_access_time: FileTime, + #[smb_direct(start(fixed = 16))] + pub last_write_time: FileTime, + #[smb_direct(start(fixed = 24))] + pub change_time: FileTime, + #[smb_direct(start(fixed = 32))] + pub file_attributes: SMBFileAttributes, + #[smb_direct(start(fixed = 36))] + pub reserved: u32, +} + +/// FILE_STANDARD_INFORMATION (MS-FSCC 2.4.41) — 24 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileStandardInformation { + #[smb_direct(start(fixed = 0))] + pub allocation_size: u64, + #[smb_direct(start(fixed = 8))] + pub end_of_file: u64, + #[smb_direct(start(fixed = 16))] + pub number_of_links: u32, + #[smb_direct(start(fixed = 20))] + pub delete_pending: u8, + #[smb_direct(start(fixed = 21))] + pub directory: u8, + #[smb_direct(start(fixed = 22))] + pub reserved: u16, +} + +/// FILE_INTERNAL_INFORMATION (MS-FSCC 2.4.20) — 8 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileInternalInformation { + #[smb_direct(start(fixed = 0))] + pub index_number: u64, +} + +/// FILE_EA_INFORMATION (MS-FSCC 2.4.12) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileEaInformation { + #[smb_direct(start(fixed = 0))] + pub ea_size: u32, +} + +/// FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileAccessInformation { + #[smb_direct(start(fixed = 0))] + pub access_flags: u32, +} + +/// FILE_POSITION_INFORMATION (MS-FSCC 2.4.35) — 8 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FilePositionInformation { + #[smb_direct(start(fixed = 0))] + pub current_byte_offset: u64, +} + +/// FILE_MODE_INFORMATION (MS-FSCC 2.4.26) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileModeInformation { + #[smb_direct(start(fixed = 0))] + pub mode: u32, +} + +/// FILE_ALIGNMENT_INFORMATION (MS-FSCC 2.4.3) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileAlignmentInformation { + #[smb_direct(start(fixed = 0))] + pub alignment_requirement: u32, +} + +/// FILE_NAME_INFORMATION (MS-FSCC 2.4.28) — variable length +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileNameInformation { + #[smb_direct(start(fixed = 0))] + pub file_name_length: u32, + #[smb_string( + order = 0, + start(fixed = 4), + length(inner(start = 0, num_type = "u32")), + underlying = "u16" + )] + pub file_name: String, +} + +/// FILE_NETWORK_OPEN_INFORMATION (MS-FSCC 2.4.29) — 56 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileNetworkOpenInformation { + #[smb_direct(start(fixed = 0))] + pub creation_time: FileTime, + #[smb_direct(start(fixed = 8))] + pub last_access_time: FileTime, + #[smb_direct(start(fixed = 16))] + pub last_write_time: FileTime, + #[smb_direct(start(fixed = 24))] + pub change_time: FileTime, + #[smb_direct(start(fixed = 32))] + pub allocation_size: u64, + #[smb_direct(start(fixed = 40))] + pub end_of_file: u64, + #[smb_direct(start(fixed = 48))] + pub file_attributes: SMBFileAttributes, + #[smb_direct(start(fixed = 52))] + pub reserved: u32, +} + +/// FILE_ALL_INFORMATION (MS-FSCC 2.4.2) — composite structure +/// +/// This is a concatenation of the sub-structures above. +/// We serialize it by concatenating each sub-struct's bytes rather than +/// using the derive macro, because the derive macro doesn't support +/// nested struct composition at variable offsets. +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct FileAllInformation { + pub basic: FileBasicInformation, + pub standard: FileStandardInformation, + pub internal: FileInternalInformation, + pub ea: FileEaInformation, + pub access: FileAccessInformation, + pub position: FilePositionInformation, + pub mode: FileModeInformation, + pub alignment: FileAlignmentInformation, + pub name: FileNameInformation, +} + +impl FileAllInformation { + pub fn to_bytes(&self) -> Vec { + use smb_core::SMBToBytes; + let mut buf = Vec::with_capacity(104); + buf.extend_from_slice(&self.basic.smb_to_bytes()); + buf.extend_from_slice(&self.standard.smb_to_bytes()); + buf.extend_from_slice(&self.internal.smb_to_bytes()); + buf.extend_from_slice(&self.ea.smb_to_bytes()); + buf.extend_from_slice(&self.access.smb_to_bytes()); + buf.extend_from_slice(&self.position.smb_to_bytes()); + buf.extend_from_slice(&self.mode.smb_to_bytes()); + buf.extend_from_slice(&self.alignment.smb_to_bytes()); + buf.extend_from_slice(&self.name.smb_to_bytes()); + buf + } +} + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; + + #[test] + fn file_basic_information_size_is_40() { + let info = FileBasicInformation { + creation_time: FileTime::zero(), + last_access_time: FileTime::zero(), + last_write_time: FileTime::zero(), + change_time: FileTime::zero(), + file_attributes: SMBFileAttributes::NORMAL, + reserved: 0, + }; + assert_eq!(info.smb_byte_size(), 40); + } + + #[test] + fn file_basic_information_round_trip() { + let info = FileBasicInformation { + creation_time: FileTime::now(), + last_access_time: FileTime::now(), + last_write_time: FileTime::now(), + change_time: FileTime::now(), + file_attributes: SMBFileAttributes::ARCHIVE | SMBFileAttributes::READONLY, + reserved: 0, + }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 40); + let (_, parsed) = FileBasicInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_standard_information_size_is_24() { + let info = FileStandardInformation { + allocation_size: 4096, + end_of_file: 1024, + number_of_links: 1, + delete_pending: 0, + directory: 0, + reserved: 0, + }; + assert_eq!(info.smb_byte_size(), 24); + } + + #[test] + fn file_standard_information_round_trip() { + let info = FileStandardInformation { + allocation_size: 8192, + end_of_file: 2048, + number_of_links: 3, + delete_pending: 1, + directory: 0, + reserved: 0, + }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 24); + let (_, parsed) = FileStandardInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_internal_information_round_trip() { + let info = FileInternalInformation { index_number: 42 }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 8); + let (_, parsed) = FileInternalInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_ea_information_round_trip() { + let info = FileEaInformation { ea_size: 0 }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 4); + let (_, parsed) = FileEaInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_access_information_round_trip() { + let info = FileAccessInformation { + access_flags: 0x001f01ff, + }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 4); + let (_, parsed) = FileAccessInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_position_information_round_trip() { + let info = FilePositionInformation { + current_byte_offset: 512, + }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 8); + let (_, parsed) = FilePositionInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_mode_information_round_trip() { + let info = FileModeInformation { mode: 0 }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 4); + let (_, parsed) = FileModeInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_alignment_information_round_trip() { + let info = FileAlignmentInformation { + alignment_requirement: 0, + }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 4); + let (_, parsed) = FileAlignmentInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_network_open_information_size_is_56() { + let info = FileNetworkOpenInformation { + creation_time: FileTime::zero(), + last_access_time: FileTime::zero(), + last_write_time: FileTime::zero(), + change_time: FileTime::zero(), + allocation_size: 0, + end_of_file: 0, + file_attributes: SMBFileAttributes::NORMAL, + reserved: 0, + }; + assert_eq!(info.smb_byte_size(), 56); + } + + #[test] + fn file_network_open_information_round_trip() { + let info = FileNetworkOpenInformation { + creation_time: FileTime::now(), + last_access_time: FileTime::now(), + last_write_time: FileTime::now(), + change_time: FileTime::now(), + allocation_size: 4096, + end_of_file: 1024, + file_attributes: SMBFileAttributes::ARCHIVE, + reserved: 0, + }; + let bytes = info.smb_to_bytes(); + assert_eq!(bytes.len(), 56); + let (_, parsed) = FileNetworkOpenInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn file_all_information_contains_all_sub_structs() { + let all = FileAllInformation { + basic: FileBasicInformation { + creation_time: FileTime::zero(), + last_access_time: FileTime::zero(), + last_write_time: FileTime::zero(), + change_time: FileTime::zero(), + file_attributes: SMBFileAttributes::NORMAL, + reserved: 0, + }, + standard: FileStandardInformation { + allocation_size: 4096, + end_of_file: 21, + number_of_links: 1, + delete_pending: 0, + directory: 0, + reserved: 0, + }, + internal: FileInternalInformation { index_number: 0 }, + ea: FileEaInformation { ea_size: 0 }, + access: FileAccessInformation { + access_flags: 0x001f01ff, + }, + position: FilePositionInformation { + current_byte_offset: 0, + }, + mode: FileModeInformation { mode: 0 }, + alignment: FileAlignmentInformation { + alignment_requirement: 0, + }, + name: FileNameInformation { + file_name_length: 24, + file_name: "testfile.txt".into(), + }, + }; + let bytes = all.to_bytes(); + // 40 + 24 + 8 + 4 + 4 + 8 + 4 + 4 + (4 + 24) = 124 + assert_eq!(bytes.len(), 124); + } + + #[test] + fn file_all_information_basic_segment_matches_standalone() { + let basic = FileBasicInformation { + creation_time: FileTime::now(), + last_access_time: FileTime::now(), + last_write_time: FileTime::now(), + change_time: FileTime::now(), + file_attributes: SMBFileAttributes::ARCHIVE, + reserved: 0, + }; + let all = FileAllInformation { + basic: basic.clone(), + standard: FileStandardInformation { + allocation_size: 0, + end_of_file: 0, + number_of_links: 1, + delete_pending: 0, + directory: 0, + reserved: 0, + }, + internal: FileInternalInformation { index_number: 0 }, + ea: FileEaInformation { ea_size: 0 }, + access: FileAccessInformation { access_flags: 0 }, + position: FilePositionInformation { + current_byte_offset: 0, + }, + mode: FileModeInformation { mode: 0 }, + alignment: FileAlignmentInformation { + alignment_requirement: 0, + }, + name: FileNameInformation { + file_name_length: 0, + file_name: String::new(), + }, + }; + let all_bytes = all.to_bytes(); + let basic_bytes = basic.smb_to_bytes(); + assert_eq!(&all_bytes[..40], &basic_bytes[..]); + } +} diff --git a/smb/src/protocol/body/mod.rs b/smb/src/protocol/body/mod.rs index 7db66f5..a8290ad 100644 --- a/smb/src/protocol/body/mod.rs +++ b/smb/src/protocol/body/mod.rs @@ -56,6 +56,7 @@ pub mod create; pub mod echo; pub mod empty; pub mod error; +pub mod file_info; pub mod flush; pub mod ioctl; pub mod lock; diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 6f38d97..e40372d 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -4,15 +4,21 @@ use std::sync::{Arc, Weak}; use tokio::sync::RwLock; -use smb_core::SMBResult; use smb_core::error::SMBError; use smb_core::logging::{debug, trace}; use smb_core::nt_status::NTStatus; +use smb_core::{SMBByteSize, SMBResult, SMBToBytes}; use crate::protocol::body::SMBBody; use crate::protocol::body::close::{SMBCloseRequest, SMBCloseResponse}; +use crate::protocol::body::create::file_attributes::SMBFileAttributes; use crate::protocol::body::create::file_id::SMBFileId; use crate::protocol::body::create::{SMBCreateRequest, SMBCreateResponse}; +use crate::protocol::body::file_info::{ + FileAccessInformation, FileAlignmentInformation, FileAllInformation, FileBasicInformation, + FileEaInformation, FileInternalInformation, FileModeInformation, FileNameInformation, + FileNetworkOpenInformation, FilePositionInformation, FileStandardInformation, +}; use crate::protocol::body::filetime::FileTime; use crate::protocol::body::query_info::info_type::SMBInfoType; use crate::protocol::body::query_info::{SMBQueryInfoRequest, SMBQueryInfoResponse}; @@ -77,96 +83,70 @@ impl SMBTreeConnect { .ok_or(SMBError::response_error(NTStatus::FileClosed)) } - /// Build a FILE_BASIC_INFORMATION buffer (MS-FSCC 2.4.7) from open metadata. - fn build_file_basic_info(open: &S::Open) -> SMBResult> { + fn build_basic_info(open: &S::Open) -> SMBResult { let metadata = open.file_metadata()?; - let mut buf = Vec::with_capacity(40); - buf.extend_from_slice(&metadata.creation_time.as_bytes()); // CreationTime (8) - buf.extend_from_slice(&metadata.last_access_time.as_bytes()); // LastAccessTime (8) - buf.extend_from_slice(&metadata.last_write_time.as_bytes()); // LastWriteTime (8) - buf.extend_from_slice(&metadata.last_modification_time.as_bytes()); // ChangeTime (8) - buf.extend_from_slice(&(open.file_attributes().bits() as u32).to_le_bytes()); // FileAttributes (4) - buf.extend_from_slice(&[0u8; 4]); // Reserved (4) - Ok(buf) + Ok(FileBasicInformation { + creation_time: metadata.creation_time, + last_access_time: metadata.last_access_time, + last_write_time: metadata.last_write_time, + change_time: metadata.last_modification_time, + file_attributes: open.file_attributes(), + reserved: 0, + }) } - /// Build a FILE_STANDARD_INFORMATION buffer (MS-FSCC 2.4.41) from open metadata. - fn build_file_standard_info(open: &S::Open) -> SMBResult> { + fn build_standard_info(open: &S::Open) -> SMBResult { let metadata = open.file_metadata()?; - let mut buf = Vec::with_capacity(24); - buf.extend_from_slice(&metadata.allocated_size.to_le_bytes()); // AllocationSize (8) - buf.extend_from_slice(&metadata.actual_size.to_le_bytes()); // EndOfFile (8) - buf.extend_from_slice(&1u32.to_le_bytes()); // NumberOfLinks (4) - buf.push(0); // DeletePending (1) - buf.push( - if open.file_attributes().contains( - crate::protocol::body::create::file_attributes::SMBFileAttributes::DIRECTORY, - ) { - 1 - } else { - 0 - }, - ); // Directory (1) - buf.extend_from_slice(&[0u8; 2]); // Reserved (2) - Ok(buf) + let is_dir = open + .file_attributes() + .contains(SMBFileAttributes::DIRECTORY); + Ok(FileStandardInformation { + allocation_size: metadata.allocated_size, + end_of_file: metadata.actual_size, + number_of_links: 1, + delete_pending: 0, + directory: if is_dir { 1 } else { 0 }, + reserved: 0, + }) } - /// Build a FILE_NETWORK_OPEN_INFORMATION buffer (MS-FSCC 2.4.29) from open metadata. - fn build_file_network_open_info(open: &S::Open) -> SMBResult> { + fn build_network_open_info(open: &S::Open) -> SMBResult { let metadata = open.file_metadata()?; - let mut buf = Vec::with_capacity(56); - buf.extend_from_slice(&metadata.creation_time.as_bytes()); // CreationTime (8) - buf.extend_from_slice(&metadata.last_access_time.as_bytes()); // LastAccessTime (8) - buf.extend_from_slice(&metadata.last_write_time.as_bytes()); // LastWriteTime (8) - buf.extend_from_slice(&metadata.last_modification_time.as_bytes()); // ChangeTime (8) - buf.extend_from_slice(&metadata.allocated_size.to_le_bytes()); // AllocationSize (8) - buf.extend_from_slice(&metadata.actual_size.to_le_bytes()); // EndOfFile (8) - buf.extend_from_slice(&(open.file_attributes().bits() as u32).to_le_bytes()); // FileAttributes (4) - buf.extend_from_slice(&[0u8; 4]); // Reserved (4) - Ok(buf) + Ok(FileNetworkOpenInformation { + creation_time: metadata.creation_time, + last_access_time: metadata.last_access_time, + last_write_time: metadata.last_write_time, + change_time: metadata.last_modification_time, + allocation_size: metadata.allocated_size, + end_of_file: metadata.actual_size, + file_attributes: open.file_attributes(), + reserved: 0, + }) } - /// Build a FILE_ALL_INFORMATION buffer (MS-FSCC 2.4.2) from open metadata. - /// Composite of Basic + Standard + Internal + EA + Access + Position + Mode + Alignment + Name. - fn build_file_all_info(open: &S::Open) -> SMBResult> { - let metadata = open.file_metadata()?; - let is_dir = open - .file_attributes() - .contains(crate::protocol::body::create::file_attributes::SMBFileAttributes::DIRECTORY); - let mut buf = Vec::with_capacity(104); - // FileBasicInformation (40 bytes) - buf.extend_from_slice(&metadata.creation_time.as_bytes()); - buf.extend_from_slice(&metadata.last_access_time.as_bytes()); - buf.extend_from_slice(&metadata.last_write_time.as_bytes()); - buf.extend_from_slice(&metadata.last_modification_time.as_bytes()); - buf.extend_from_slice(&(open.file_attributes().bits() as u32).to_le_bytes()); - buf.extend_from_slice(&[0u8; 4]); // Reserved - // FileStandardInformation (24 bytes) - buf.extend_from_slice(&metadata.allocated_size.to_le_bytes()); - buf.extend_from_slice(&metadata.actual_size.to_le_bytes()); - buf.extend_from_slice(&1u32.to_le_bytes()); // NumberOfLinks - buf.push(0); // DeletePending - buf.push(if is_dir { 1 } else { 0 }); // Directory - buf.extend_from_slice(&[0u8; 2]); // Reserved - // FileInternalInformation (8 bytes) - buf.extend_from_slice(&0u64.to_le_bytes()); // IndexNumber - // FileEaInformation (4 bytes) - buf.extend_from_slice(&0u32.to_le_bytes()); // EaSize - // FileAccessInformation (4 bytes) - buf.extend_from_slice(&0x001f01ffu32.to_le_bytes()); // AccessFlags (GENERIC_ALL) - // FilePositionInformation (8 bytes) - buf.extend_from_slice(&0u64.to_le_bytes()); // CurrentByteOffset - // FileModeInformation (4 bytes) - buf.extend_from_slice(&0u32.to_le_bytes()); // Mode - // FileAlignmentInformation (4 bytes) - buf.extend_from_slice(&0u32.to_le_bytes()); // AlignmentRequirement - // FileNameInformation (variable) + fn build_all_info(open: &S::Open) -> SMBResult { let name = open.file_name(); - let name_utf16: Vec = name.encode_utf16().collect(); - let name_bytes: Vec = name_utf16.iter().flat_map(|c| c.to_le_bytes()).collect(); - buf.extend_from_slice(&(name_bytes.len() as u32).to_le_bytes()); // FileNameLength - buf.extend_from_slice(&name_bytes); // FileName - Ok(buf) + let name_byte_len = (name.encode_utf16().count() * 2) as u32; + Ok(FileAllInformation { + basic: Self::build_basic_info(open)?, + standard: Self::build_standard_info(open)?, + internal: FileInternalInformation { index_number: 0 }, + ea: FileEaInformation { ea_size: 0 }, + access: FileAccessInformation { + access_flags: 0x001f01ff, + }, + position: FilePositionInformation { + current_byte_offset: 0, + }, + mode: FileModeInformation { mode: 0 }, + alignment: FileAlignmentInformation { + alignment_requirement: 0, + }, + name: FileNameInformation { + file_name_length: name_byte_len, + file_name: name.into(), + }, + }) } } @@ -301,10 +281,10 @@ impl SMBLockedMessageHandlerBase for Arc> { SMBInfoType::File => { // MS-FSCC file information classes match message.file_info_class() { - 4 => SMBTreeConnect::::build_file_basic_info(&*open_rd)?, // FileBasicInformation - 5 => SMBTreeConnect::::build_file_standard_info(&*open_rd)?, // FileStandardInformation - 18 => SMBTreeConnect::::build_file_all_info(&*open_rd)?, // FileAllInformation - 34 => SMBTreeConnect::::build_file_network_open_info(&*open_rd)?, // FileNetworkOpenInformation + 4 => SMBTreeConnect::::build_basic_info(&*open_rd)?.smb_to_bytes(), + 5 => SMBTreeConnect::::build_standard_info(&*open_rd)?.smb_to_bytes(), + 18 => SMBTreeConnect::::build_all_info(&*open_rd)?.to_bytes(), + 34 => SMBTreeConnect::::build_network_open_info(&*open_rd)?.smb_to_bytes(), _ => { debug!( class = message.file_info_class(), From 56eb98e9048f2ead0a832da49bf35c15c084d82c Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Wed, 11 Feb 2026 19:52:01 -0500 Subject: [PATCH 04/15] refactor: split file_info structs into separate files per project convention Move each MS-FSCC file information struct out of mod.rs into its own file (access.rs, alignment.rs, basic.rs, ea.rs, internal.rs, mode.rs, name.rs, network_open.rs, position.rs, standard.rs). mod.rs now only contains module declarations, re-exports, and the composite FileAllInformation type. All 80 unit tests and 10 integration tests pass. --- smb/src/protocol/body/file_info/access.rs | 12 ++ smb/src/protocol/body/file_info/alignment.rs | 12 ++ smb/src/protocol/body/file_info/basic.rs | 25 +++ smb/src/protocol/body/file_info/ea.rs | 12 ++ smb/src/protocol/body/file_info/internal.rs | 12 ++ smb/src/protocol/body/file_info/mod.rs | 158 +++--------------- smb/src/protocol/body/file_info/mode.rs | 12 ++ smb/src/protocol/body/file_info/name.rs | 19 +++ .../protocol/body/file_info/network_open.rs | 29 ++++ smb/src/protocol/body/file_info/position.rs | 12 ++ smb/src/protocol/body/file_info/standard.rs | 22 +++ 11 files changed, 189 insertions(+), 136 deletions(-) create mode 100644 smb/src/protocol/body/file_info/access.rs create mode 100644 smb/src/protocol/body/file_info/alignment.rs create mode 100644 smb/src/protocol/body/file_info/basic.rs create mode 100644 smb/src/protocol/body/file_info/ea.rs create mode 100644 smb/src/protocol/body/file_info/internal.rs create mode 100644 smb/src/protocol/body/file_info/mode.rs create mode 100644 smb/src/protocol/body/file_info/name.rs create mode 100644 smb/src/protocol/body/file_info/network_open.rs create mode 100644 smb/src/protocol/body/file_info/position.rs create mode 100644 smb/src/protocol/body/file_info/standard.rs diff --git a/smb/src/protocol/body/file_info/access.rs b/smb/src/protocol/body/file_info/access.rs new file mode 100644 index 0000000..10b63cf --- /dev/null +++ b/smb/src/protocol/body/file_info/access.rs @@ -0,0 +1,12 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileAccessInformation { + #[smb_direct(start(fixed = 0))] + pub access_flags: u32, +} diff --git a/smb/src/protocol/body/file_info/alignment.rs b/smb/src/protocol/body/file_info/alignment.rs new file mode 100644 index 0000000..e039bd1 --- /dev/null +++ b/smb/src/protocol/body/file_info/alignment.rs @@ -0,0 +1,12 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_ALIGNMENT_INFORMATION (MS-FSCC 2.4.3) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileAlignmentInformation { + #[smb_direct(start(fixed = 0))] + pub alignment_requirement: u32, +} diff --git a/smb/src/protocol/body/file_info/basic.rs b/smb/src/protocol/body/file_info/basic.rs new file mode 100644 index 0000000..3451adb --- /dev/null +++ b/smb/src/protocol/body/file_info/basic.rs @@ -0,0 +1,25 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +use crate::protocol::body::create::file_attributes::SMBFileAttributes; +use crate::protocol::body::filetime::FileTime; + +/// FILE_BASIC_INFORMATION (MS-FSCC 2.4.7) — 40 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileBasicInformation { + #[smb_direct(start(fixed = 0))] + pub creation_time: FileTime, + #[smb_direct(start(fixed = 8))] + pub last_access_time: FileTime, + #[smb_direct(start(fixed = 16))] + pub last_write_time: FileTime, + #[smb_direct(start(fixed = 24))] + pub change_time: FileTime, + #[smb_direct(start(fixed = 32))] + pub file_attributes: SMBFileAttributes, + #[smb_direct(start(fixed = 36))] + pub reserved: u32, +} diff --git a/smb/src/protocol/body/file_info/ea.rs b/smb/src/protocol/body/file_info/ea.rs new file mode 100644 index 0000000..8372ca7 --- /dev/null +++ b/smb/src/protocol/body/file_info/ea.rs @@ -0,0 +1,12 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_EA_INFORMATION (MS-FSCC 2.4.12) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileEaInformation { + #[smb_direct(start(fixed = 0))] + pub ea_size: u32, +} diff --git a/smb/src/protocol/body/file_info/internal.rs b/smb/src/protocol/body/file_info/internal.rs new file mode 100644 index 0000000..723fb4d --- /dev/null +++ b/smb/src/protocol/body/file_info/internal.rs @@ -0,0 +1,12 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_INTERNAL_INFORMATION (MS-FSCC 2.4.20) — 8 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileInternalInformation { + #[smb_direct(start(fixed = 0))] + pub index_number: u64, +} diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index cd68ddd..f8590a4 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -3,143 +3,27 @@ //! Typed representations of the file information structures defined in //! [MS-FSCC] sections 2.4.x, used in QueryInfo / SetInfo responses. -use serde::{Deserialize, Serialize}; +mod access; +mod alignment; +mod basic; +mod ea; +mod internal; +mod mode; +mod name; +mod network_open; +mod position; +mod standard; -use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; - -use crate::protocol::body::create::file_attributes::SMBFileAttributes; -use crate::protocol::body::filetime::FileTime; - -/// FILE_BASIC_INFORMATION (MS-FSCC 2.4.7) — 40 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileBasicInformation { - #[smb_direct(start(fixed = 0))] - pub creation_time: FileTime, - #[smb_direct(start(fixed = 8))] - pub last_access_time: FileTime, - #[smb_direct(start(fixed = 16))] - pub last_write_time: FileTime, - #[smb_direct(start(fixed = 24))] - pub change_time: FileTime, - #[smb_direct(start(fixed = 32))] - pub file_attributes: SMBFileAttributes, - #[smb_direct(start(fixed = 36))] - pub reserved: u32, -} - -/// FILE_STANDARD_INFORMATION (MS-FSCC 2.4.41) — 24 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileStandardInformation { - #[smb_direct(start(fixed = 0))] - pub allocation_size: u64, - #[smb_direct(start(fixed = 8))] - pub end_of_file: u64, - #[smb_direct(start(fixed = 16))] - pub number_of_links: u32, - #[smb_direct(start(fixed = 20))] - pub delete_pending: u8, - #[smb_direct(start(fixed = 21))] - pub directory: u8, - #[smb_direct(start(fixed = 22))] - pub reserved: u16, -} - -/// FILE_INTERNAL_INFORMATION (MS-FSCC 2.4.20) — 8 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileInternalInformation { - #[smb_direct(start(fixed = 0))] - pub index_number: u64, -} - -/// FILE_EA_INFORMATION (MS-FSCC 2.4.12) — 4 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileEaInformation { - #[smb_direct(start(fixed = 0))] - pub ea_size: u32, -} - -/// FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1) — 4 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileAccessInformation { - #[smb_direct(start(fixed = 0))] - pub access_flags: u32, -} - -/// FILE_POSITION_INFORMATION (MS-FSCC 2.4.35) — 8 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FilePositionInformation { - #[smb_direct(start(fixed = 0))] - pub current_byte_offset: u64, -} - -/// FILE_MODE_INFORMATION (MS-FSCC 2.4.26) — 4 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileModeInformation { - #[smb_direct(start(fixed = 0))] - pub mode: u32, -} - -/// FILE_ALIGNMENT_INFORMATION (MS-FSCC 2.4.3) — 4 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileAlignmentInformation { - #[smb_direct(start(fixed = 0))] - pub alignment_requirement: u32, -} - -/// FILE_NAME_INFORMATION (MS-FSCC 2.4.28) — variable length -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileNameInformation { - #[smb_direct(start(fixed = 0))] - pub file_name_length: u32, - #[smb_string( - order = 0, - start(fixed = 4), - length(inner(start = 0, num_type = "u32")), - underlying = "u16" - )] - pub file_name: String, -} - -/// FILE_NETWORK_OPEN_INFORMATION (MS-FSCC 2.4.29) — 56 bytes -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, -)] -pub struct FileNetworkOpenInformation { - #[smb_direct(start(fixed = 0))] - pub creation_time: FileTime, - #[smb_direct(start(fixed = 8))] - pub last_access_time: FileTime, - #[smb_direct(start(fixed = 16))] - pub last_write_time: FileTime, - #[smb_direct(start(fixed = 24))] - pub change_time: FileTime, - #[smb_direct(start(fixed = 32))] - pub allocation_size: u64, - #[smb_direct(start(fixed = 40))] - pub end_of_file: u64, - #[smb_direct(start(fixed = 48))] - pub file_attributes: SMBFileAttributes, - #[smb_direct(start(fixed = 52))] - pub reserved: u32, -} +pub use access::FileAccessInformation; +pub use alignment::FileAlignmentInformation; +pub use basic::FileBasicInformation; +pub use ea::FileEaInformation; +pub use internal::FileInternalInformation; +pub use mode::FileModeInformation; +pub use name::FileNameInformation; +pub use network_open::FileNetworkOpenInformation; +pub use position::FilePositionInformation; +pub use standard::FileStandardInformation; /// FILE_ALL_INFORMATION (MS-FSCC 2.4.2) — composite structure /// @@ -180,6 +64,8 @@ impl FileAllInformation { #[cfg(test)] mod tests { use super::*; + use crate::protocol::body::create::file_attributes::SMBFileAttributes; + use crate::protocol::body::filetime::FileTime; use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; #[test] diff --git a/smb/src/protocol/body/file_info/mode.rs b/smb/src/protocol/body/file_info/mode.rs new file mode 100644 index 0000000..04e0d6b --- /dev/null +++ b/smb/src/protocol/body/file_info/mode.rs @@ -0,0 +1,12 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_MODE_INFORMATION (MS-FSCC 2.4.26) — 4 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileModeInformation { + #[smb_direct(start(fixed = 0))] + pub mode: u32, +} diff --git a/smb/src/protocol/body/file_info/name.rs b/smb/src/protocol/body/file_info/name.rs new file mode 100644 index 0000000..f5eaeb5 --- /dev/null +++ b/smb/src/protocol/body/file_info/name.rs @@ -0,0 +1,19 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_NAME_INFORMATION (MS-FSCC 2.4.28) — variable length +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileNameInformation { + #[smb_direct(start(fixed = 0))] + pub file_name_length: u32, + #[smb_string( + order = 0, + start(fixed = 4), + length(inner(start = 0, num_type = "u32")), + underlying = "u16" + )] + pub file_name: String, +} diff --git a/smb/src/protocol/body/file_info/network_open.rs b/smb/src/protocol/body/file_info/network_open.rs new file mode 100644 index 0000000..3d79fbf --- /dev/null +++ b/smb/src/protocol/body/file_info/network_open.rs @@ -0,0 +1,29 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +use crate::protocol::body::create::file_attributes::SMBFileAttributes; +use crate::protocol::body::filetime::FileTime; + +/// FILE_NETWORK_OPEN_INFORMATION (MS-FSCC 2.4.29) — 56 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileNetworkOpenInformation { + #[smb_direct(start(fixed = 0))] + pub creation_time: FileTime, + #[smb_direct(start(fixed = 8))] + pub last_access_time: FileTime, + #[smb_direct(start(fixed = 16))] + pub last_write_time: FileTime, + #[smb_direct(start(fixed = 24))] + pub change_time: FileTime, + #[smb_direct(start(fixed = 32))] + pub allocation_size: u64, + #[smb_direct(start(fixed = 40))] + pub end_of_file: u64, + #[smb_direct(start(fixed = 48))] + pub file_attributes: SMBFileAttributes, + #[smb_direct(start(fixed = 52))] + pub reserved: u32, +} diff --git a/smb/src/protocol/body/file_info/position.rs b/smb/src/protocol/body/file_info/position.rs new file mode 100644 index 0000000..d74b3b6 --- /dev/null +++ b/smb/src/protocol/body/file_info/position.rs @@ -0,0 +1,12 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_POSITION_INFORMATION (MS-FSCC 2.4.35) — 8 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FilePositionInformation { + #[smb_direct(start(fixed = 0))] + pub current_byte_offset: u64, +} diff --git a/smb/src/protocol/body/file_info/standard.rs b/smb/src/protocol/body/file_info/standard.rs new file mode 100644 index 0000000..340aa68 --- /dev/null +++ b/smb/src/protocol/body/file_info/standard.rs @@ -0,0 +1,22 @@ +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + +/// FILE_STANDARD_INFORMATION (MS-FSCC 2.4.41) — 24 bytes +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] +pub struct FileStandardInformation { + #[smb_direct(start(fixed = 0))] + pub allocation_size: u64, + #[smb_direct(start(fixed = 8))] + pub end_of_file: u64, + #[smb_direct(start(fixed = 16))] + pub number_of_links: u32, + #[smb_direct(start(fixed = 20))] + pub delete_pending: u8, + #[smb_direct(start(fixed = 21))] + pub directory: u8, + #[smb_direct(start(fixed = 22))] + pub reserved: u16, +} From 27765c8a07974ae81cafbe2302fc5e8fe8a66b5a Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sun, 29 Mar 2026 17:47:02 -0400 Subject: [PATCH 05/15] fix: correct FileId construction, close cleanup, and DurableFileId verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B1: file_id() now uses global_id for persistent (DurableFileId) and session_id for volatile (FileId) per MS-SMB2 §2.2.14.1 / §3.3.1.10. B2: handle_close remove_open now passes file_id.persistent (global_id) to the server's GlobalOpenTable instead of file_id.volatile. B3: find_open and handle_close now verify Open.DurableFileId == FileId.Persistent per MS-SMB2 §3.3.5.10/12/20, returning STATUS_FILE_CLOSED on mismatch. B4: handle_create response header now passes 0 (STATUS_SUCCESS) instead of echoing the request's ChannelSequence into the response Status field (MS-SMB2 §2.2.1.2). B6: handle_close logs warn! when server/connection lock acquisition fails during GlobalOpenTable cleanup. B7: FileAllInformation::to_bytes() capacity hint now accounts for the variable-length file name. Tests: added SMBFileId wire-format unit tests (size, layout, round-trip, field independence). --- smb/src/protocol/body/create/file_id.rs | 59 +++++++++++++++++++++++++ smb/src/protocol/body/file_info/mod.rs | 2 +- smb/src/server/open.rs | 4 +- smb/src/server/tree_connect.rs | 31 ++++++++----- 4 files changed, 83 insertions(+), 13 deletions(-) diff --git a/smb/src/protocol/body/create/file_id.rs b/smb/src/protocol/body/create/file_id.rs index 85237b2..f3cb795 100644 --- a/smb/src/protocol/body/create/file_id.rs +++ b/smb/src/protocol/body/create/file_id.rs @@ -11,3 +11,62 @@ pub struct SMBFileId { #[smb_direct(start(fixed = 8))] pub volatile: u64, } + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; + + /// MS-SMB2 section 2.2.14.1: SMB2_FILEID is 16 bytes (Persistent u64 + Volatile u64) + #[test] + fn file_id_is_16_bytes() { + let fid = SMBFileId { + persistent: 0, + volatile: 0, + }; + assert_eq!(fid.smb_byte_size(), 16); + } + + /// Persistent is at offset 0, Volatile at offset 8 + #[test] + fn file_id_wire_layout() { + let fid = SMBFileId { + persistent: 0xDEAD, + volatile: 0xBEEF, + }; + let bytes = fid.smb_to_bytes(); + assert_eq!(bytes.len(), 16); + let persistent = u64::from_le_bytes(bytes[0..8].try_into().unwrap()); + let volatile = u64::from_le_bytes(bytes[8..16].try_into().unwrap()); + assert_eq!(persistent, 0xDEAD); + assert_eq!(volatile, 0xBEEF); + } + + #[test] + fn file_id_round_trip() { + let fid = SMBFileId { + persistent: 42, + volatile: 99, + }; + let bytes = fid.smb_to_bytes(); + let (_, parsed) = SMBFileId::smb_from_bytes(&bytes).unwrap(); + assert_eq!(fid, parsed); + } + + /// Per section 2.2.14.1, persistent and volatile are distinct fields. + /// Verify they serialize independently. + #[test] + fn file_id_persistent_and_volatile_are_independent() { + let a = SMBFileId { + persistent: 1, + volatile: 2, + }; + let b = SMBFileId { + persistent: 2, + volatile: 1, + }; + let bytes_a = a.smb_to_bytes(); + let bytes_b = b.smb_to_bytes(); + assert_ne!(bytes_a, bytes_b); + } +} diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index f8590a4..79342fa 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -47,7 +47,7 @@ pub struct FileAllInformation { impl FileAllInformation { pub fn to_bytes(&self) -> Vec { use smb_core::SMBToBytes; - let mut buf = Vec::with_capacity(104); + let mut buf = Vec::with_capacity(96 + self.name.file_name.len() * 2 + 4); buf.extend_from_slice(&self.basic.smb_to_bytes()); buf.extend_from_slice(&self.standard.smb_to_bytes()); buf.extend_from_slice(&self.internal.smb_to_bytes()); diff --git a/smb/src/server/open.rs b/smb/src/server/open.rs index 7002ca7..93ba01b 100644 --- a/smb/src/server/open.rs +++ b/smb/src/server/open.rs @@ -142,8 +142,8 @@ impl Open for SMBOpen { fn file_id(&self) -> SMBFileId { SMBFileId { - persistent: self.session_id, - volatile: self.session_id, + persistent: self.global_id as u64, + volatile: self.session_id as u64, } } diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index e40372d..190b72c 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -5,7 +5,7 @@ use std::sync::{Arc, Weak}; use tokio::sync::RwLock; use smb_core::error::SMBError; -use smb_core::logging::{debug, trace}; +use smb_core::logging::{debug, trace, warn}; use smb_core::nt_status::NTStatus; use smb_core::{SMBByteSize, SMBResult, SMBToBytes}; @@ -76,11 +76,18 @@ impl SMBTreeConnect { async fn find_open(&self, file_id: &SMBFileId) -> SMBResult>> { let session = self.get_session()?; let session_rd = session.read().await; - session_rd + let open = session_rd .open_table() .get(&file_id.volatile) .cloned() - .ok_or(SMBError::response_error(NTStatus::FileClosed)) + .ok_or(SMBError::response_error(NTStatus::FileClosed))?; + // MS-SMB2 §3.3.5.10/12/20: verify Open.DurableFileId == FileId.Persistent + let open_rd = open.read().await; + if open_rd.file_id().persistent != file_id.persistent { + return Err(SMBError::response_error(NTStatus::FileClosed)); + } + drop(open_rd); + Ok(open) } fn build_basic_info(open: &S::Open) -> SMBResult { @@ -179,11 +186,7 @@ impl SMBLockedMessageHandlerBase for Arc> { session.write().await.set_previous_file_id(file_id); } debug!("tree connect create handled"); - let header = header.create_response_header( - header.channel_sequence, - header.session_id, - header.tree_id, - ); + let header = header.create_response_header(0, header.session_id, header.tree_id); trace!( response_size = response.smb_byte_size(), "create response built" @@ -210,6 +213,10 @@ impl SMBLockedMessageHandlerBase for Arc> { }; let (response, file_id) = { let open_rd = open.read().await; + // MS-SMB2 section 3.3.5.10: verify Open.DurableFileId == FileId.Persistent + if open_rd.file_id().persistent != message.file_id().persistent { + return Err(SMBError::response_error(NTStatus::FileClosed)); + } let response = if message .flags() .contains(crate::protocol::body::close::flags::SMBCloseFlags::POSTQUERY_ATTRIB) @@ -223,11 +230,15 @@ impl SMBLockedMessageHandlerBase for Arc> { }; // Phase 2: Cleanup — acquire locks outer to inner (server_wr, then session_wr) - // Server write first (outermost) + // Server write first (outermost) — use persistent (global_id) as GlobalOpenTable key if let Ok(conn) = session.upper().await { if let Ok(server) = conn.upper().await { - server.write().await.remove_open(file_id.volatile as u32); + server.write().await.remove_open(file_id.persistent as u32); + } else { + warn!(file_id = ?file_id, "failed to acquire server lock during close; global open table entry leaked"); } + } else { + warn!(file_id = ?file_id, "failed to acquire connection lock during close; global open table entry leaked"); } // Session write second (inner relative to server) { From f04b7b4a89975f6879a3d2a50049a470c0c1548c Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sun, 29 Mar 2026 17:53:39 -0400 Subject: [PATCH 06/15] refactor: replace manual FileAllInformation::to_bytes() with SMBToBytes derive FileAllInformation now uses #[derive(SMBByteSize, SMBFromBytes, SMBToBytes)] with #[smb_direct(start(fixed = N))] annotations for each sub-struct at its computed offset. Removes the manual to_bytes() impl. Adds a round-trip test for FileAllInformation (serialize + deserialize). --- smb/src/protocol/body/file_info/mod.rs | 74 +++++++++++++++++--------- smb/src/server/tree_connect.rs | 2 +- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index 79342fa..018111c 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -25,42 +25,37 @@ pub use network_open::FileNetworkOpenInformation; pub use position::FilePositionInformation; pub use standard::FileStandardInformation; +use serde::{Deserialize, Serialize}; + +use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; + /// FILE_ALL_INFORMATION (MS-FSCC 2.4.2) — composite structure /// -/// This is a concatenation of the sub-structures above. -/// We serialize it by concatenating each sub-struct's bytes rather than -/// using the derive macro, because the derive macro doesn't support -/// nested struct composition at variable offsets. -#[derive(Debug, PartialEq, Eq, Clone)] +/// Concatenation of sub-structures at fixed offsets: +/// basic(40) + standard(24) + internal(8) + ea(4) + access(4) +/// + position(8) + mode(4) + alignment(4) + name(variable). +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes)] pub struct FileAllInformation { + #[smb_direct(start(fixed = 0))] pub basic: FileBasicInformation, + #[smb_direct(start(fixed = 40))] pub standard: FileStandardInformation, + #[smb_direct(start(fixed = 64))] pub internal: FileInternalInformation, + #[smb_direct(start(fixed = 72))] pub ea: FileEaInformation, + #[smb_direct(start(fixed = 76))] pub access: FileAccessInformation, + #[smb_direct(start(fixed = 80))] pub position: FilePositionInformation, + #[smb_direct(start(fixed = 88))] pub mode: FileModeInformation, + #[smb_direct(start(fixed = 92))] pub alignment: FileAlignmentInformation, + #[smb_direct(start(fixed = 96))] pub name: FileNameInformation, } -impl FileAllInformation { - pub fn to_bytes(&self) -> Vec { - use smb_core::SMBToBytes; - let mut buf = Vec::with_capacity(96 + self.name.file_name.len() * 2 + 4); - buf.extend_from_slice(&self.basic.smb_to_bytes()); - buf.extend_from_slice(&self.standard.smb_to_bytes()); - buf.extend_from_slice(&self.internal.smb_to_bytes()); - buf.extend_from_slice(&self.ea.smb_to_bytes()); - buf.extend_from_slice(&self.access.smb_to_bytes()); - buf.extend_from_slice(&self.position.smb_to_bytes()); - buf.extend_from_slice(&self.mode.smb_to_bytes()); - buf.extend_from_slice(&self.alignment.smb_to_bytes()); - buf.extend_from_slice(&self.name.smb_to_bytes()); - buf - } -} - #[cfg(test)] mod tests { use super::*; @@ -255,7 +250,7 @@ mod tests { file_name: "testfile.txt".into(), }, }; - let bytes = all.to_bytes(); + let bytes = all.smb_to_bytes(); // 40 + 24 + 8 + 4 + 4 + 8 + 4 + 4 + (4 + 24) = 124 assert_eq!(bytes.len(), 124); } @@ -295,8 +290,39 @@ mod tests { file_name: String::new(), }, }; - let all_bytes = all.to_bytes(); + let all_bytes = all.smb_to_bytes(); let basic_bytes = basic.smb_to_bytes(); assert_eq!(&all_bytes[..40], &basic_bytes[..]); } + + #[test] + fn file_all_information_round_trip() { + let all = FileAllInformation { + basic: FileBasicInformation { + creation_time: FileTime::now(), + last_access_time: FileTime::now(), + last_write_time: FileTime::now(), + change_time: FileTime::now(), + file_attributes: SMBFileAttributes::ARCHIVE, + reserved: 0, + }, + standard: FileStandardInformation { + allocation_size: 4096, end_of_file: 512, number_of_links: 1, + delete_pending: 0, directory: 0, reserved: 0, + }, + internal: FileInternalInformation { index_number: 7 }, + ea: FileEaInformation { ea_size: 0 }, + access: FileAccessInformation { access_flags: 0x001f01ff }, + position: FilePositionInformation { current_byte_offset: 256 }, + mode: FileModeInformation { mode: 0 }, + alignment: FileAlignmentInformation { alignment_requirement: 0 }, + name: FileNameInformation { + file_name_length: 24, + file_name: "testfile.txt".into(), + }, + }; + let bytes = all.smb_to_bytes(); + let (_, parsed) = FileAllInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(all, parsed); + } } diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 190b72c..49599d1 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -294,7 +294,7 @@ impl SMBLockedMessageHandlerBase for Arc> { match message.file_info_class() { 4 => SMBTreeConnect::::build_basic_info(&*open_rd)?.smb_to_bytes(), 5 => SMBTreeConnect::::build_standard_info(&*open_rd)?.smb_to_bytes(), - 18 => SMBTreeConnect::::build_all_info(&*open_rd)?.to_bytes(), + 18 => SMBTreeConnect::::build_all_info(&*open_rd)?.smb_to_bytes(), 34 => SMBTreeConnect::::build_network_open_info(&*open_rd)?.smb_to_bytes(), _ => { debug!( From a806452f69ce902b05d09f538da1a66c7f88b758 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Mon, 30 Mar 2026 20:12:20 -0400 Subject: [PATCH 07/15] refactor: replace raw u32 fields with typed bitflags/enums in file_info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - FileAccessInformation.access_flags: u32 → FileAccessFlags bitflags (MS-FSCC §2.4.1 / MS-DTYP §2.4.3 ACCESS_MASK) - FileAlignmentInformation.alignment_requirement: u32 → FileAlignmentRequirement enum (MS-FSCC §2.4.3 device alignment boundaries) - FileModeInformation.mode: u32 → FileModeFlags bitflags (MS-FSCC §2.4.26 file mode flags) FileEaInformation.ea_size remains u32 — it is a plain byte count per MS-FSCC §2.4.12, not flags or an enum. All types exported from file_info module. Tests and tree_connect.rs call site updated to use typed constructors. --- smb/src/protocol/body/file_info/access.rs | 41 ++++++++++++- smb/src/protocol/body/file_info/alignment.rs | 64 +++++++++++++++++++- smb/src/protocol/body/file_info/mod.rs | 60 ++++++++++++------ smb/src/protocol/body/file_info/mode.rs | 24 +++++++- smb/src/server/tree_connect.rs | 15 +++-- 5 files changed, 174 insertions(+), 30 deletions(-) diff --git a/smb/src/protocol/body/file_info/access.rs b/smb/src/protocol/body/file_info/access.rs index 10b63cf..c3ec191 100644 --- a/smb/src/protocol/body/file_info/access.rs +++ b/smb/src/protocol/body/file_info/access.rs @@ -1,12 +1,51 @@ +use bitflags::bitflags; use serde::{Deserialize, Serialize}; use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; +use crate::util::flags_helper::{ + impl_smb_byte_size_for_bitflag, impl_smb_from_bytes_for_bitflag, impl_smb_to_bytes_for_bitflag, +}; + +/// ACCESS_MASK flags for FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1). +/// +/// These are the same ACCESS_MASK values defined in [MS-DTYP] §2.4.3 / +/// [MS-SMB2] §2.2.13.1, representing the access rights granted on the open. +bitflags! { + #[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)] + pub struct FileAccessFlags: u32 { + const FILE_READ_DATA = 0x00000001; + const FILE_WRITE_DATA = 0x00000002; + const FILE_APPEND_DATA = 0x00000004; + const FILE_READ_EA = 0x00000008; + const FILE_WRITE_EA = 0x00000010; + const FILE_EXECUTE = 0x00000020; + const FILE_DELETE_CHILD = 0x00000040; + const FILE_READ_ATTRIBUTES = 0x00000080; + const FILE_WRITE_ATTRIBUTES = 0x00000100; + const DELETE = 0x00010000; + const READ_CONTROL = 0x00020000; + const WRITE_DAC = 0x00040000; + const WRITE_OWNER = 0x00080000; + const SYNCHRONIZE = 0x00100000; + const ACCESS_SYSTEM_SECURITY = 0x01000000; + const MAXIMUM_ALLOWED = 0x02000000; + const GENERIC_ALL = 0x10000000; + const GENERIC_EXECUTE = 0x20000000; + const GENERIC_WRITE = 0x40000000; + const GENERIC_READ = 0x80000000; + } +} + +impl_smb_byte_size_for_bitflag! { FileAccessFlags } +impl_smb_to_bytes_for_bitflag! { FileAccessFlags } +impl_smb_from_bytes_for_bitflag! { FileAccessFlags } + /// FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1) — 4 bytes #[derive( Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, )] pub struct FileAccessInformation { #[smb_direct(start(fixed = 0))] - pub access_flags: u32, + pub access_flags: FileAccessFlags, } diff --git a/smb/src/protocol/body/file_info/alignment.rs b/smb/src/protocol/body/file_info/alignment.rs index e039bd1..adbb355 100644 --- a/smb/src/protocol/body/file_info/alignment.rs +++ b/smb/src/protocol/body/file_info/alignment.rs @@ -1,12 +1,70 @@ +use num_enum::TryFromPrimitive; use serde::{Deserialize, Serialize}; -use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; +use smb_core::error::SMBError; +use smb_core::{SMBByteSize, SMBFromBytes, SMBParseResult, SMBToBytes}; +use smb_derive::{ + SMBByteSize as SMBByteSizeDerive, SMBFromBytes as SMBFromBytesDerive, + SMBToBytes as SMBToBytesDerive, +}; + +/// Device alignment requirements (MS-FSCC 2.4.3). +/// +/// Each value specifies the address boundary the device requires +/// for data transfers. For example, `Quad` means the device requires +/// 8-byte aligned addresses. +#[repr(u32)] +#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize, TryFromPrimitive)] +pub enum FileAlignmentRequirement { + Byte = 0x00000000, + Word = 0x00000001, + Long = 0x00000003, + Quad = 0x00000007, + Octa = 0x0000000F, + Align32 = 0x0000001F, + Align64 = 0x0000003F, + Align128 = 0x0000007F, + Align256 = 0x000000FF, + Align512 = 0x000001FF, +} + +impl SMBByteSize for FileAlignmentRequirement { + fn smb_byte_size(&self) -> usize { + std::mem::size_of::() + } +} + +impl SMBFromBytes for FileAlignmentRequirement { + fn smb_from_bytes(input: &[u8]) -> SMBParseResult<&[u8], Self> + where + Self: Sized, + { + u32::smb_from_bytes(input).map(|(remaining, val)| { + let req = Self::try_from_primitive(val).map_err(SMBError::parse_error)?; + Ok((remaining, req)) + })? + } +} + +impl SMBToBytes for FileAlignmentRequirement { + fn smb_to_bytes(&self) -> Vec { + (*self as u32).smb_to_bytes() + } +} /// FILE_ALIGNMENT_INFORMATION (MS-FSCC 2.4.3) — 4 bytes #[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, + Debug, + PartialEq, + Eq, + Clone, + Serialize, + Deserialize, + SMBByteSizeDerive, + SMBFromBytesDerive, + SMBToBytesDerive, )] pub struct FileAlignmentInformation { #[smb_direct(start(fixed = 0))] - pub alignment_requirement: u32, + pub alignment_requirement: FileAlignmentRequirement, } diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index 018111c..ee2684f 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -14,12 +14,12 @@ mod network_open; mod position; mod standard; -pub use access::FileAccessInformation; -pub use alignment::FileAlignmentInformation; +pub use access::{FileAccessFlags, FileAccessInformation}; +pub use alignment::{FileAlignmentInformation, FileAlignmentRequirement}; pub use basic::FileBasicInformation; pub use ea::FileEaInformation; pub use internal::FileInternalInformation; -pub use mode::FileModeInformation; +pub use mode::{FileModeFlags, FileModeInformation}; pub use name::FileNameInformation; pub use network_open::FileNetworkOpenInformation; pub use position::FilePositionInformation; @@ -34,7 +34,9 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; /// Concatenation of sub-structures at fixed offsets: /// basic(40) + standard(24) + internal(8) + ea(4) + access(4) /// + position(8) + mode(4) + alignment(4) + name(variable). -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes)] +#[derive( + Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, +)] pub struct FileAllInformation { #[smb_direct(start(fixed = 0))] pub basic: FileBasicInformation, @@ -142,7 +144,7 @@ mod tests { #[test] fn file_access_information_round_trip() { let info = FileAccessInformation { - access_flags: 0x001f01ff, + access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), }; let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); @@ -163,7 +165,9 @@ mod tests { #[test] fn file_mode_information_round_trip() { - let info = FileModeInformation { mode: 0 }; + let info = FileModeInformation { + mode: FileModeFlags::empty(), + }; let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); let (_, parsed) = FileModeInformation::smb_from_bytes(&bytes).unwrap(); @@ -173,7 +177,7 @@ mod tests { #[test] fn file_alignment_information_round_trip() { let info = FileAlignmentInformation { - alignment_requirement: 0, + alignment_requirement: FileAlignmentRequirement::Byte, }; let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); @@ -236,14 +240,16 @@ mod tests { internal: FileInternalInformation { index_number: 0 }, ea: FileEaInformation { ea_size: 0 }, access: FileAccessInformation { - access_flags: 0x001f01ff, + access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), }, position: FilePositionInformation { current_byte_offset: 0, }, - mode: FileModeInformation { mode: 0 }, + mode: FileModeInformation { + mode: FileModeFlags::empty(), + }, alignment: FileAlignmentInformation { - alignment_requirement: 0, + alignment_requirement: FileAlignmentRequirement::Byte, }, name: FileNameInformation { file_name_length: 24, @@ -277,13 +283,17 @@ mod tests { }, internal: FileInternalInformation { index_number: 0 }, ea: FileEaInformation { ea_size: 0 }, - access: FileAccessInformation { access_flags: 0 }, + access: FileAccessInformation { + access_flags: FileAccessFlags::empty(), + }, position: FilePositionInformation { current_byte_offset: 0, }, - mode: FileModeInformation { mode: 0 }, + mode: FileModeInformation { + mode: FileModeFlags::empty(), + }, alignment: FileAlignmentInformation { - alignment_requirement: 0, + alignment_requirement: FileAlignmentRequirement::Byte, }, name: FileNameInformation { file_name_length: 0, @@ -307,15 +317,27 @@ mod tests { reserved: 0, }, standard: FileStandardInformation { - allocation_size: 4096, end_of_file: 512, number_of_links: 1, - delete_pending: 0, directory: 0, reserved: 0, + allocation_size: 4096, + end_of_file: 512, + number_of_links: 1, + delete_pending: 0, + directory: 0, + reserved: 0, }, internal: FileInternalInformation { index_number: 7 }, ea: FileEaInformation { ea_size: 0 }, - access: FileAccessInformation { access_flags: 0x001f01ff }, - position: FilePositionInformation { current_byte_offset: 256 }, - mode: FileModeInformation { mode: 0 }, - alignment: FileAlignmentInformation { alignment_requirement: 0 }, + access: FileAccessInformation { + access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), + }, + position: FilePositionInformation { + current_byte_offset: 256, + }, + mode: FileModeInformation { + mode: FileModeFlags::empty(), + }, + alignment: FileAlignmentInformation { + alignment_requirement: FileAlignmentRequirement::Byte, + }, name: FileNameInformation { file_name_length: 24, file_name: "testfile.txt".into(), diff --git a/smb/src/protocol/body/file_info/mode.rs b/smb/src/protocol/body/file_info/mode.rs index 04e0d6b..701e67c 100644 --- a/smb/src/protocol/body/file_info/mode.rs +++ b/smb/src/protocol/body/file_info/mode.rs @@ -1,12 +1,34 @@ +use bitflags::bitflags; use serde::{Deserialize, Serialize}; use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; +use crate::util::flags_helper::{ + impl_smb_byte_size_for_bitflag, impl_smb_from_bytes_for_bitflag, impl_smb_to_bytes_for_bitflag, +}; + +/// Mode flags for FILE_MODE_INFORMATION (MS-FSCC 2.4.26). +bitflags! { + #[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)] + pub struct FileModeFlags: u32 { + const FILE_WRITE_THROUGH = 0x00000002; + const FILE_SEQUENTIAL_ONLY = 0x00000004; + const FILE_NO_INTERMEDIATE_BUFFERING = 0x00000008; + const FILE_SYNCHRONOUS_IO_ALERT = 0x00000010; + const FILE_SYNCHRONOUS_IO_NONALERT = 0x00000020; + const FILE_DELETE_ON_CLOSE = 0x00001000; + } +} + +impl_smb_byte_size_for_bitflag! { FileModeFlags } +impl_smb_to_bytes_for_bitflag! { FileModeFlags } +impl_smb_from_bytes_for_bitflag! { FileModeFlags } + /// FILE_MODE_INFORMATION (MS-FSCC 2.4.26) — 4 bytes #[derive( Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, )] pub struct FileModeInformation { #[smb_direct(start(fixed = 0))] - pub mode: u32, + pub mode: FileModeFlags, } diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 49599d1..debca40 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -15,9 +15,10 @@ use crate::protocol::body::create::file_attributes::SMBFileAttributes; use crate::protocol::body::create::file_id::SMBFileId; use crate::protocol::body::create::{SMBCreateRequest, SMBCreateResponse}; use crate::protocol::body::file_info::{ - FileAccessInformation, FileAlignmentInformation, FileAllInformation, FileBasicInformation, - FileEaInformation, FileInternalInformation, FileModeInformation, FileNameInformation, - FileNetworkOpenInformation, FilePositionInformation, FileStandardInformation, + FileAccessFlags, FileAccessInformation, FileAlignmentInformation, FileAlignmentRequirement, + FileAllInformation, FileBasicInformation, FileEaInformation, FileInternalInformation, + FileModeFlags, FileModeInformation, FileNameInformation, FileNetworkOpenInformation, + FilePositionInformation, FileStandardInformation, }; use crate::protocol::body::filetime::FileTime; use crate::protocol::body::query_info::info_type::SMBInfoType; @@ -140,14 +141,16 @@ impl SMBTreeConnect { internal: FileInternalInformation { index_number: 0 }, ea: FileEaInformation { ea_size: 0 }, access: FileAccessInformation { - access_flags: 0x001f01ff, + access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), }, position: FilePositionInformation { current_byte_offset: 0, }, - mode: FileModeInformation { mode: 0 }, + mode: FileModeInformation { + mode: FileModeFlags::empty(), + }, alignment: FileAlignmentInformation { - alignment_requirement: 0, + alignment_requirement: FileAlignmentRequirement::Byte, }, name: FileNameInformation { file_name_length: name_byte_len, From 73e40d119744c846bf39f2477c22dc36af17beb2 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sat, 4 Apr 2026 02:47:58 -0400 Subject: [PATCH 08/15] fix: address security bugs and encapsulate struct fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security & correctness fixes: - Fix path traversal vulnerability: normalize_path() rejects "../" escaping the share root (STATUS_ACCESS_DENIED) - Cap read_data buffer to 8 MB (MAX_READ_SIZE) to prevent OOM from malicious clients per MS-SMB2 §3.3.5.12 - Return STATUS_END_OF_FILE when read returns 0 bytes at/past EOF per MS-SMB2 §3.3.5.12 - Enforce OutputBufferLength in handle_query_info: truncate and return STATUS_BUFFER_OVERFLOW per MS-SMB2 §3.3.5.20.1 - Add NTStatus::BufferOverflow (0x80000005) and InfoLengthMismatch (0xC0000004) Encapsulation: - Make SMBFileId fields private with new(), persistent(), volatile() - Make SMBFileMetadata fields private with new() and getters - Make all file_info struct fields private with constructors and getters - FileStandardInformation: accept bool for delete_pending/directory Tests: 95 unit tests pass (15 new: normalize_path, bool getters, FileAllInformation getters). Co-Authored-By: Claude Opus 4.6 (1M context) --- smb-core/src/nt_status.rs | 2 + smb/src/protocol/body/close/mod.rs | 53 ++- smb/src/protocol/body/create/file_id.rs | 53 ++- smb/src/protocol/body/create/mod.rs | 12 +- smb/src/protocol/body/file_info/access.rs | 12 +- smb/src/protocol/body/file_info/alignment.rs | 14 +- smb/src/protocol/body/file_info/basic.rs | 47 ++- smb/src/protocol/body/file_info/ea.rs | 12 +- smb/src/protocol/body/file_info/internal.rs | 12 +- smb/src/protocol/body/file_info/mod.rs | 377 +++++++++--------- smb/src/protocol/body/file_info/mode.rs | 12 +- smb/src/protocol/body/file_info/name.rs | 20 +- .../protocol/body/file_info/network_open.rs | 61 ++- smb/src/protocol/body/file_info/position.rs | 14 +- smb/src/protocol/body/file_info/standard.rs | 49 ++- smb/src/protocol/body/query_info/mod.rs | 4 +- smb/src/protocol/body/read/mod.rs | 4 +- smb/src/server/open.rs | 5 +- smb/src/server/share/file_system.rs | 116 +++++- smb/src/server/share/ipc.rs | 16 +- smb/src/server/share/mod.rs | 56 ++- smb/src/server/tree_connect.rs | 125 +++--- 22 files changed, 714 insertions(+), 362 deletions(-) diff --git a/smb-core/src/nt_status.rs b/smb-core/src/nt_status.rs index 86a8fdc..8d4ab0f 100644 --- a/smb-core/src/nt_status.rs +++ b/smb-core/src/nt_status.rs @@ -23,6 +23,8 @@ pub enum NTStatus { EndOfFile = 0xC0000011, InvalidInfoClass = 0xC0000003, InvalidDeviceRequest = 0xC0000010, + BufferOverflow = 0x80000005, + InfoLengthMismatch = 0xC0000004, UnknownError = 0xFFFFFFFF, } diff --git a/smb/src/protocol/body/close/mod.rs b/smb/src/protocol/body/close/mod.rs index 5d4d34a..b99ff93 100644 --- a/smb/src/protocol/body/close/mod.rs +++ b/smb/src/protocol/body/close/mod.rs @@ -67,12 +67,12 @@ impl SMBCloseResponse { Self { flags: SMBCloseFlags::POSTQUERY_ATTRIB, reserved: PhantomData, - creation_time: metadata.creation_time.clone(), - last_access_time: metadata.last_access_time.clone(), - last_write_time: metadata.last_write_time.clone(), - change_time: metadata.last_modification_time.clone(), - allocation_size: metadata.allocated_size, - end_of_file: metadata.actual_size, + creation_time: metadata.creation_time().clone(), + last_access_time: metadata.last_access_time().clone(), + last_write_time: metadata.last_write_time().clone(), + change_time: metadata.last_modification_time().clone(), + allocation_size: metadata.allocated_size(), + end_of_file: metadata.actual_size(), file_attributes: attributes, } } @@ -118,14 +118,14 @@ mod tests { #[test] fn close_response_from_metadata_sets_postquery_flag() { use crate::server::share::SMBFileMetadata; - let metadata = SMBFileMetadata { - creation_time: FileTime::from_unix(1700000000), - last_access_time: FileTime::from_unix(1700000100), - last_write_time: FileTime::from_unix(1700000200), - last_modification_time: FileTime::from_unix(1700000300), - allocated_size: 4096, - actual_size: 1024, - }; + let metadata = SMBFileMetadata::new( + FileTime::from_unix(1700000000), + FileTime::from_unix(1700000100), + FileTime::from_unix(1700000200), + FileTime::from_unix(1700000300), + 4096, + 1024, + ); let resp = SMBCloseResponse::from_metadata(&metadata, SMBFileAttributes::NORMAL); assert!(resp.flags.contains(SMBCloseFlags::POSTQUERY_ATTRIB)); assert_eq!(resp.allocation_size, 4096); @@ -136,14 +136,14 @@ mod tests { #[test] fn close_response_from_metadata_serialization_round_trip() { use crate::server::share::SMBFileMetadata; - let metadata = SMBFileMetadata { - creation_time: FileTime::from_unix(1700000000), - last_access_time: FileTime::from_unix(1700000100), - last_write_time: FileTime::from_unix(1700000200), - last_modification_time: FileTime::from_unix(1700000300), - allocated_size: 8192, - actual_size: 2048, - }; + let metadata = SMBFileMetadata::new( + FileTime::from_unix(1700000000), + FileTime::from_unix(1700000100), + FileTime::from_unix(1700000200), + FileTime::from_unix(1700000300), + 8192, + 2048, + ); let resp = SMBCloseResponse::from_metadata(&metadata, SMBFileAttributes::ARCHIVE); let bytes = resp.smb_to_bytes(); assert_eq!(bytes.len(), resp.smb_byte_size()); @@ -153,10 +153,7 @@ mod tests { #[test] fn close_request_accessors() { - let file_id = SMBFileId { - persistent: 42, - volatile: 99, - }; + let file_id = SMBFileId::new(42, 99); let bytes = { let mut buf = Vec::new(); // struct_size (u16) = 24 @@ -171,8 +168,8 @@ mod tests { buf }; let (_, req) = SMBCloseRequest::smb_from_bytes(&bytes).unwrap(); - assert_eq!(req.file_id().persistent, file_id.persistent); - assert_eq!(req.file_id().volatile, file_id.volatile); + assert_eq!(req.file_id().persistent(), file_id.persistent()); + assert_eq!(req.file_id().volatile(), file_id.volatile()); assert!(req.flags().contains(SMBCloseFlags::POSTQUERY_ATTRIB)); } } diff --git a/smb/src/protocol/body/create/file_id.rs b/smb/src/protocol/body/create/file_id.rs index f3cb795..8c83b33 100644 --- a/smb/src/protocol/body/create/file_id.rs +++ b/smb/src/protocol/body/create/file_id.rs @@ -7,9 +7,26 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; )] pub struct SMBFileId { #[smb_direct(start(fixed = 0))] - pub persistent: u64, + persistent: u64, #[smb_direct(start(fixed = 8))] - pub volatile: u64, + volatile: u64, +} + +impl SMBFileId { + pub fn new(persistent: u64, volatile: u64) -> Self { + Self { + persistent, + volatile, + } + } + + pub fn persistent(&self) -> u64 { + self.persistent + } + + pub fn volatile(&self) -> u64 { + self.volatile + } } #[cfg(test)] @@ -20,20 +37,14 @@ mod tests { /// MS-SMB2 section 2.2.14.1: SMB2_FILEID is 16 bytes (Persistent u64 + Volatile u64) #[test] fn file_id_is_16_bytes() { - let fid = SMBFileId { - persistent: 0, - volatile: 0, - }; + let fid = SMBFileId::new(0, 0); assert_eq!(fid.smb_byte_size(), 16); } /// Persistent is at offset 0, Volatile at offset 8 #[test] fn file_id_wire_layout() { - let fid = SMBFileId { - persistent: 0xDEAD, - volatile: 0xBEEF, - }; + let fid = SMBFileId::new(0xDEAD, 0xBEEF); let bytes = fid.smb_to_bytes(); assert_eq!(bytes.len(), 16); let persistent = u64::from_le_bytes(bytes[0..8].try_into().unwrap()); @@ -44,10 +55,7 @@ mod tests { #[test] fn file_id_round_trip() { - let fid = SMBFileId { - persistent: 42, - volatile: 99, - }; + let fid = SMBFileId::new(42, 99); let bytes = fid.smb_to_bytes(); let (_, parsed) = SMBFileId::smb_from_bytes(&bytes).unwrap(); assert_eq!(fid, parsed); @@ -57,16 +65,17 @@ mod tests { /// Verify they serialize independently. #[test] fn file_id_persistent_and_volatile_are_independent() { - let a = SMBFileId { - persistent: 1, - volatile: 2, - }; - let b = SMBFileId { - persistent: 2, - volatile: 1, - }; + let a = SMBFileId::new(1, 2); + let b = SMBFileId::new(2, 1); let bytes_a = a.smb_to_bytes(); let bytes_b = b.smb_to_bytes(); assert_ne!(bytes_a, bytes_b); } + + #[test] + fn file_id_getters() { + let fid = SMBFileId::new(100, 200); + assert_eq!(fid.persistent(), 100); + assert_eq!(fid.volatile(), 200); + } } diff --git a/smb/src/protocol/body/create/mod.rs b/smb/src/protocol/body/create/mod.rs index d868a70..80cf1ae 100644 --- a/smb/src/protocol/body/create/mod.rs +++ b/smb/src/protocol/body/create/mod.rs @@ -179,12 +179,12 @@ impl SMBCreateResponse { oplock_level: open.oplock_level(), flags: SMBCreateFlags::empty(), action: SMBCreateAction::Created, - creation_time: metadata.creation_time, - last_access_time: metadata.last_access_time, - last_write_time: metadata.last_write_time, - change_time: metadata.last_modification_time, - allocation_size: metadata.allocated_size, - end_of_file: metadata.actual_size, + creation_time: metadata.creation_time().clone(), + last_access_time: metadata.last_access_time().clone(), + last_write_time: metadata.last_write_time().clone(), + change_time: metadata.last_modification_time().clone(), + allocation_size: metadata.allocated_size(), + end_of_file: metadata.actual_size(), attributes: open.file_attributes(), reserved: PhantomData, file_id: open.file_id(), diff --git a/smb/src/protocol/body/file_info/access.rs b/smb/src/protocol/body/file_info/access.rs index c3ec191..e28aa46 100644 --- a/smb/src/protocol/body/file_info/access.rs +++ b/smb/src/protocol/body/file_info/access.rs @@ -47,5 +47,15 @@ impl_smb_from_bytes_for_bitflag! { FileAccessFlags } )] pub struct FileAccessInformation { #[smb_direct(start(fixed = 0))] - pub access_flags: FileAccessFlags, + access_flags: FileAccessFlags, +} + +impl FileAccessInformation { + pub fn new(access_flags: FileAccessFlags) -> Self { + Self { access_flags } + } + + pub fn access_flags(&self) -> FileAccessFlags { + self.access_flags + } } diff --git a/smb/src/protocol/body/file_info/alignment.rs b/smb/src/protocol/body/file_info/alignment.rs index adbb355..f036d65 100644 --- a/smb/src/protocol/body/file_info/alignment.rs +++ b/smb/src/protocol/body/file_info/alignment.rs @@ -66,5 +66,17 @@ impl SMBToBytes for FileAlignmentRequirement { )] pub struct FileAlignmentInformation { #[smb_direct(start(fixed = 0))] - pub alignment_requirement: FileAlignmentRequirement, + alignment_requirement: FileAlignmentRequirement, +} + +impl FileAlignmentInformation { + pub fn new(alignment_requirement: FileAlignmentRequirement) -> Self { + Self { + alignment_requirement, + } + } + + pub fn alignment_requirement(&self) -> FileAlignmentRequirement { + self.alignment_requirement + } } diff --git a/smb/src/protocol/body/file_info/basic.rs b/smb/src/protocol/body/file_info/basic.rs index 3451adb..bda566f 100644 --- a/smb/src/protocol/body/file_info/basic.rs +++ b/smb/src/protocol/body/file_info/basic.rs @@ -11,15 +11,50 @@ use crate::protocol::body::filetime::FileTime; )] pub struct FileBasicInformation { #[smb_direct(start(fixed = 0))] - pub creation_time: FileTime, + creation_time: FileTime, #[smb_direct(start(fixed = 8))] - pub last_access_time: FileTime, + last_access_time: FileTime, #[smb_direct(start(fixed = 16))] - pub last_write_time: FileTime, + last_write_time: FileTime, #[smb_direct(start(fixed = 24))] - pub change_time: FileTime, + change_time: FileTime, #[smb_direct(start(fixed = 32))] - pub file_attributes: SMBFileAttributes, + file_attributes: SMBFileAttributes, #[smb_direct(start(fixed = 36))] - pub reserved: u32, + reserved: u32, +} + +impl FileBasicInformation { + pub fn new( + creation_time: FileTime, + last_access_time: FileTime, + last_write_time: FileTime, + change_time: FileTime, + file_attributes: SMBFileAttributes, + ) -> Self { + Self { + creation_time, + last_access_time, + last_write_time, + change_time, + file_attributes, + reserved: 0, + } + } + + pub fn creation_time(&self) -> &FileTime { + &self.creation_time + } + pub fn last_access_time(&self) -> &FileTime { + &self.last_access_time + } + pub fn last_write_time(&self) -> &FileTime { + &self.last_write_time + } + pub fn change_time(&self) -> &FileTime { + &self.change_time + } + pub fn file_attributes(&self) -> SMBFileAttributes { + self.file_attributes + } } diff --git a/smb/src/protocol/body/file_info/ea.rs b/smb/src/protocol/body/file_info/ea.rs index 8372ca7..511c96d 100644 --- a/smb/src/protocol/body/file_info/ea.rs +++ b/smb/src/protocol/body/file_info/ea.rs @@ -8,5 +8,15 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; )] pub struct FileEaInformation { #[smb_direct(start(fixed = 0))] - pub ea_size: u32, + ea_size: u32, +} + +impl FileEaInformation { + pub fn new(ea_size: u32) -> Self { + Self { ea_size } + } + + pub fn ea_size(&self) -> u32 { + self.ea_size + } } diff --git a/smb/src/protocol/body/file_info/internal.rs b/smb/src/protocol/body/file_info/internal.rs index 723fb4d..2316014 100644 --- a/smb/src/protocol/body/file_info/internal.rs +++ b/smb/src/protocol/body/file_info/internal.rs @@ -8,5 +8,15 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; )] pub struct FileInternalInformation { #[smb_direct(start(fixed = 0))] - pub index_number: u64, + index_number: u64, +} + +impl FileInternalInformation { + pub fn new(index_number: u64) -> Self { + Self { index_number } + } + + pub fn index_number(&self) -> u64 { + self.index_number + } } diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index ee2684f..bdb1c26 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -39,23 +39,77 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; )] pub struct FileAllInformation { #[smb_direct(start(fixed = 0))] - pub basic: FileBasicInformation, + basic: FileBasicInformation, #[smb_direct(start(fixed = 40))] - pub standard: FileStandardInformation, + standard: FileStandardInformation, #[smb_direct(start(fixed = 64))] - pub internal: FileInternalInformation, + internal: FileInternalInformation, #[smb_direct(start(fixed = 72))] - pub ea: FileEaInformation, + ea: FileEaInformation, #[smb_direct(start(fixed = 76))] - pub access: FileAccessInformation, + access: FileAccessInformation, #[smb_direct(start(fixed = 80))] - pub position: FilePositionInformation, + position: FilePositionInformation, #[smb_direct(start(fixed = 88))] - pub mode: FileModeInformation, + mode: FileModeInformation, #[smb_direct(start(fixed = 92))] - pub alignment: FileAlignmentInformation, + alignment: FileAlignmentInformation, #[smb_direct(start(fixed = 96))] - pub name: FileNameInformation, + name: FileNameInformation, +} + +impl FileAllInformation { + pub fn new( + basic: FileBasicInformation, + standard: FileStandardInformation, + internal: FileInternalInformation, + ea: FileEaInformation, + access: FileAccessInformation, + position: FilePositionInformation, + mode: FileModeInformation, + alignment: FileAlignmentInformation, + name: FileNameInformation, + ) -> Self { + Self { + basic, + standard, + internal, + ea, + access, + position, + mode, + alignment, + name, + } + } + + pub fn basic(&self) -> &FileBasicInformation { + &self.basic + } + pub fn standard(&self) -> &FileStandardInformation { + &self.standard + } + pub fn internal(&self) -> &FileInternalInformation { + &self.internal + } + pub fn ea(&self) -> &FileEaInformation { + &self.ea + } + pub fn access(&self) -> &FileAccessInformation { + &self.access + } + pub fn position(&self) -> &FilePositionInformation { + &self.position + } + pub fn mode(&self) -> &FileModeInformation { + &self.mode + } + pub fn alignment(&self) -> &FileAlignmentInformation { + &self.alignment + } + pub fn name(&self) -> &FileNameInformation { + &self.name + } } #[cfg(test)] @@ -67,27 +121,25 @@ mod tests { #[test] fn file_basic_information_size_is_40() { - let info = FileBasicInformation { - creation_time: FileTime::zero(), - last_access_time: FileTime::zero(), - last_write_time: FileTime::zero(), - change_time: FileTime::zero(), - file_attributes: SMBFileAttributes::NORMAL, - reserved: 0, - }; + let info = FileBasicInformation::new( + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + SMBFileAttributes::NORMAL, + ); assert_eq!(info.smb_byte_size(), 40); } #[test] fn file_basic_information_round_trip() { - let info = FileBasicInformation { - creation_time: FileTime::now(), - last_access_time: FileTime::now(), - last_write_time: FileTime::now(), - change_time: FileTime::now(), - file_attributes: SMBFileAttributes::ARCHIVE | SMBFileAttributes::READONLY, - reserved: 0, - }; + let info = FileBasicInformation::new( + FileTime::now(), + FileTime::now(), + FileTime::now(), + FileTime::now(), + SMBFileAttributes::ARCHIVE | SMBFileAttributes::READONLY, + ); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 40); let (_, parsed) = FileBasicInformation::smb_from_bytes(&bytes).unwrap(); @@ -96,36 +148,33 @@ mod tests { #[test] fn file_standard_information_size_is_24() { - let info = FileStandardInformation { - allocation_size: 4096, - end_of_file: 1024, - number_of_links: 1, - delete_pending: 0, - directory: 0, - reserved: 0, - }; + let info = FileStandardInformation::new(4096, 1024, 1, false, false); assert_eq!(info.smb_byte_size(), 24); } #[test] fn file_standard_information_round_trip() { - let info = FileStandardInformation { - allocation_size: 8192, - end_of_file: 2048, - number_of_links: 3, - delete_pending: 1, - directory: 0, - reserved: 0, - }; + let info = FileStandardInformation::new(8192, 2048, 3, true, false); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 24); let (_, parsed) = FileStandardInformation::smb_from_bytes(&bytes).unwrap(); assert_eq!(info, parsed); } + #[test] + fn file_standard_information_bool_getters() { + let info = FileStandardInformation::new(0, 0, 1, true, false); + assert!(info.delete_pending()); + assert!(!info.directory()); + + let info2 = FileStandardInformation::new(0, 0, 1, false, true); + assert!(!info2.delete_pending()); + assert!(info2.directory()); + } + #[test] fn file_internal_information_round_trip() { - let info = FileInternalInformation { index_number: 42 }; + let info = FileInternalInformation::new(42); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 8); let (_, parsed) = FileInternalInformation::smb_from_bytes(&bytes).unwrap(); @@ -134,7 +183,7 @@ mod tests { #[test] fn file_ea_information_round_trip() { - let info = FileEaInformation { ea_size: 0 }; + let info = FileEaInformation::new(0); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); let (_, parsed) = FileEaInformation::smb_from_bytes(&bytes).unwrap(); @@ -143,9 +192,7 @@ mod tests { #[test] fn file_access_information_round_trip() { - let info = FileAccessInformation { - access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), - }; + let info = FileAccessInformation::new(FileAccessFlags::from_bits_truncate(0x001f01ff)); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); let (_, parsed) = FileAccessInformation::smb_from_bytes(&bytes).unwrap(); @@ -154,9 +201,7 @@ mod tests { #[test] fn file_position_information_round_trip() { - let info = FilePositionInformation { - current_byte_offset: 512, - }; + let info = FilePositionInformation::new(512); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 8); let (_, parsed) = FilePositionInformation::smb_from_bytes(&bytes).unwrap(); @@ -165,9 +210,7 @@ mod tests { #[test] fn file_mode_information_round_trip() { - let info = FileModeInformation { - mode: FileModeFlags::empty(), - }; + let info = FileModeInformation::new(FileModeFlags::empty()); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); let (_, parsed) = FileModeInformation::smb_from_bytes(&bytes).unwrap(); @@ -176,9 +219,7 @@ mod tests { #[test] fn file_alignment_information_round_trip() { - let info = FileAlignmentInformation { - alignment_requirement: FileAlignmentRequirement::Byte, - }; + let info = FileAlignmentInformation::new(FileAlignmentRequirement::Byte); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 4); let (_, parsed) = FileAlignmentInformation::smb_from_bytes(&bytes).unwrap(); @@ -187,31 +228,29 @@ mod tests { #[test] fn file_network_open_information_size_is_56() { - let info = FileNetworkOpenInformation { - creation_time: FileTime::zero(), - last_access_time: FileTime::zero(), - last_write_time: FileTime::zero(), - change_time: FileTime::zero(), - allocation_size: 0, - end_of_file: 0, - file_attributes: SMBFileAttributes::NORMAL, - reserved: 0, - }; + let info = FileNetworkOpenInformation::new( + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + 0, + 0, + SMBFileAttributes::NORMAL, + ); assert_eq!(info.smb_byte_size(), 56); } #[test] fn file_network_open_information_round_trip() { - let info = FileNetworkOpenInformation { - creation_time: FileTime::now(), - last_access_time: FileTime::now(), - last_write_time: FileTime::now(), - change_time: FileTime::now(), - allocation_size: 4096, - end_of_file: 1024, - file_attributes: SMBFileAttributes::ARCHIVE, - reserved: 0, - }; + let info = FileNetworkOpenInformation::new( + FileTime::now(), + FileTime::now(), + FileTime::now(), + FileTime::now(), + 4096, + 1024, + SMBFileAttributes::ARCHIVE, + ); let bytes = info.smb_to_bytes(); assert_eq!(bytes.len(), 56); let (_, parsed) = FileNetworkOpenInformation::smb_from_bytes(&bytes).unwrap(); @@ -220,42 +259,23 @@ mod tests { #[test] fn file_all_information_contains_all_sub_structs() { - let all = FileAllInformation { - basic: FileBasicInformation { - creation_time: FileTime::zero(), - last_access_time: FileTime::zero(), - last_write_time: FileTime::zero(), - change_time: FileTime::zero(), - file_attributes: SMBFileAttributes::NORMAL, - reserved: 0, - }, - standard: FileStandardInformation { - allocation_size: 4096, - end_of_file: 21, - number_of_links: 1, - delete_pending: 0, - directory: 0, - reserved: 0, - }, - internal: FileInternalInformation { index_number: 0 }, - ea: FileEaInformation { ea_size: 0 }, - access: FileAccessInformation { - access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), - }, - position: FilePositionInformation { - current_byte_offset: 0, - }, - mode: FileModeInformation { - mode: FileModeFlags::empty(), - }, - alignment: FileAlignmentInformation { - alignment_requirement: FileAlignmentRequirement::Byte, - }, - name: FileNameInformation { - file_name_length: 24, - file_name: "testfile.txt".into(), - }, - }; + let all = FileAllInformation::new( + FileBasicInformation::new( + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + SMBFileAttributes::NORMAL, + ), + FileStandardInformation::new(4096, 21, 1, false, false), + FileInternalInformation::new(0), + FileEaInformation::new(0), + FileAccessInformation::new(FileAccessFlags::from_bits_truncate(0x001f01ff)), + FilePositionInformation::new(0), + FileModeInformation::new(FileModeFlags::empty()), + FileAlignmentInformation::new(FileAlignmentRequirement::Byte), + FileNameInformation::new(24, "testfile.txt".into()), + ); let bytes = all.smb_to_bytes(); // 40 + 24 + 8 + 4 + 4 + 8 + 4 + 4 + (4 + 24) = 124 assert_eq!(bytes.len(), 124); @@ -263,43 +283,24 @@ mod tests { #[test] fn file_all_information_basic_segment_matches_standalone() { - let basic = FileBasicInformation { - creation_time: FileTime::now(), - last_access_time: FileTime::now(), - last_write_time: FileTime::now(), - change_time: FileTime::now(), - file_attributes: SMBFileAttributes::ARCHIVE, - reserved: 0, - }; - let all = FileAllInformation { - basic: basic.clone(), - standard: FileStandardInformation { - allocation_size: 0, - end_of_file: 0, - number_of_links: 1, - delete_pending: 0, - directory: 0, - reserved: 0, - }, - internal: FileInternalInformation { index_number: 0 }, - ea: FileEaInformation { ea_size: 0 }, - access: FileAccessInformation { - access_flags: FileAccessFlags::empty(), - }, - position: FilePositionInformation { - current_byte_offset: 0, - }, - mode: FileModeInformation { - mode: FileModeFlags::empty(), - }, - alignment: FileAlignmentInformation { - alignment_requirement: FileAlignmentRequirement::Byte, - }, - name: FileNameInformation { - file_name_length: 0, - file_name: String::new(), - }, - }; + let basic = FileBasicInformation::new( + FileTime::now(), + FileTime::now(), + FileTime::now(), + FileTime::now(), + SMBFileAttributes::ARCHIVE, + ); + let all = FileAllInformation::new( + basic.clone(), + FileStandardInformation::new(0, 0, 1, false, false), + FileInternalInformation::new(0), + FileEaInformation::new(0), + FileAccessInformation::new(FileAccessFlags::empty()), + FilePositionInformation::new(0), + FileModeInformation::new(FileModeFlags::empty()), + FileAlignmentInformation::new(FileAlignmentRequirement::Byte), + FileNameInformation::new(0, String::new()), + ); let all_bytes = all.smb_to_bytes(); let basic_bytes = basic.smb_to_bytes(); assert_eq!(&all_bytes[..40], &basic_bytes[..]); @@ -307,44 +308,52 @@ mod tests { #[test] fn file_all_information_round_trip() { - let all = FileAllInformation { - basic: FileBasicInformation { - creation_time: FileTime::now(), - last_access_time: FileTime::now(), - last_write_time: FileTime::now(), - change_time: FileTime::now(), - file_attributes: SMBFileAttributes::ARCHIVE, - reserved: 0, - }, - standard: FileStandardInformation { - allocation_size: 4096, - end_of_file: 512, - number_of_links: 1, - delete_pending: 0, - directory: 0, - reserved: 0, - }, - internal: FileInternalInformation { index_number: 7 }, - ea: FileEaInformation { ea_size: 0 }, - access: FileAccessInformation { - access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), - }, - position: FilePositionInformation { - current_byte_offset: 256, - }, - mode: FileModeInformation { - mode: FileModeFlags::empty(), - }, - alignment: FileAlignmentInformation { - alignment_requirement: FileAlignmentRequirement::Byte, - }, - name: FileNameInformation { - file_name_length: 24, - file_name: "testfile.txt".into(), - }, - }; + let all = FileAllInformation::new( + FileBasicInformation::new( + FileTime::now(), + FileTime::now(), + FileTime::now(), + FileTime::now(), + SMBFileAttributes::ARCHIVE, + ), + FileStandardInformation::new(4096, 512, 1, false, false), + FileInternalInformation::new(7), + FileEaInformation::new(0), + FileAccessInformation::new(FileAccessFlags::from_bits_truncate(0x001f01ff)), + FilePositionInformation::new(256), + FileModeInformation::new(FileModeFlags::empty()), + FileAlignmentInformation::new(FileAlignmentRequirement::Byte), + FileNameInformation::new(24, "testfile.txt".into()), + ); let bytes = all.smb_to_bytes(); let (_, parsed) = FileAllInformation::smb_from_bytes(&bytes).unwrap(); assert_eq!(all, parsed); } + + #[test] + fn file_all_information_getters() { + let all = FileAllInformation::new( + FileBasicInformation::new( + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + FileTime::zero(), + SMBFileAttributes::NORMAL, + ), + FileStandardInformation::new(4096, 100, 1, false, false), + FileInternalInformation::new(5), + FileEaInformation::new(0), + FileAccessInformation::new(FileAccessFlags::from_bits_truncate(0x001f01ff)), + FilePositionInformation::new(50), + FileModeInformation::new(FileModeFlags::empty()), + FileAlignmentInformation::new(FileAlignmentRequirement::Byte), + FileNameInformation::new(8, "test".into()), + ); + assert_eq!(all.basic().file_attributes(), SMBFileAttributes::NORMAL); + assert_eq!(all.standard().allocation_size(), 4096); + assert_eq!(all.standard().end_of_file(), 100); + assert_eq!(all.internal().index_number(), 5); + assert_eq!(all.position().current_byte_offset(), 50); + assert_eq!(all.name().file_name(), "test"); + } } diff --git a/smb/src/protocol/body/file_info/mode.rs b/smb/src/protocol/body/file_info/mode.rs index 701e67c..b34715c 100644 --- a/smb/src/protocol/body/file_info/mode.rs +++ b/smb/src/protocol/body/file_info/mode.rs @@ -30,5 +30,15 @@ impl_smb_from_bytes_for_bitflag! { FileModeFlags } )] pub struct FileModeInformation { #[smb_direct(start(fixed = 0))] - pub mode: FileModeFlags, + mode: FileModeFlags, +} + +impl FileModeInformation { + pub fn new(mode: FileModeFlags) -> Self { + Self { mode } + } + + pub fn mode(&self) -> FileModeFlags { + self.mode + } } diff --git a/smb/src/protocol/body/file_info/name.rs b/smb/src/protocol/body/file_info/name.rs index f5eaeb5..9cf1974 100644 --- a/smb/src/protocol/body/file_info/name.rs +++ b/smb/src/protocol/body/file_info/name.rs @@ -8,12 +8,28 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; )] pub struct FileNameInformation { #[smb_direct(start(fixed = 0))] - pub file_name_length: u32, + file_name_length: u32, #[smb_string( order = 0, start(fixed = 4), length(inner(start = 0, num_type = "u32")), underlying = "u16" )] - pub file_name: String, + file_name: String, +} + +impl FileNameInformation { + pub fn new(file_name_length: u32, file_name: String) -> Self { + Self { + file_name_length, + file_name, + } + } + + pub fn file_name_length(&self) -> u32 { + self.file_name_length + } + pub fn file_name(&self) -> &str { + &self.file_name + } } diff --git a/smb/src/protocol/body/file_info/network_open.rs b/smb/src/protocol/body/file_info/network_open.rs index 3d79fbf..862bf0b 100644 --- a/smb/src/protocol/body/file_info/network_open.rs +++ b/smb/src/protocol/body/file_info/network_open.rs @@ -11,19 +11,64 @@ use crate::protocol::body::filetime::FileTime; )] pub struct FileNetworkOpenInformation { #[smb_direct(start(fixed = 0))] - pub creation_time: FileTime, + creation_time: FileTime, #[smb_direct(start(fixed = 8))] - pub last_access_time: FileTime, + last_access_time: FileTime, #[smb_direct(start(fixed = 16))] - pub last_write_time: FileTime, + last_write_time: FileTime, #[smb_direct(start(fixed = 24))] - pub change_time: FileTime, + change_time: FileTime, #[smb_direct(start(fixed = 32))] - pub allocation_size: u64, + allocation_size: u64, #[smb_direct(start(fixed = 40))] - pub end_of_file: u64, + end_of_file: u64, #[smb_direct(start(fixed = 48))] - pub file_attributes: SMBFileAttributes, + file_attributes: SMBFileAttributes, #[smb_direct(start(fixed = 52))] - pub reserved: u32, + reserved: u32, +} + +impl FileNetworkOpenInformation { + pub fn new( + creation_time: FileTime, + last_access_time: FileTime, + last_write_time: FileTime, + change_time: FileTime, + allocation_size: u64, + end_of_file: u64, + file_attributes: SMBFileAttributes, + ) -> Self { + Self { + creation_time, + last_access_time, + last_write_time, + change_time, + allocation_size, + end_of_file, + file_attributes, + reserved: 0, + } + } + + pub fn creation_time(&self) -> &FileTime { + &self.creation_time + } + pub fn last_access_time(&self) -> &FileTime { + &self.last_access_time + } + pub fn last_write_time(&self) -> &FileTime { + &self.last_write_time + } + pub fn change_time(&self) -> &FileTime { + &self.change_time + } + pub fn allocation_size(&self) -> u64 { + self.allocation_size + } + pub fn end_of_file(&self) -> u64 { + self.end_of_file + } + pub fn file_attributes(&self) -> SMBFileAttributes { + self.file_attributes + } } diff --git a/smb/src/protocol/body/file_info/position.rs b/smb/src/protocol/body/file_info/position.rs index d74b3b6..5a9d4ac 100644 --- a/smb/src/protocol/body/file_info/position.rs +++ b/smb/src/protocol/body/file_info/position.rs @@ -8,5 +8,17 @@ use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; )] pub struct FilePositionInformation { #[smb_direct(start(fixed = 0))] - pub current_byte_offset: u64, + current_byte_offset: u64, +} + +impl FilePositionInformation { + pub fn new(current_byte_offset: u64) -> Self { + Self { + current_byte_offset, + } + } + + pub fn current_byte_offset(&self) -> u64 { + self.current_byte_offset + } } diff --git a/smb/src/protocol/body/file_info/standard.rs b/smb/src/protocol/body/file_info/standard.rs index 340aa68..3ca08aa 100644 --- a/smb/src/protocol/body/file_info/standard.rs +++ b/smb/src/protocol/body/file_info/standard.rs @@ -3,20 +3,57 @@ use serde::{Deserialize, Serialize}; use smb_derive::{SMBByteSize, SMBFromBytes, SMBToBytes}; /// FILE_STANDARD_INFORMATION (MS-FSCC 2.4.41) — 24 bytes +/// +/// `delete_pending` and `directory` are booleans on the wire (u8: 0 or 1). #[derive( Debug, PartialEq, Eq, Clone, Serialize, Deserialize, SMBByteSize, SMBFromBytes, SMBToBytes, )] pub struct FileStandardInformation { #[smb_direct(start(fixed = 0))] - pub allocation_size: u64, + allocation_size: u64, #[smb_direct(start(fixed = 8))] - pub end_of_file: u64, + end_of_file: u64, #[smb_direct(start(fixed = 16))] - pub number_of_links: u32, + number_of_links: u32, #[smb_direct(start(fixed = 20))] - pub delete_pending: u8, + delete_pending: u8, #[smb_direct(start(fixed = 21))] - pub directory: u8, + directory: u8, #[smb_direct(start(fixed = 22))] - pub reserved: u16, + reserved: u16, +} + +impl FileStandardInformation { + pub fn new( + allocation_size: u64, + end_of_file: u64, + number_of_links: u32, + delete_pending: bool, + directory: bool, + ) -> Self { + Self { + allocation_size, + end_of_file, + number_of_links, + delete_pending: delete_pending as u8, + directory: directory as u8, + reserved: 0, + } + } + + pub fn allocation_size(&self) -> u64 { + self.allocation_size + } + pub fn end_of_file(&self) -> u64 { + self.end_of_file + } + pub fn number_of_links(&self) -> u32 { + self.number_of_links + } + pub fn delete_pending(&self) -> bool { + self.delete_pending != 0 + } + pub fn directory(&self) -> bool { + self.directory != 0 + } } diff --git a/smb/src/protocol/body/query_info/mod.rs b/smb/src/protocol/body/query_info/mod.rs index 7b8cbd8..ee32008 100644 --- a/smb/src/protocol/body/query_info/mod.rs +++ b/smb/src/protocol/body/query_info/mod.rs @@ -142,7 +142,7 @@ mod tests { assert_eq!(req.info_type(), SMBInfoType::File); assert_eq!(req.file_info_class(), 4); assert_eq!(req.output_buffer_length(), 4096); - assert_eq!(req.file_id().persistent, 55); - assert_eq!(req.file_id().volatile, 77); + assert_eq!(req.file_id().persistent(), 55); + assert_eq!(req.file_id().volatile(), 77); } } diff --git a/smb/src/protocol/body/read/mod.rs b/smb/src/protocol/body/read/mod.rs index 166f40f..b596ee5 100644 --- a/smb/src/protocol/body/read/mod.rs +++ b/smb/src/protocol/body/read/mod.rs @@ -148,7 +148,7 @@ mod tests { assert_eq!(req.read_length(), 1024); assert_eq!(req.read_offset(), 512); assert_eq!(req.minimum_count(), 256); - assert_eq!(req.file_id().persistent, 10); - assert_eq!(req.file_id().volatile, 20); + assert_eq!(req.file_id().persistent(), 10); + assert_eq!(req.file_id().volatile(), 20); } } diff --git a/smb/src/server/open.rs b/smb/src/server/open.rs index 93ba01b..01eec9e 100644 --- a/smb/src/server/open.rs +++ b/smb/src/server/open.rs @@ -141,10 +141,7 @@ impl Open for SMBOpen { } fn file_id(&self) -> SMBFileId { - SMBFileId { - persistent: self.global_id as u64, - volatile: self.session_id as u64, - } + SMBFileId::new(self.global_id as u64, self.session_id as u64) } fn file_metadata(&self) -> SMBResult { diff --git a/smb/src/server/share/file_system.rs b/smb/src/server/share/file_system.rs index 50c2de1..255e973 100644 --- a/smb/src/server/share/file_system.rs +++ b/smb/src/server/share/file_system.rs @@ -4,11 +4,13 @@ use std::fs; use std::fs::{File, OpenOptions, ReadDir}; use std::io::{Read, Seek, SeekFrom}; use std::marker::PhantomData; +use std::path::{Component, Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; use smb_core::SMBResult; use smb_core::error::SMBError; -use smb_core::logging::debug; +use smb_core::logging::{debug, warn}; +use smb_core::nt_status::NTStatus; use crate::protocol::body::create::disposition::SMBCreateDisposition; use crate::protocol::body::filetime::FileTime; @@ -18,6 +20,31 @@ use crate::server::share::{ ConnectAllowed, FilePerms, ResourceHandle, ResourceType, SMBFileMetadata, SharedResource, }; +/// Maximum single read size (8 MB), per MS-SMB2 §3.3.5.12 recommendation for SMB 3.x. +const MAX_READ_SIZE: u32 = 8 * 1024 * 1024; + +/// Normalize a path by resolving `.` and `..` components lexically (without +/// touching the filesystem). Returns `None` if the normalized path would +/// escape the root (i.e., more `..` than preceding components). +fn normalize_path(path: &str) -> Option { + let mut components = Vec::new(); + for component in Path::new(path).components() { + match component { + Component::ParentDir => { + if components.is_empty() { + // Attempting to go above root — reject + return None; + } + components.pop(); + } + Component::Normal(c) => components.push(c), + Component::CurDir => {} // skip "." + Component::RootDir | Component::Prefix(_) => {} // skip absolute prefixes + } + } + Some(components.iter().collect()) +} + #[derive(Debug)] pub struct SMBFileSystemHandle { path: String, @@ -89,28 +116,24 @@ impl ResourceHandle for SMBFileSystemHandle { )) })?; let time_transform = |time: SystemTime| time.duration_since(UNIX_EPOCH).unwrap().as_secs(); - Ok(SMBFileMetadata { - creation_time: FileTime::from_unix(metadata.created().map(time_transform).unwrap_or(0)), - last_access_time: FileTime::from_unix( - metadata.accessed().map(time_transform).unwrap_or(0), - ), - last_write_time: FileTime::from_unix( - metadata.modified().map(time_transform).unwrap_or(0), - ), - last_modification_time: FileTime::from_unix( - metadata.modified().map(time_transform).unwrap_or(0), - ), - allocated_size: metadata.len(), - actual_size: metadata.len(), - }) + Ok(SMBFileMetadata::new( + FileTime::from_unix(metadata.created().map(time_transform).unwrap_or(0)), + FileTime::from_unix(metadata.accessed().map(time_transform).unwrap_or(0)), + FileTime::from_unix(metadata.modified().map(time_transform).unwrap_or(0)), + FileTime::from_unix(metadata.modified().map(time_transform).unwrap_or(0)), + metadata.len(), + metadata.len(), + )) } fn read_data(&mut self, offset: u64, length: u32) -> SMBResult> { match &mut self.resource { SMBFileSystemResourceHandle::File(file) => { + // Cap to MAX_READ_SIZE to prevent OOM from malicious clients + let capped_length = length.min(MAX_READ_SIZE) as usize; file.seek(SeekFrom::Start(offset)) .map_err(SMBError::io_error)?; - let mut buf = vec![0u8; length as usize]; + let mut buf = vec![0u8; capped_length]; let bytes_read = file.read(&mut buf).map_err(SMBError::io_error)?; buf.truncate(bytes_read); Ok(buf) @@ -200,7 +223,14 @@ impl< // Sanitize: strip NUL terminators from UTF-16LE wire encoding, // convert Windows backslashes to forward slashes let sanitized = path.trim_end_matches('\0').replace('\\', "/"); - let path = format!("{}/{}", self.local_path, sanitized); + + // Normalize and reject path traversal attempts (e.g. "../../etc/passwd") + let relative = normalize_path(&sanitized).ok_or_else(|| { + warn!(path = %sanitized, "rejected path traversal attempt"); + SMBError::response_error(NTStatus::AccessDenied) + })?; + let path = format!("{}/{}", self.local_path, relative.display()); + let resource = match directory { true => SMBFileSystemResourceHandle::directory(&path), false => SMBFileSystemResourceHandle::file(&path, disposition), @@ -301,3 +331,55 @@ impl> Debug .finish() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn normalize_path_simple() { + assert_eq!( + normalize_path("foo/bar.txt"), + Some(PathBuf::from("foo/bar.txt")) + ); + } + + #[test] + fn normalize_path_strips_current_dir() { + assert_eq!( + normalize_path("./foo/./bar.txt"), + Some(PathBuf::from("foo/bar.txt")) + ); + } + + #[test] + fn normalize_path_resolves_parent_within_subtree() { + assert_eq!( + normalize_path("foo/bar/../baz.txt"), + Some(PathBuf::from("foo/baz.txt")) + ); + } + + #[test] + fn normalize_path_rejects_traversal_above_root() { + assert_eq!(normalize_path("../etc/passwd"), None); + } + + #[test] + fn normalize_path_rejects_deep_traversal() { + assert_eq!(normalize_path("foo/../../etc/passwd"), None); + } + + #[test] + fn normalize_path_empty() { + assert_eq!(normalize_path(""), Some(PathBuf::from(""))); + } + + #[test] + fn normalize_path_backslash_after_sanitize() { + assert_eq!( + normalize_path("subdir/file.txt"), + Some(PathBuf::from("subdir/file.txt")) + ); + } +} diff --git a/smb/src/server/share/ipc.rs b/smb/src/server/share/ipc.rs index aaaff28..18ff7c1 100644 --- a/smb/src/server/share/ipc.rs +++ b/smb/src/server/share/ipc.rs @@ -36,14 +36,14 @@ impl ResourceHandle for SMBIPCHandle { } fn metadata(&self) -> SMBResult { - Ok(SMBFileMetadata { - creation_time: FileTime::default(), - last_access_time: FileTime::default(), - last_write_time: FileTime::default(), - last_modification_time: FileTime::default(), - allocated_size: 0, - actual_size: 0, - }) + Ok(SMBFileMetadata::new( + FileTime::default(), + FileTime::default(), + FileTime::default(), + FileTime::default(), + 0, + 0, + )) } fn read_data(&mut self, _offset: u64, _length: u32) -> SMBResult> { diff --git a/smb/src/server/share/mod.rs b/smb/src/server/share/mod.rs index 70a1573..181f042 100644 --- a/smb/src/server/share/mod.rs +++ b/smb/src/server/share/mod.rs @@ -28,12 +28,56 @@ pub trait ResourceHandle: Send + Sync { } pub struct SMBFileMetadata { - pub creation_time: FileTime, - pub last_access_time: FileTime, - pub last_write_time: FileTime, - pub last_modification_time: FileTime, - pub allocated_size: u64, - pub actual_size: u64, + creation_time: FileTime, + last_access_time: FileTime, + last_write_time: FileTime, + last_modification_time: FileTime, + allocated_size: u64, + actual_size: u64, +} + +impl SMBFileMetadata { + pub fn new( + creation_time: FileTime, + last_access_time: FileTime, + last_write_time: FileTime, + last_modification_time: FileTime, + allocated_size: u64, + actual_size: u64, + ) -> Self { + Self { + creation_time, + last_access_time, + last_write_time, + last_modification_time, + allocated_size, + actual_size, + } + } + + pub fn creation_time(&self) -> &FileTime { + &self.creation_time + } + + pub fn last_access_time(&self) -> &FileTime { + &self.last_access_time + } + + pub fn last_write_time(&self) -> &FileTime { + &self.last_write_time + } + + pub fn last_modification_time(&self) -> &FileTime { + &self.last_modification_time + } + + pub fn allocated_size(&self) -> u64 { + self.allocated_size + } + + pub fn actual_size(&self) -> u64 { + self.actual_size + } } impl ResourceHandle for Box { diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index debca40..2c0a072 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -79,12 +79,12 @@ impl SMBTreeConnect { let session_rd = session.read().await; let open = session_rd .open_table() - .get(&file_id.volatile) + .get(&file_id.volatile()) .cloned() .ok_or(SMBError::response_error(NTStatus::FileClosed))?; // MS-SMB2 §3.3.5.10/12/20: verify Open.DurableFileId == FileId.Persistent let open_rd = open.read().await; - if open_rd.file_id().persistent != file_id.persistent { + if open_rd.file_id().persistent() != file_id.persistent() { return Err(SMBError::response_error(NTStatus::FileClosed)); } drop(open_rd); @@ -93,14 +93,13 @@ impl SMBTreeConnect { fn build_basic_info(open: &S::Open) -> SMBResult { let metadata = open.file_metadata()?; - Ok(FileBasicInformation { - creation_time: metadata.creation_time, - last_access_time: metadata.last_access_time, - last_write_time: metadata.last_write_time, - change_time: metadata.last_modification_time, - file_attributes: open.file_attributes(), - reserved: 0, - }) + Ok(FileBasicInformation::new( + metadata.creation_time().clone(), + metadata.last_access_time().clone(), + metadata.last_write_time().clone(), + metadata.last_modification_time().clone(), + open.file_attributes(), + )) } fn build_standard_info(open: &S::Open) -> SMBResult { @@ -108,55 +107,42 @@ impl SMBTreeConnect { let is_dir = open .file_attributes() .contains(SMBFileAttributes::DIRECTORY); - Ok(FileStandardInformation { - allocation_size: metadata.allocated_size, - end_of_file: metadata.actual_size, - number_of_links: 1, - delete_pending: 0, - directory: if is_dir { 1 } else { 0 }, - reserved: 0, - }) + Ok(FileStandardInformation::new( + metadata.allocated_size(), + metadata.actual_size(), + 1, + false, + is_dir, + )) } fn build_network_open_info(open: &S::Open) -> SMBResult { let metadata = open.file_metadata()?; - Ok(FileNetworkOpenInformation { - creation_time: metadata.creation_time, - last_access_time: metadata.last_access_time, - last_write_time: metadata.last_write_time, - change_time: metadata.last_modification_time, - allocation_size: metadata.allocated_size, - end_of_file: metadata.actual_size, - file_attributes: open.file_attributes(), - reserved: 0, - }) + Ok(FileNetworkOpenInformation::new( + metadata.creation_time().clone(), + metadata.last_access_time().clone(), + metadata.last_write_time().clone(), + metadata.last_modification_time().clone(), + metadata.allocated_size(), + metadata.actual_size(), + open.file_attributes(), + )) } fn build_all_info(open: &S::Open) -> SMBResult { let name = open.file_name(); let name_byte_len = (name.encode_utf16().count() * 2) as u32; - Ok(FileAllInformation { - basic: Self::build_basic_info(open)?, - standard: Self::build_standard_info(open)?, - internal: FileInternalInformation { index_number: 0 }, - ea: FileEaInformation { ea_size: 0 }, - access: FileAccessInformation { - access_flags: FileAccessFlags::from_bits_truncate(0x001f01ff), - }, - position: FilePositionInformation { - current_byte_offset: 0, - }, - mode: FileModeInformation { - mode: FileModeFlags::empty(), - }, - alignment: FileAlignmentInformation { - alignment_requirement: FileAlignmentRequirement::Byte, - }, - name: FileNameInformation { - file_name_length: name_byte_len, - file_name: name.into(), - }, - }) + Ok(FileAllInformation::new( + Self::build_basic_info(open)?, + Self::build_standard_info(open)?, + FileInternalInformation::new(0), + FileEaInformation::new(0), + FileAccessInformation::new(FileAccessFlags::from_bits_truncate(0x001f01ff)), + FilePositionInformation::new(0), + FileModeInformation::new(FileModeFlags::empty()), + FileAlignmentInformation::new(FileAlignmentRequirement::Byte), + FileNameInformation::new(name_byte_len, name.into()), + )) } } @@ -210,14 +196,14 @@ impl SMBLockedMessageHandlerBase for Arc> { let session_rd = session.read().await; session_rd .open_table() - .get(&message.file_id().volatile) + .get(&message.file_id().volatile()) .cloned() .ok_or(SMBError::response_error(NTStatus::FileClosed))? }; let (response, file_id) = { let open_rd = open.read().await; // MS-SMB2 section 3.3.5.10: verify Open.DurableFileId == FileId.Persistent - if open_rd.file_id().persistent != message.file_id().persistent { + if open_rd.file_id().persistent() != message.file_id().persistent() { return Err(SMBError::response_error(NTStatus::FileClosed)); } let response = if message @@ -236,7 +222,10 @@ impl SMBLockedMessageHandlerBase for Arc> { // Server write first (outermost) — use persistent (global_id) as GlobalOpenTable key if let Ok(conn) = session.upper().await { if let Ok(server) = conn.upper().await { - server.write().await.remove_open(file_id.persistent as u32); + server + .write() + .await + .remove_open(file_id.persistent() as u32); } else { warn!(file_id = ?file_id, "failed to acquire server lock during close; global open table entry leaked"); } @@ -246,7 +235,7 @@ impl SMBLockedMessageHandlerBase for Arc> { // Session write second (inner relative to server) { let mut session_wr = session.write().await; - session_wr.open_table_mut().remove(&file_id.volatile); + session_wr.open_table_mut().remove(&file_id.volatile()); } debug!(file_id = ?file_id, "close completed"); @@ -268,6 +257,11 @@ impl SMBLockedMessageHandlerBase for Arc> { let data = open_wr.read_data(message.read_offset(), message.read_length())?; drop(open_wr); + // MS-SMB2 §3.3.5.12: if read returns 0 bytes at/past EOF, fail with STATUS_END_OF_FILE + if data.is_empty() && message.read_length() > 0 { + return Err(SMBError::response_error(NTStatus::EndOfFile)); + } + if data.len() < message.minimum_count() as usize { return Err(SMBError::response_error(NTStatus::EndOfFile)); } @@ -291,7 +285,7 @@ impl SMBLockedMessageHandlerBase for Arc> { let open = self.find_open(message.file_id()).await?; let open_rd = open.read().await; - let data = match message.info_type() { + let mut data = match message.info_type() { SMBInfoType::File => { // MS-FSCC file information classes match message.file_info_class() { @@ -314,6 +308,27 @@ impl SMBLockedMessageHandlerBase for Arc> { } }; + // MS-SMB2 §3.3.5.20.1: enforce OutputBufferLength — truncate and + // return STATUS_BUFFER_OVERFLOW for variable-length info classes + let max_output = message.output_buffer_length() as usize; + if max_output > 0 && data.len() > max_output { + debug!( + data_len = data.len(), + max_output, "truncating response to output_buffer_length" + ); + data.truncate(max_output); + let response = SMBQueryInfoResponse::new(data); + let header = header.create_response_header( + NTStatus::BufferOverflow as u32, + header.session_id, + header.tree_id, + ); + return Ok(SMBHandlerState::Finished(SMBMessage::new( + header, + SMBBody::QueryInfoResponse(response), + ))); + } + debug!(data_len = data.len(), "query_info completed"); let response = SMBQueryInfoResponse::new(data); let header = header.create_response_header(0, header.session_id, header.tree_id); From 2fa31409ba7d66f8b25e94be8df2f689e1a6a11b Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sat, 4 Apr 2026 20:38:09 -0400 Subject: [PATCH 09/15] fix: resolve clippy warnings from rebase - Add missing SMBError import in ipc.rs - Move doc comments inside bitflags! macros - Remove unused SMBByteSize import - Fix unnecessary cast and needless borrow - Allow too_many_arguments on FileAllInformation::new (matches MS-FSCC spec) Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/src/protocol/body/file_info/access.rs | 8 ++++---- smb/src/protocol/body/file_info/mod.rs | 1 + smb/src/protocol/body/file_info/mode.rs | 2 +- smb/src/server/open.rs | 2 +- smb/src/server/session.rs | 2 +- smb/src/server/share/ipc.rs | 1 + smb/src/server/tree_connect.rs | 2 +- 7 files changed, 10 insertions(+), 8 deletions(-) diff --git a/smb/src/protocol/body/file_info/access.rs b/smb/src/protocol/body/file_info/access.rs index e28aa46..7c64ada 100644 --- a/smb/src/protocol/body/file_info/access.rs +++ b/smb/src/protocol/body/file_info/access.rs @@ -7,11 +7,11 @@ use crate::util::flags_helper::{ impl_smb_byte_size_for_bitflag, impl_smb_from_bytes_for_bitflag, impl_smb_to_bytes_for_bitflag, }; -/// ACCESS_MASK flags for FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1). -/// -/// These are the same ACCESS_MASK values defined in [MS-DTYP] §2.4.3 / -/// [MS-SMB2] §2.2.13.1, representing the access rights granted on the open. bitflags! { + /// ACCESS_MASK flags for FILE_ACCESS_INFORMATION (MS-FSCC 2.4.1). + /// + /// These are the same ACCESS_MASK values defined in [MS-DTYP] §2.4.3 / + /// [MS-SMB2] §2.2.13.1, representing the access rights granted on the open. #[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)] pub struct FileAccessFlags: u32 { const FILE_READ_DATA = 0x00000001; diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index bdb1c26..e395126 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -59,6 +59,7 @@ pub struct FileAllInformation { } impl FileAllInformation { + #[allow(clippy::too_many_arguments)] pub fn new( basic: FileBasicInformation, standard: FileStandardInformation, diff --git a/smb/src/protocol/body/file_info/mode.rs b/smb/src/protocol/body/file_info/mode.rs index b34715c..102002a 100644 --- a/smb/src/protocol/body/file_info/mode.rs +++ b/smb/src/protocol/body/file_info/mode.rs @@ -7,8 +7,8 @@ use crate::util::flags_helper::{ impl_smb_byte_size_for_bitflag, impl_smb_from_bytes_for_bitflag, impl_smb_to_bytes_for_bitflag, }; -/// Mode flags for FILE_MODE_INFORMATION (MS-FSCC 2.4.26). bitflags! { + /// Mode flags for FILE_MODE_INFORMATION (MS-FSCC 2.4.26). #[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)] pub struct FileModeFlags: u32 { const FILE_WRITE_THROUGH = 0x00000002; diff --git a/smb/src/server/open.rs b/smb/src/server/open.rs index 01eec9e..6b38665 100644 --- a/smb/src/server/open.rs +++ b/smb/src/server/open.rs @@ -141,7 +141,7 @@ impl Open for SMBOpen { } fn file_id(&self) -> SMBFileId { - SMBFileId::new(self.global_id as u64, self.session_id as u64) + SMBFileId::new(self.global_id as u64, self.session_id) } fn file_metadata(&self) -> SMBResult { diff --git a/smb/src/server/session.rs b/smb/src/server/session.rs index 889b0b6..7b33d85 100644 --- a/smb/src/server/session.rs +++ b/smb/src/server/session.rs @@ -290,7 +290,7 @@ impl>> SMBLockedMessageHandlerBase share.clone(), response.access_mask().clone(), ); - let header = SMBSyncHeader::create_response_header(&header, 0, self_rd.id(), tree_id); + let header = SMBSyncHeader::create_response_header(header, 0, self_rd.id(), tree_id); drop(self_rd); let mut self_wr = self.write().await; self_wr diff --git a/smb/src/server/share/ipc.rs b/smb/src/server/share/ipc.rs index 18ff7c1..aab44e9 100644 --- a/smb/src/server/share/ipc.rs +++ b/smb/src/server/share/ipc.rs @@ -3,6 +3,7 @@ use std::fmt::{Debug, Formatter}; use std::marker::PhantomData; use smb_core::SMBResult; +use smb_core::error::SMBError; use crate::protocol::body::create::disposition::SMBCreateDisposition; use crate::protocol::body::filetime::FileTime; diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 2c0a072..23a1d82 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -7,7 +7,7 @@ use tokio::sync::RwLock; use smb_core::error::SMBError; use smb_core::logging::{debug, trace, warn}; use smb_core::nt_status::NTStatus; -use smb_core::{SMBByteSize, SMBResult, SMBToBytes}; +use smb_core::{SMBResult, SMBToBytes}; use crate::protocol::body::SMBBody; use crate::protocol::body::close::{SMBCloseRequest, SMBCloseResponse}; From 7ccb0b15ef58633e02106f0bead6448724e81911 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 21:18:22 -0400 Subject: [PATCH 10/15] fix: create response after open registration, handle short reads, and deduplicate close logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build SMBCreateResponse after add_open so file_id reflects assigned global_id/session_id instead of zeroes. Replace single file.read() with take()+read_to_end() to handle OS short reads correctly. Refactor handle_close to reuse find_open() instead of duplicating lookup logic. Use try_into() for u64→u32 global_id conversion instead of silent truncation. Replace FileNameInformation::new with from_name() that auto-computes UTF-16 byte length. Fix pre-existing _e/e typos in error logging. Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/src/protocol/body/file_info/mod.rs | 8 +- smb/src/protocol/body/file_info/name.rs | 43 +++++++++- smb/src/server/connection.rs | 2 +- smb/src/server/mod.rs | 2 +- smb/src/server/share/file_system.rs | 107 +++++++++++++++++++++++- smb/src/server/tree_connect.rs | 46 ++++------ 6 files changed, 169 insertions(+), 39 deletions(-) diff --git a/smb/src/protocol/body/file_info/mod.rs b/smb/src/protocol/body/file_info/mod.rs index e395126..341c217 100644 --- a/smb/src/protocol/body/file_info/mod.rs +++ b/smb/src/protocol/body/file_info/mod.rs @@ -275,7 +275,7 @@ mod tests { FilePositionInformation::new(0), FileModeInformation::new(FileModeFlags::empty()), FileAlignmentInformation::new(FileAlignmentRequirement::Byte), - FileNameInformation::new(24, "testfile.txt".into()), + FileNameInformation::from_name("testfile.txt".into()), ); let bytes = all.smb_to_bytes(); // 40 + 24 + 8 + 4 + 4 + 8 + 4 + 4 + (4 + 24) = 124 @@ -300,7 +300,7 @@ mod tests { FilePositionInformation::new(0), FileModeInformation::new(FileModeFlags::empty()), FileAlignmentInformation::new(FileAlignmentRequirement::Byte), - FileNameInformation::new(0, String::new()), + FileNameInformation::from_name(String::new()), ); let all_bytes = all.smb_to_bytes(); let basic_bytes = basic.smb_to_bytes(); @@ -324,7 +324,7 @@ mod tests { FilePositionInformation::new(256), FileModeInformation::new(FileModeFlags::empty()), FileAlignmentInformation::new(FileAlignmentRequirement::Byte), - FileNameInformation::new(24, "testfile.txt".into()), + FileNameInformation::from_name("testfile.txt".into()), ); let bytes = all.smb_to_bytes(); let (_, parsed) = FileAllInformation::smb_from_bytes(&bytes).unwrap(); @@ -348,7 +348,7 @@ mod tests { FilePositionInformation::new(50), FileModeInformation::new(FileModeFlags::empty()), FileAlignmentInformation::new(FileAlignmentRequirement::Byte), - FileNameInformation::new(8, "test".into()), + FileNameInformation::from_name("test".into()), ); assert_eq!(all.basic().file_attributes(), SMBFileAttributes::NORMAL); assert_eq!(all.standard().allocation_size(), 4096); diff --git a/smb/src/protocol/body/file_info/name.rs b/smb/src/protocol/body/file_info/name.rs index 9cf1974..b81360d 100644 --- a/smb/src/protocol/body/file_info/name.rs +++ b/smb/src/protocol/body/file_info/name.rs @@ -19,7 +19,8 @@ pub struct FileNameInformation { } impl FileNameInformation { - pub fn new(file_name_length: u32, file_name: String) -> Self { + pub fn from_name(file_name: String) -> Self { + let file_name_length = (file_name.encode_utf16().count() * 2) as u32; Self { file_name_length, file_name, @@ -33,3 +34,43 @@ impl FileNameInformation { &self.file_name } } + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBFromBytes, SMBToBytes}; + + #[test] + fn from_name_computes_utf16_byte_length() { + let info = FileNameInformation::from_name("test.txt".into()); + // "test.txt" = 8 UTF-16 code units × 2 bytes = 16 + assert_eq!(info.file_name_length(), 16); + assert_eq!(info.file_name(), "test.txt"); + } + + #[test] + fn from_name_empty_string() { + let info = FileNameInformation::from_name(String::new()); + assert_eq!(info.file_name_length(), 0); + assert_eq!(info.file_name(), ""); + } + + #[test] + fn from_name_round_trip() { + let info = FileNameInformation::from_name("hello.doc".into()); + let bytes = info.smb_to_bytes(); + let (_, parsed) = FileNameInformation::smb_from_bytes(&bytes).unwrap(); + assert_eq!(info, parsed); + } + + #[test] + fn from_name_length_matches_wire_size() { + let info = FileNameInformation::from_name("testfile.txt".into()); + let bytes = info.smb_to_bytes(); + // Wire: 4 bytes (length field) + 24 bytes (12 UTF-16 code units) + assert_eq!(bytes.len(), 4 + 24); + // The length field in the first 4 bytes should equal 24 + let wire_length = u32::from_le_bytes(bytes[0..4].try_into().unwrap()); + assert_eq!(wire_length, 24); + } +} diff --git a/smb/src/server/connection.rs b/smb/src/server/connection.rs index 42c1797..f0f7e02 100644 --- a/smb/src/server/connection.rs +++ b/smb/src/server/connection.rs @@ -283,7 +283,7 @@ where debug!(?status, "handler returned response error"); Self::build_error_response(&incoming, status) } - Err(_e) => { + Err(e) => { error!(?e, "non-response error, sending NOT_SUPPORTED"); Self::build_error_response(&incoming, NTStatus::NotSupported) } diff --git a/smb/src/server/mod.rs b/smb/src/server/mod.rs index 35677d9..3969d48 100644 --- a/smb/src/server/mod.rs +++ b/smb/src/server/mod.rs @@ -465,7 +465,7 @@ impl< .await { Ok(()) => debug!("message handler completed"), - Err(_e) => warn!(?e, "message handler exited with error"), + Err(e) => warn!(?e, "message handler exited with error"), } }); } diff --git a/smb/src/server/share/file_system.rs b/smb/src/server/share/file_system.rs index 255e973..39cba33 100644 --- a/smb/src/server/share/file_system.rs +++ b/smb/src/server/share/file_system.rs @@ -130,12 +130,14 @@ impl ResourceHandle for SMBFileSystemHandle { match &mut self.resource { SMBFileSystemResourceHandle::File(file) => { // Cap to MAX_READ_SIZE to prevent OOM from malicious clients - let capped_length = length.min(MAX_READ_SIZE) as usize; + let capped_length = length.min(MAX_READ_SIZE) as u64; file.seek(SeekFrom::Start(offset)) .map_err(SMBError::io_error)?; - let mut buf = vec![0u8; capped_length]; - let bytes_read = file.read(&mut buf).map_err(SMBError::io_error)?; - buf.truncate(bytes_read); + // Use take() + read_to_end() to handle short reads correctly + let mut buf = Vec::with_capacity(capped_length as usize); + file.take(capped_length) + .read_to_end(&mut buf) + .map_err(SMBError::io_error)?; Ok(buf) } SMBFileSystemResourceHandle::Directory(_) => Err(SMBError::response_error( @@ -382,4 +384,101 @@ mod tests { Some(PathBuf::from("subdir/file.txt")) ); } + + #[test] + fn read_data_returns_full_contents() { + let dir = std::env::temp_dir().join("smb_test_read_full"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("read_test.bin"); + let data: Vec = (0..4096).map(|i| (i % 256) as u8).collect(); + std::fs::write(&path, &data).unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + let result = handle.read_data(0, 4096).unwrap(); + assert_eq!( + result.len(), + 4096, + "read_data must return all requested bytes when available" + ); + assert_eq!(result, data); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn read_data_at_offset_returns_remaining() { + let dir = std::env::temp_dir().join("smb_test_read_offset"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("offset_test.bin"); + let data = vec![0xAA; 100]; + std::fs::write(&path, &data).unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + // Read past end of file — should return only remaining bytes + let result = handle.read_data(90, 50).unwrap(); + assert_eq!(result.len(), 10); + + // Read at exact EOF — should return empty + let result = handle.read_data(100, 50).unwrap(); + assert!(result.is_empty()); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn read_data_capped_at_max_read_size() { + let dir = std::env::temp_dir().join("smb_test_read_cap"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("cap_test.bin"); + // Write a small file but request more than MAX_READ_SIZE + let data = vec![0xBB; 64]; + std::fs::write(&path, &data).unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + // Request u32::MAX bytes — should be capped and not OOM + let result = handle.read_data(0, u32::MAX).unwrap(); + assert_eq!(result.len(), 64); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn read_data_directory_returns_error() { + let dir = std::env::temp_dir().join("smb_test_read_dir"); + std::fs::create_dir_all(&dir).unwrap(); + + let mut handle = SMBFileSystemHandle { + path: dir.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::directory(dir.to_str().unwrap()).unwrap(), + }; + + let result = handle.read_data(0, 100); + assert!(result.is_err()); + + std::fs::remove_dir_all(&dir).unwrap(); + } } diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 23a1d82..6f80969 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -7,7 +7,7 @@ use tokio::sync::RwLock; use smb_core::error::SMBError; use smb_core::logging::{debug, trace, warn}; use smb_core::nt_status::NTStatus; -use smb_core::{SMBResult, SMBToBytes}; +use smb_core::{SMBByteSize, SMBResult, SMBToBytes}; use crate::protocol::body::SMBBody; use crate::protocol::body::close::{SMBCloseRequest, SMBCloseResponse}; @@ -130,8 +130,6 @@ impl SMBTreeConnect { } fn build_all_info(open: &S::Open) -> SMBResult { - let name = open.file_name(); - let name_byte_len = (name.encode_utf16().count() * 2) as u32; Ok(FileAllInformation::new( Self::build_basic_info(open)?, Self::build_standard_info(open)?, @@ -141,7 +139,7 @@ impl SMBTreeConnect { FilePositionInformation::new(0), FileModeInformation::new(FileModeFlags::empty()), FileAlignmentInformation::new(FileAlignmentRequirement::Byte), - FileNameInformation::new(name_byte_len, name.into()), + FileNameInformation::from_name(open.file_name().into()), )) } } @@ -161,7 +159,6 @@ impl SMBLockedMessageHandlerBase for Arc> { let (path, disposition, directory) = message.validate(self.share.deref())?; let handle = self.share.handle_create(path, disposition, directory)?; let open_raw = Open::init(handle, message); - let response = SMBBody::CreateResponse(SMBCreateResponse::for_open::(&open_raw)?); let open = Arc::new(RwLock::new(open_raw)); let session = self.get_session()?; // Register with server first (outermost), then session (inner) @@ -170,10 +167,13 @@ impl SMBLockedMessageHandlerBase for Arc> { server.write().await.add_open(open.clone()).await; } session.write().await.add_open(open.clone()).await; - { - let file_id = open.read().await.file_id(); - session.write().await.set_previous_file_id(file_id); - } + // Build response AFTER registration so file_id reflects assigned IDs + let (response, file_id) = { + let open_rd = open.read().await; + let resp = SMBBody::CreateResponse(SMBCreateResponse::for_open::(&*open_rd)?); + (resp, open_rd.file_id()) + }; + session.write().await.set_previous_file_id(file_id); debug!("tree connect create handled"); let header = header.create_response_header(0, header.session_id, header.tree_id); trace!( @@ -190,22 +190,10 @@ impl SMBLockedMessageHandlerBase for Arc> { ) -> SMBResult> { debug!(file_id = ?message.file_id(), "handling close request"); - // Phase 1: Read open data (session_rd → open_rd, outer before inner) - let session = self.get_session()?; - let open = { - let session_rd = session.read().await; - session_rd - .open_table() - .get(&message.file_id().volatile()) - .cloned() - .ok_or(SMBError::response_error(NTStatus::FileClosed))? - }; + // Phase 1: Validate and read open data via shared find_open logic + let open = self.find_open(message.file_id()).await?; let (response, file_id) = { let open_rd = open.read().await; - // MS-SMB2 section 3.3.5.10: verify Open.DurableFileId == FileId.Persistent - if open_rd.file_id().persistent() != message.file_id().persistent() { - return Err(SMBError::response_error(NTStatus::FileClosed)); - } let response = if message .flags() .contains(crate::protocol::body::close::flags::SMBCloseFlags::POSTQUERY_ATTRIB) @@ -219,13 +207,15 @@ impl SMBLockedMessageHandlerBase for Arc> { }; // Phase 2: Cleanup — acquire locks outer to inner (server_wr, then session_wr) - // Server write first (outermost) — use persistent (global_id) as GlobalOpenTable key + let session = self.get_session()?; + let global_id: u32 = file_id + .persistent() + .try_into() + .expect("global_id fits in u32"); + // Server write first (outermost) if let Ok(conn) = session.upper().await { if let Ok(server) = conn.upper().await { - server - .write() - .await - .remove_open(file_id.persistent() as u32); + server.write().await.remove_open(global_id); } else { warn!(file_id = ?file_id, "failed to acquire server lock during close; global open table entry leaked"); } From d19dd863c75600a32f460bf64d243112472112c7 Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 21:36:15 -0400 Subject: [PATCH 11/15] fix(clippy): gate SMBByteSize import and suppress unused variable warnings SMBByteSize is only used inside trace!() which is a no-op without the logging feature. Similarly, error variables in match arms are only consumed by tracing macros. Gate the import with #[cfg(feature = "logging")] and add #[allow(unused_variables)] to affected match arms. Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/src/server/connection.rs | 1 + smb/src/server/mod.rs | 8 +++++--- smb/src/server/tree_connect.rs | 4 +++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/smb/src/server/connection.rs b/smb/src/server/connection.rs index f0f7e02..cebe9ec 100644 --- a/smb/src/server/connection.rs +++ b/smb/src/server/connection.rs @@ -283,6 +283,7 @@ where debug!(?status, "handler returned response error"); Self::build_error_response(&incoming, status) } + #[allow(unused_variables)] Err(e) => { error!(?e, "non-response error, sending NOT_SUPPORTED"); Self::build_error_response(&incoming, NTStatus::NotSupported) diff --git a/smb/src/server/mod.rs b/smb/src/server/mod.rs index 3969d48..d66f874 100644 --- a/smb/src/server/mod.rs +++ b/smb/src/server/mod.rs @@ -457,15 +457,17 @@ impl< tokio::spawn(async move { debug!(client = %name, "starting message handler"); let mut stream = socket.lock().await; - match SMBConnection::start_message_handler::( + #[allow(unused_variables)] + if let Err(e) = SMBConnection::start_message_handler::( &mut stream, wrapped_connection, update_channel, ) .await { - Ok(()) => debug!("message handler completed"), - Err(e) => warn!(?e, "message handler exited with error"), + warn!(?e, "message handler exited with error"); + } else { + debug!("message handler completed"); } }); } diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index 6f80969..f2d45d2 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -4,10 +4,12 @@ use std::sync::{Arc, Weak}; use tokio::sync::RwLock; +#[cfg(feature = "logging")] +use smb_core::SMBByteSize; use smb_core::error::SMBError; use smb_core::logging::{debug, trace, warn}; use smb_core::nt_status::NTStatus; -use smb_core::{SMBByteSize, SMBResult, SMBToBytes}; +use smb_core::{SMBResult, SMBToBytes}; use crate::protocol::body::SMBBody; use crate::protocol::body::close::{SMBCloseRequest, SMBCloseResponse}; From 96adf53903fa04a7bcd5aefe614f0a7b046bf1ee Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sat, 4 Apr 2026 22:58:09 -0400 Subject: [PATCH 12/15] feat: implement Write handler for E2E file write - Add accessor methods to SMBWriteRequest (file_id, write_offset, etc.) - Add SMBWriteResponse::new() constructor - Add write_data() to ResourceHandle trait, Open trait, and all impls - Implement handle_write in tree_connect handler following read pattern - Cap write size to 8MB (MAX_WRITE_SIZE) matching read behavior - Add tests for write request/response serialization Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/src/protocol/body/write/mod.rs | 92 +++++++++++++++++++++++++++++ smb/src/server/open.rs | 5 ++ smb/src/server/share/file_system.rs | 18 +++++- smb/src/server/share/ipc.rs | 6 ++ smb/src/server/share/mod.rs | 5 ++ smb/src/server/tree_connect.rs | 21 +++++++ 6 files changed, 146 insertions(+), 1 deletion(-) diff --git a/smb/src/protocol/body/write/mod.rs b/smb/src/protocol/body/write/mod.rs index f4ff936..b560a6c 100644 --- a/smb/src/protocol/body/write/mod.rs +++ b/smb/src/protocol/body/write/mod.rs @@ -39,6 +39,24 @@ pub struct SMBWriteRequest { data_to_write: Vec, } +impl SMBWriteRequest { + pub fn file_id(&self) -> &SMBFileId { + &self.file_id + } + + pub fn write_offset(&self) -> u64 { + self.write_offset + } + + pub fn write_length(&self) -> u32 { + self.write_length + } + + pub fn data_to_write(&self) -> &[u8] { + &self.data_to_write + } +} + #[derive( Debug, PartialEq, Eq, SMBByteSize, SMBToBytes, SMBFromBytes, Serialize, Deserialize, Clone, )] @@ -55,3 +73,77 @@ pub struct SMBWriteResponse { #[smb_skip(start = 14, length = 2)] write_channel_info_len: PhantomData>, } + +impl SMBWriteResponse { + pub fn new(bytes_written: u32) -> Self { + Self { + reserved: PhantomData, + bytes_written, + remaining_bytes: PhantomData, + write_channel_info_offset: PhantomData, + write_channel_info_len: PhantomData, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use smb_core::{SMBByteSize, SMBFromBytes, SMBToBytes}; + + #[test] + fn write_response_new_sets_bytes_written() { + let resp = SMBWriteResponse::new(4096); + assert_eq!(resp.bytes_written, 4096); + } + + #[test] + fn write_response_serialization_round_trip() { + let resp = SMBWriteResponse::new(512); + let bytes = resp.smb_to_bytes(); + assert_eq!(bytes.len(), resp.smb_byte_size()); + let (_, parsed) = SMBWriteResponse::smb_from_bytes(&bytes).unwrap(); + assert_eq!(resp, parsed); + } + + #[test] + fn write_request_accessors() { + let bytes = { + let mut buf = Vec::new(); + // struct_size (u16) = 49 + buf.extend_from_slice(&49u16.to_le_bytes()); + // data_offset (u16) — points past header (offset 2) + let data_offset: u16 = 64 + 49; // header + struct + buf.extend_from_slice(&data_offset.to_le_bytes()); + // write_length (u32) = 100 (offset 4) + buf.extend_from_slice(&100u32.to_le_bytes()); + // write_offset (u64) = 200 (offset 8) + buf.extend_from_slice(&200u64.to_le_bytes()); + // file_id: persistent (u64) + volatile (u64) (offset 16) + buf.extend_from_slice(&5u64.to_le_bytes()); + buf.extend_from_slice(&15u64.to_le_bytes()); + // channel (u32) = 0 (offset 32..36 — but channel is at 36 per struct) + // remaining_bytes (u32) = 0 (offset 36) + buf.extend_from_slice(&0u32.to_le_bytes()); + buf.extend_from_slice(&0u32.to_le_bytes()); + // channel_info_offset (u16) = 0, channel_info_length (u16) = 0 (offset 40..44) + buf.extend_from_slice(&0u16.to_le_bytes()); + buf.extend_from_slice(&0u16.to_le_bytes()); + // flags (u32) = 0 (offset 44) + buf.extend_from_slice(&0u32.to_le_bytes()); + // pad to data_offset - 64 + while buf.len() < (data_offset - 64) as usize { + buf.push(0); + } + // data (100 bytes) + buf.extend_from_slice(&vec![0xAB; 100]); + buf + }; + let (_, req) = SMBWriteRequest::smb_from_bytes(&bytes).unwrap(); + assert_eq!(req.write_length(), 100); + assert_eq!(req.write_offset(), 200); + assert_eq!(req.file_id().persistent(), 5); + assert_eq!(req.file_id().volatile(), 15); + assert_eq!(req.data_to_write().len(), 100); + } +} diff --git a/smb/src/server/open.rs b/smb/src/server/open.rs index 6b38665..5cef55f 100644 --- a/smb/src/server/open.rs +++ b/smb/src/server/open.rs @@ -33,6 +33,7 @@ pub trait Open: Send + Sync { fn file_id(&self) -> SMBFileId; fn file_metadata(&self) -> SMBResult; fn read_data(&mut self, offset: u64, length: u32) -> SMBResult>; + fn write_data(&mut self, offset: u64, data: &[u8]) -> SMBResult; } pub struct SMBOpen { @@ -151,6 +152,10 @@ impl Open for SMBOpen { fn read_data(&mut self, offset: u64, length: u32) -> SMBResult> { self.underlying.read_data(offset, length) } + + fn write_data(&mut self, offset: u64, data: &[u8]) -> SMBResult { + self.underlying.write_data(offset, data) + } } // TODO: From MS-FSCC section 2.6 diff --git a/smb/src/server/share/file_system.rs b/smb/src/server/share/file_system.rs index 39cba33..c4a997e 100644 --- a/smb/src/server/share/file_system.rs +++ b/smb/src/server/share/file_system.rs @@ -2,7 +2,7 @@ use std::any::Any; use std::fmt::{Debug, Formatter}; use std::fs; use std::fs::{File, OpenOptions, ReadDir}; -use std::io::{Read, Seek, SeekFrom}; +use std::io::{Read, Seek, SeekFrom, Write}; use std::marker::PhantomData; use std::path::{Component, Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -22,6 +22,7 @@ use crate::server::share::{ /// Maximum single read size (8 MB), per MS-SMB2 §3.3.5.12 recommendation for SMB 3.x. const MAX_READ_SIZE: u32 = 8 * 1024 * 1024; +const MAX_WRITE_SIZE: u32 = 8 * 1024 * 1024; /// Normalize a path by resolving `.` and `..` components lexically (without /// touching the filesystem). Returns `None` if the normalized path would @@ -145,6 +146,21 @@ impl ResourceHandle for SMBFileSystemHandle { )), } } + + fn write_data(&mut self, offset: u64, data: &[u8]) -> SMBResult { + match &mut self.resource { + SMBFileSystemResourceHandle::File(file) => { + let capped = &data[..data.len().min(MAX_WRITE_SIZE as usize)]; + file.seek(SeekFrom::Start(offset)) + .map_err(SMBError::io_error)?; + let bytes_written = file.write(capped).map_err(SMBError::io_error)?; + Ok(bytes_written as u32) + } + SMBFileSystemResourceHandle::Directory(_) => { + Err(SMBError::response_error(NTStatus::InvalidDeviceRequest)) + } + } + } } impl SMBFileSystemResourceHandle { diff --git a/smb/src/server/share/ipc.rs b/smb/src/server/share/ipc.rs index aab44e9..246b80c 100644 --- a/smb/src/server/share/ipc.rs +++ b/smb/src/server/share/ipc.rs @@ -52,6 +52,12 @@ impl ResourceHandle for SMBIPCHandle { smb_core::nt_status::NTStatus::InvalidDeviceRequest, )) } + + fn write_data(&mut self, _offset: u64, _data: &[u8]) -> SMBResult { + Err(SMBError::response_error( + smb_core::nt_status::NTStatus::InvalidDeviceRequest, + )) + } } impl From for Box { diff --git a/smb/src/server/share/mod.rs b/smb/src/server/share/mod.rs index 181f042..cdf4a65 100644 --- a/smb/src/server/share/mod.rs +++ b/smb/src/server/share/mod.rs @@ -25,6 +25,7 @@ pub trait ResourceHandle: Send + Sync { fn path(&self) -> &str; fn metadata(&self) -> SMBResult; fn read_data(&mut self, offset: u64, length: u32) -> SMBResult>; + fn write_data(&mut self, offset: u64, data: &[u8]) -> SMBResult; } pub struct SMBFileMetadata { @@ -104,6 +105,10 @@ impl ResourceHandle for Box { fn read_data(&mut self, offset: u64, length: u32) -> SMBResult> { H::read_data(self, offset, length) } + + fn write_data(&mut self, offset: u64, data: &[u8]) -> SMBResult { + H::write_data(self, offset, data) + } } pub trait SharedResource: Send + Sync { diff --git a/smb/src/server/tree_connect.rs b/smb/src/server/tree_connect.rs index f2d45d2..1c58c79 100644 --- a/smb/src/server/tree_connect.rs +++ b/smb/src/server/tree_connect.rs @@ -27,6 +27,7 @@ use crate::protocol::body::query_info::info_type::SMBInfoType; use crate::protocol::body::query_info::{SMBQueryInfoRequest, SMBQueryInfoResponse}; use crate::protocol::body::read::{SMBReadRequest, SMBReadResponse}; use crate::protocol::body::tree_connect::access_mask::SMBAccessMask; +use crate::protocol::body::write::{SMBWriteRequest, SMBWriteResponse}; use crate::protocol::header::SMBSyncHeader; use crate::protocol::message::SMBMessage; use crate::server::Server; @@ -268,6 +269,26 @@ impl SMBLockedMessageHandlerBase for Arc> { ))) } + async fn handle_write( + &mut self, + header: &SMBSyncHeader, + message: &SMBWriteRequest, + ) -> SMBResult> { + debug!(file_id = ?message.file_id(), offset = message.write_offset(), length = message.write_length(), "handling write request"); + let open = self.find_open(message.file_id()).await?; + let mut open_wr = open.write().await; + let bytes_written = open_wr.write_data(message.write_offset(), message.data_to_write())?; + drop(open_wr); + + debug!(bytes_written, "write completed"); + let response = SMBWriteResponse::new(bytes_written); + let header = header.create_response_header(0, header.session_id, header.tree_id); + Ok(SMBHandlerState::Finished(SMBMessage::new( + header, + SMBBody::WriteResponse(response), + ))) + } + async fn handle_query_info( &mut self, header: &SMBSyncHeader, From d85e6896ed2c5a823e6c57cb3c03d266f84ce57e Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 15:24:36 -0400 Subject: [PATCH 13/15] test: add E2E smbclient tests for Write handler - file_write_uploads_file: verify smbclient put writes a file to share - file_write_then_read_round_trip: verify put+get round-trips data correctly - Fix useless format! lint on existing tests - Allow zombie_processes lint at module level (tests kill server explicitly) - Fix useless vec! in write request unit test Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/src/protocol/body/write/mod.rs | 2 +- smb/tests/smbclient.rs | 180 ++++++++++++++++++++++++++++- 2 files changed, 178 insertions(+), 4 deletions(-) diff --git a/smb/src/protocol/body/write/mod.rs b/smb/src/protocol/body/write/mod.rs index b560a6c..02e9d7f 100644 --- a/smb/src/protocol/body/write/mod.rs +++ b/smb/src/protocol/body/write/mod.rs @@ -136,7 +136,7 @@ mod tests { buf.push(0); } // data (100 bytes) - buf.extend_from_slice(&vec![0xAB; 100]); + buf.extend_from_slice(&[0xAB; 100]); buf }; let (_, req) = SMBWriteRequest::smb_from_bytes(&bytes).unwrap(); diff --git a/smb/tests/smbclient.rs b/smb/tests/smbclient.rs index a936aa3..229e2a2 100644 --- a/smb/tests/smbclient.rs +++ b/smb/tests/smbclient.rs @@ -13,6 +13,9 @@ //! without the server binary. Use `cargo test --test smbclient --features server -- --ignored` //! to run them explicitly. +// Tests spawn the server and kill it at the end; we don't need to wait on exit status. +#![allow(clippy::zombie_processes)] + use std::net::TcpListener; use std::process::{Child, Command, Stdio}; use std::time::Duration; @@ -323,7 +326,7 @@ fn file_read_does_not_crash_server() { let download_str = download_path.to_str().unwrap().to_string(); let get_cmd = format!("get testfile.txt {}", download_str); let (success, stdout, stderr) = run_smbclient(&[ - &format!("//127.0.0.1/test"), + "//127.0.0.1/test", "-p", &port_str, "-U", @@ -396,7 +399,7 @@ fn directory_listing_does_not_crash_server() { let port_str = port.to_string(); let (_success, _stdout, stderr) = run_smbclient(&[ - &format!("//127.0.0.1/test"), + "//127.0.0.1/test", "-p", &port_str, "-U", @@ -448,7 +451,7 @@ fn read_nonexistent_file_returns_error() { let port_str = port.to_string(); let (success, stdout, stderr) = run_smbclient(&[ - &format!("//127.0.0.1/test"), + "//127.0.0.1/test", "-p", &port_str, "-U", @@ -480,6 +483,177 @@ fn read_nonexistent_file_returns_error() { let _ = std::fs::remove_dir_all(&tmp_dir); } +// --------------------------------------------------------------------------- +// File Write Tests +// --------------------------------------------------------------------------- + +/// Verify that smbclient can write (upload) a file to the share and that +/// the contents match what was written. +#[test] +#[ignore] +fn file_write_uploads_file() { + use std::io::Write; + + let port = free_port(); + + let tmp_dir = std::env::temp_dir().join(format!("smb_test_write_{}", port)); + std::fs::create_dir_all(&tmp_dir).expect("Failed to create temp dir"); + + // Create a source file for smbclient to upload + let source_file = tmp_dir.join("upload_source.txt"); + let source_contents = b"hello written to smb server"; + { + let mut f = std::fs::File::create(&source_file).expect("Failed to create source file"); + f.write_all(source_contents) + .expect("Failed to write source file"); + } + + let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); + let mut server = std::process::Command::new(server_bin) + .env("SMB_PORT", port.to_string()) + .env("SMB_SHARE_PATH", tmp_dir.to_str().unwrap()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("Failed to spawn SMB server binary"); + + let addr = format!("127.0.0.1:{}", port); + for _ in 0..50 { + if std::net::TcpStream::connect(&addr).is_ok() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + + let port_str = port.to_string(); + let source_str = source_file.to_str().unwrap().to_string(); + let put_cmd = format!("put {} uploaded.txt", source_str); + let (success, stdout, stderr) = run_smbclient(&[ + "//127.0.0.1/test", + "-p", + &port_str, + "-U", + "tejasmehta%password", + "-m", + "SMB2", + "-c", + &put_cmd, + ]); + + // Server should not crash + std::thread::sleep(Duration::from_millis(200)); + let status = server.try_wait().expect("Failed to check server status"); + assert!( + status.is_none(), + "Server should still be running after file write. stdout: {} stderr: {}", + stdout, + stderr + ); + + assert!( + success, + "smbclient put should succeed. stdout: {} stderr: {}", + stdout, stderr + ); + + // Verify the uploaded file exists on the server side and contents match + let uploaded_path = tmp_dir.join("uploaded.txt"); + let uploaded = std::fs::read(&uploaded_path).expect("Uploaded file should exist on server"); + assert_eq!( + uploaded, source_contents, + "Uploaded file contents should match the source" + ); + + server.kill().ok(); + let _ = std::fs::remove_dir_all(&tmp_dir); +} + +/// Verify that smbclient can write a file and then read it back, and the +/// contents round-trip correctly. +#[test] +#[ignore] +fn file_write_then_read_round_trip() { + use std::io::Write; + + let port = free_port(); + + let tmp_dir = std::env::temp_dir().join(format!("smb_test_rw_{}", port)); + std::fs::create_dir_all(&tmp_dir).expect("Failed to create temp dir"); + + // Create a source file for smbclient to upload + let source_file = tmp_dir.join("rw_source.txt"); + let source_contents = b"round-trip test data"; + { + let mut f = std::fs::File::create(&source_file).expect("Failed to create source file"); + f.write_all(source_contents) + .expect("Failed to write source file"); + } + + let server_bin = env!("CARGO_BIN_EXE_spin_server_up"); + let mut server = std::process::Command::new(server_bin) + .env("SMB_PORT", port.to_string()) + .env("SMB_SHARE_PATH", tmp_dir.to_str().unwrap()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("Failed to spawn SMB server binary"); + + let addr = format!("127.0.0.1:{}", port); + for _ in 0..50 { + if std::net::TcpStream::connect(&addr).is_ok() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + + let port_str = port.to_string(); + let source_str = source_file.to_str().unwrap().to_string(); + let download_path = tmp_dir.join("rw_downloaded.txt"); + let download_str = download_path.to_str().unwrap().to_string(); + + // Upload, then download in a single smbclient session + let cmd = format!( + "put {} rw_remote.txt; get rw_remote.txt {}", + source_str, download_str + ); + let (success, stdout, stderr) = run_smbclient(&[ + "//127.0.0.1/test", + "-p", + &port_str, + "-U", + "tejasmehta%password", + "-m", + "SMB2", + "-c", + &cmd, + ]); + + // Server should not crash + std::thread::sleep(Duration::from_millis(200)); + let status = server.try_wait().expect("Failed to check server status"); + assert!( + status.is_none(), + "Server should still be running after put+get. stdout: {} stderr: {}", + stdout, + stderr + ); + + assert!( + success, + "smbclient put+get should succeed. stdout: {} stderr: {}", + stdout, stderr + ); + + let downloaded = std::fs::read(&download_path).expect("Downloaded file should exist"); + assert_eq!( + downloaded, source_contents, + "Downloaded file contents should match the originally written data" + ); + + server.kill().ok(); + let _ = std::fs::remove_dir_all(&tmp_dir); +} + // --------------------------------------------------------------------------- // Echo Tests // --------------------------------------------------------------------------- From f72b5682d83ce285887e47929f981cd8ecdd55af Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 15:49:03 -0400 Subject: [PATCH 14/15] test: remove #[ignore] from smbclient integration tests All 12 smbclient E2E tests pass with the Write handler implementation. Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/tests/smbclient.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/smb/tests/smbclient.rs b/smb/tests/smbclient.rs index 229e2a2..6d07ef0 100644 --- a/smb/tests/smbclient.rs +++ b/smb/tests/smbclient.rs @@ -73,7 +73,6 @@ fn run_smbclient(args: &[&str]) -> (bool, String, String) { /// should succeed — indicated by smbclient progressing past the initial /// connection phase. #[test] -#[ignore] fn negotiate_completes() { let port = free_port(); let mut server = spawn_server(port); @@ -106,7 +105,6 @@ fn negotiate_completes() { /// Verify that the server rejects connections with an unsupported dialect /// gracefully (no crash). #[test] -#[ignore] fn server_does_not_crash_on_smb1_only() { let port = free_port(); let mut server = spawn_server(port); @@ -146,7 +144,6 @@ fn server_does_not_crash_on_smb1_only() { /// succeeds depends on the auth configuration, but the server should not /// crash. #[test] -#[ignore] fn session_setup_with_credentials() { let port = free_port(); let mut server = spawn_server(port); @@ -178,7 +175,6 @@ fn session_setup_with_credentials() { /// Verify that anonymous (no-auth) session setup is handled. #[test] -#[ignore] fn session_setup_anonymous() { let port = free_port(); let mut server = spawn_server(port); @@ -217,7 +213,6 @@ fn session_setup_anonymous() { /// reject it (e.g. due to signing issues) but should respond with a /// proper NT status, not crash. #[test] -#[ignore] fn tree_connect_to_share() { let port = free_port(); let mut server = spawn_server(port); @@ -249,7 +244,6 @@ fn tree_connect_to_share() { /// Verify that tree connect to a nonexistent share returns an error. #[test] -#[ignore] fn tree_connect_nonexistent_share() { let port = free_port(); let mut server = spawn_server(port); @@ -286,7 +280,6 @@ fn tree_connect_nonexistent_share() { /// Expected: The server handles Create, Read, QueryInfo, and Close /// without crashing. smbclient should be able to retrieve file contents. #[test] -#[ignore] fn file_read_does_not_crash_server() { use std::io::Write; @@ -365,7 +358,6 @@ fn file_read_does_not_crash_server() { /// Verify that smbclient can list files (which triggers QueryInfo). #[test] -#[ignore] fn directory_listing_does_not_crash_server() { use std::io::Write; @@ -425,7 +417,6 @@ fn directory_listing_does_not_crash_server() { /// Verify that reading a nonexistent file returns an error without crashing. #[test] -#[ignore] fn read_nonexistent_file_returns_error() { let port = free_port(); @@ -490,7 +481,6 @@ fn read_nonexistent_file_returns_error() { /// Verify that smbclient can write (upload) a file to the share and that /// the contents match what was written. #[test] -#[ignore] fn file_write_uploads_file() { use std::io::Write; @@ -571,7 +561,6 @@ fn file_write_uploads_file() { /// Verify that smbclient can write a file and then read it back, and the /// contents round-trip correctly. #[test] -#[ignore] fn file_write_then_read_round_trip() { use std::io::Write; @@ -663,7 +652,6 @@ fn file_write_then_read_round_trip() { /// Note: smbclient doesn't have a direct "echo" command, but we can /// verify the server stays alive through multiple operations. #[test] -#[ignore] fn server_survives_multiple_connections() { let port = free_port(); let mut server = spawn_server(port); From 8e7cbea6b17c7fa064b3f47fccf7abe44778964e Mon Sep 17 00:00:00 2001 From: "tejas-claude-bot[bot]" <273638023+tejas-claude-bot[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 21:19:58 -0400 Subject: [PATCH 15/15] fix: use write_all to prevent short writes and add write_data tests Replace file.write() with write_all() so all bytes are guaranteed to be written instead of silently accepting partial writes. Add unit tests for write_data covering basic writes, offset writes, size capping, directory error handling, and write-then-read round trips. Co-Authored-By: Claude Opus 4.6 (1M context) --- smb/src/server/share/file_system.rs | 126 +++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 2 deletions(-) diff --git a/smb/src/server/share/file_system.rs b/smb/src/server/share/file_system.rs index c4a997e..e70079c 100644 --- a/smb/src/server/share/file_system.rs +++ b/smb/src/server/share/file_system.rs @@ -153,8 +153,8 @@ impl ResourceHandle for SMBFileSystemHandle { let capped = &data[..data.len().min(MAX_WRITE_SIZE as usize)]; file.seek(SeekFrom::Start(offset)) .map_err(SMBError::io_error)?; - let bytes_written = file.write(capped).map_err(SMBError::io_error)?; - Ok(bytes_written as u32) + file.write_all(capped).map_err(SMBError::io_error)?; + Ok(capped.len() as u32) } SMBFileSystemResourceHandle::Directory(_) => { Err(SMBError::response_error(NTStatus::InvalidDeviceRequest)) @@ -497,4 +497,126 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + + #[test] + fn write_data_writes_all_bytes() { + let dir = std::env::temp_dir().join("smb_test_write_all"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("write_test.bin"); + // Create the file first + std::fs::write(&path, b"").unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + let data = vec![0xCC; 4096]; + let written = handle.write_data(0, &data).unwrap(); + assert_eq!(written, 4096); + + // Verify the file contents + let contents = std::fs::read(&path).unwrap(); + assert_eq!(contents, data); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn write_data_at_offset() { + let dir = std::env::temp_dir().join("smb_test_write_offset"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("offset_write.bin"); + std::fs::write(&path, vec![0xAA; 100]).unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + let patch = vec![0xBB; 10]; + let written = handle.write_data(50, &patch).unwrap(); + assert_eq!(written, 10); + + let contents = std::fs::read(&path).unwrap(); + assert_eq!(&contents[..50], &[0xAA; 50]); + assert_eq!(&contents[50..60], &[0xBB; 10]); + assert_eq!(&contents[60..], &[0xAA; 40]); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn write_data_capped_at_max_write_size() { + let dir = std::env::temp_dir().join("smb_test_write_cap"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("cap_write.bin"); + std::fs::write(&path, b"").unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + // Write a small amount — just verify capping logic doesn't break small writes + let data = vec![0xDD; 64]; + let written = handle.write_data(0, &data).unwrap(); + assert_eq!(written, 64); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn write_data_directory_returns_error() { + let dir = std::env::temp_dir().join("smb_test_write_dir"); + std::fs::create_dir_all(&dir).unwrap(); + + let mut handle = SMBFileSystemHandle { + path: dir.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::directory(dir.to_str().unwrap()).unwrap(), + }; + + let result = handle.write_data(0, &[0xFF; 10]); + assert!(result.is_err()); + + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn write_then_read_round_trip() { + let dir = std::env::temp_dir().join("smb_test_write_read_rt"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("round_trip.bin"); + std::fs::write(&path, b"").unwrap(); + + let mut handle = SMBFileSystemHandle { + path: path.to_string_lossy().into(), + resource: SMBFileSystemResourceHandle::file( + path.to_str().unwrap(), + SMBCreateDisposition::Open, + ) + .unwrap(), + }; + + let data: Vec = (0..256).map(|i| i as u8).collect(); + let written = handle.write_data(0, &data).unwrap(); + assert_eq!(written, 256); + + let read_back = handle.read_data(0, 256).unwrap(); + assert_eq!(read_back, data); + + std::fs::remove_dir_all(&dir).unwrap(); + } }