Fix null DACL

src/os/windows/security_descriptor/ext.rs

@@ -51,8 +51,11 @@ Sets the ", $doc, " access control list to the specified value, assuming ownersh
 If `defaulted` is `true`, the ", $doc, " access control list is marked as having been produced by
 some default mechanism. This is only used for internal program logic and is not checked by Windows.
 
+Note that, for DACLs, a null ACL (`ptr::null_mut()`) is not the same as an unset/absent ACL: it
+actually provides ***full access*** for every security principal.
+
 # Safety
-The pointer:
+The pointer, *if not null*:
 - must point to a well-initialized ACL;
 - must not be owned elsewhere;
 - must be valid for deallocation with `LocalFree()`.

src/os/windows/security_descriptor/try_clone.rs

@@ -29,23 +29,29 @@ pub(super) unsafe fn clone(sd: *const c_void) -> io::Result<SecurityDescriptor>
 		BorrowedSecurityDescriptor::from_ptr(sd)
 	};
 
-	let acl_fn =
-		|(acl, dfl)| io::Result::<(LocalBox<ACL>, bool)>::Ok((unsafe { clone_acl(acl)? }, dfl));
-	let sid_fn = |(sid, dfl)| {
+	let clnsid = |(sid, dfl)| {
 		io::Result::<(Option<LocalBox<c_void>>, bool)>::Ok((unsafe { clone_sid(sid)? }, dfl))
 	};
-	let dacl = old_sd.dacl()?.map(acl_fn).transpose()?;
-	let sacl = old_sd.sacl()?.map(acl_fn).transpose()?;
-	let owner = sid_fn(old_sd.owner()?)?;
-	let group = sid_fn(old_sd.group()?)?;
+	let dacl = old_sd.dacl()?;
+	let sacl = old_sd.sacl()?;
+	let owner = clnsid(old_sd.owner()?)?;
+	let group = clnsid(old_sd.group()?)?;
 
 	if let Some((acl, dfl)) = dacl {
-		let mut acl = ManuallyDrop::new(acl);
-		unsafe { new_sd.set_dacl(acl.as_mut_ptr(), dfl)? };
+		if acl.is_null() {
+			unsafe { new_sd.set_dacl(ptr::null_mut(), dfl)? };
+		} else {
+			let mut acl = ManuallyDrop::new(unsafe { clone_acl(acl)? });
+			unsafe { new_sd.set_dacl(acl.as_mut_ptr(), dfl)? };
+		}
 	}
 	if let Some((acl, dfl)) = sacl {
-		let mut acl = ManuallyDrop::new(acl);
-		unsafe { new_sd.set_dacl(acl.as_mut_ptr(), dfl)? };
+		if acl.is_null() {
+			unsafe { new_sd.set_sacl(ptr::null_mut(), dfl)? };
+		} else {
+			let mut acl = ManuallyDrop::new(unsafe { clone_acl(acl)? });
+			unsafe { new_sd.set_sacl(acl.as_mut_ptr(), dfl)? };
+		}
 	}
 
 	let assid = |sid: &mut LocalBox<c_void>| sid.as_mut_ptr();

tests/os/windows/local_socket_security_descriptor.rs

@@ -1,143 +1,15 @@
-#![cfg(not(ci))]
+mod null_dacl;
+mod sd_graft;
 
-use crate::{
-	local_socket::{traits::Stream as _, Listener, ListenerOptions, Stream},
-	os::windows::{
-		local_socket::ListenerOptionsExt, AsRawHandleExt as _, AsSecurityDescriptorExt,
-		BorrowedSecurityDescriptor, LocalBox, SecurityDescriptor,
-	},
-	tests::util::*,
-	OrErrno, TryClone,
-};
-use std::{ffi::OsString, fs::File, io, mem::MaybeUninit, os::windows::prelude::*, ptr, sync::Arc};
-use widestring::{U16CStr, U16Str};
-use windows_sys::Win32::{
-	Foundation::{MAX_PATH, STATUS_SUCCESS},
-	Security::{
-		Authorization::{GetSecurityInfo, SE_KERNEL_OBJECT, SE_OBJECT_TYPE},
-		DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION,
-	},
-	System::LibraryLoader::GetModuleFileNameW,
-};
+use crate::tests::util::*;
 
-const SECINFO: u32 =
-	DACL_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION;
-
-fn get_sd(handle: BorrowedHandle<'_>, ot: SE_OBJECT_TYPE) -> TestResult<SecurityDescriptor> {
-	let mut sdptr = ptr::null_mut();
-	let errno = unsafe {
-		GetSecurityInfo(
-			handle.as_int_handle(),
-			ot,
-			SECINFO,
-			ptr::null_mut(),
-			ptr::null_mut(),
-			ptr::null_mut(),
-			ptr::null_mut(),
-			&mut sdptr,
-		) as i32
-	};
-	(errno == STATUS_SUCCESS)
-		.then_some(())
-		.ok_or_else(|| io::Error::from_raw_os_error(errno))
-		.opname("GetSecurityInfo")?;
-
-	let sdbx = unsafe { LocalBox::from_raw(sdptr) };
-	unsafe { BorrowedSecurityDescriptor::from_ptr(sdbx.as_ptr()) }
-		.to_owned_sd()
-		.opname("security descriptor clone")
-}
-
-fn count_opening_parentheses(s: &U16Str) -> u32 {
-	let mut cpa = 0;
-	for c in s.as_slice().iter().copied() {
-		if c == '(' as u16 {
-			cpa += 1;
-		}
-	}
-	cpa
-}
-
-fn ensure_equal_number_of_opening_parentheses(a: &U16Str, b: &U16Str) -> TestResult {
-	ensure_eq!(count_opening_parentheses(a), count_opening_parentheses(b));
-	Ok(())
-}
-
-fn ensure_equal_non_acl_part(a: &U16CStr, b: &U16CStr) -> TestResult<usize> {
-	let mut idx = 0;
-	for (i, (ca, cb)) in a
-		.as_slice()
-		.iter()
-		.copied()
-		.zip(b.as_slice().iter().copied())
-		.enumerate()
-	{
-		idx = i;
-		if ca == 'D' as u16 {
-			break;
-		}
-		ensure_eq!(ca, cb);
-	}
-	Ok(idx)
-}
-
-fn get_self_exe(obuf: &mut [MaybeUninit<u16>]) -> io::Result<&U16CStr> {
-	if obuf.is_empty() {
-		return Ok(Default::default());
-	}
-	let base = obuf.as_mut_ptr().cast();
-	let cap = obuf.len().try_into().unwrap_or(u32::MAX);
-	unsafe { GetModuleFileNameW(0, base, cap) != 0 }
-		.true_val_or_errno(())
-		.and_then(|()| unsafe {
-			U16CStr::from_ptr_truncate(base.cast_const(), cap as usize).map_err(io::Error::other)
-		})
-}
-
-fn test_main() -> TestResult {
-	let sd = {
-		let mut pathbuf = [MaybeUninit::uninit(); MAX_PATH as _];
-		let path: OsString = get_self_exe(&mut pathbuf)
-			.opname("query of path to own executable")?
-			.into();
-		let file = File::open(path).opname("own executable open")?;
-		get_sd(file.as_handle(), SE_KERNEL_OBJECT)
-			.opname("query of own executable's security descriptor")?
-	};
-	sd.serialize(SECINFO, |s| {
-		eprintln!("SDDL of the running executable: {}", s.display());
-	})
-	.opname("serialize")?;
-
-	let (name, listener) =
-		listen_and_pick_name(&mut namegen_local_socket(make_id!(), false), |nm| {
-			ListenerOptions::new()
-				.name(nm.borrow())
-				.security_descriptor(sd.try_clone()?)
-				.create_sync()
-		})?;
-	let _ = Stream::connect(Arc::try_unwrap(name).unwrap()).opname("client connect")?;
-
-	let listener_handle = match listener {
-		Listener::NamedPipe(l) => OwnedHandle::from(l),
-	};
-	let listener_sd =
-		get_sd(listener_handle.as_handle(), SE_KERNEL_OBJECT).opname("get listener SD")?;
-
-	sd.serialize(SECINFO, |old_s| {
-		listener_sd.serialize(SECINFO, |new_s| {
-			eprintln!("SDDL of the local socket listener: {}", new_s.display());
-			let start = ensure_equal_non_acl_part(old_s, new_s)?;
-			ensure_equal_number_of_opening_parentheses(&old_s[start..], &new_s[start..])?;
-			TestResult::Ok(())
-		})
-	})
-	.opname("serialize and check")???;
-
-	Ok(())
+#[cfg(not(ci))]
+#[test]
+fn sd_graft() -> TestResult {
+	test_wrapper(sd_graft::test_main)
 }
 
 #[test]
-fn local_socket_security_descriptor() -> TestResult {
-	test_wrapper(test_main)
+fn null_dacl() -> TestResult {
+	test_wrapper(null_dacl::test_main)
 }

tests/os/windows/local_socket_security_descriptor/null_dacl.rs

@@ -0,0 +1,25 @@
+use crate::{
+	local_socket::{prelude::*, ListenerOptions, Stream},
+	os::windows::{
+		local_socket::ListenerOptionsExt, AsSecurityDescriptorMutExt, SecurityDescriptor,
+	},
+	tests::util::*,
+	TryClone,
+};
+use std::{ptr, sync::Arc};
+
+pub(super) fn test_main() -> TestResult {
+	let mut sd = SecurityDescriptor::new().opname("security descriptor creation")?;
+	unsafe {
+		sd.set_dacl(ptr::null_mut(), false).opname("DACL setter")?;
+	}
+	let (name, _listener) =
+		listen_and_pick_name(&mut namegen_local_socket(make_id!(), false), |nm| {
+			ListenerOptions::new()
+				.name(nm.borrow())
+				.security_descriptor(sd.try_clone()?)
+				.create_sync()
+		})?;
+	let _ = Stream::connect(Arc::try_unwrap(name).unwrap()).opname("client connect")?;
+	Ok(())
+}

tests/os/windows/local_socket_security_descriptor/sd_graft.rs

@@ -0,0 +1,138 @@
+#![cfg(not(ci))]
+
+use crate::{
+	local_socket::{prelude::*, Listener, ListenerOptions, Stream},
+	os::windows::{
+		local_socket::ListenerOptionsExt, AsRawHandleExt as _, AsSecurityDescriptorExt,
+		BorrowedSecurityDescriptor, LocalBox, SecurityDescriptor,
+	},
+	tests::util::*,
+	OrErrno, TryClone,
+};
+use std::{ffi::OsString, fs::File, io, mem::MaybeUninit, os::windows::prelude::*, ptr, sync::Arc};
+use widestring::{U16CStr, U16Str};
+use windows_sys::Win32::{
+	Foundation::{MAX_PATH, STATUS_SUCCESS},
+	Security::{
+		Authorization::{GetSecurityInfo, SE_KERNEL_OBJECT, SE_OBJECT_TYPE},
+		DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION,
+	},
+	System::LibraryLoader::GetModuleFileNameW,
+};
+
+const SECINFO: u32 =
+	DACL_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION;
+
+fn get_sd(handle: BorrowedHandle<'_>, ot: SE_OBJECT_TYPE) -> TestResult<SecurityDescriptor> {
+	let mut sdptr = ptr::null_mut();
+	let errno = unsafe {
+		GetSecurityInfo(
+			handle.as_int_handle(),
+			ot,
+			SECINFO,
+			ptr::null_mut(),
+			ptr::null_mut(),
+			ptr::null_mut(),
+			ptr::null_mut(),
+			&mut sdptr,
+		) as i32
+	};
+	(errno == STATUS_SUCCESS)
+		.then_some(())
+		.ok_or_else(|| io::Error::from_raw_os_error(errno))
+		.opname("GetSecurityInfo")?;
+
+	let sdbx = unsafe { LocalBox::from_raw(sdptr) };
+	unsafe { BorrowedSecurityDescriptor::from_ptr(sdbx.as_ptr()) }
+		.to_owned_sd()
+		.opname("security descriptor clone")
+}
+
+fn count_opening_parentheses(s: &U16Str) -> u32 {
+	let mut cpa = 0;
+	for c in s.as_slice().iter().copied() {
+		if c == '(' as u16 {
+			cpa += 1;
+		}
+	}
+	cpa
+}
+
+fn ensure_equal_number_of_opening_parentheses(a: &U16Str, b: &U16Str) -> TestResult {
+	ensure_eq!(count_opening_parentheses(a), count_opening_parentheses(b));
+	Ok(())
+}
+
+fn ensure_equal_non_acl_part(a: &U16CStr, b: &U16CStr) -> TestResult<usize> {
+	let mut idx = 0;
+	for (i, (ca, cb)) in a
+		.as_slice()
+		.iter()
+		.copied()
+		.zip(b.as_slice().iter().copied())
+		.enumerate()
+	{
+		idx = i;
+		if ca == 'D' as u16 {
+			break;
+		}
+		ensure_eq!(ca, cb);
+	}
+	Ok(idx)
+}
+
+fn get_self_exe(obuf: &mut [MaybeUninit<u16>]) -> io::Result<&U16CStr> {
+	if obuf.is_empty() {
+		return Ok(Default::default());
+	}
+	let base = obuf.as_mut_ptr().cast();
+	let cap = obuf.len().try_into().unwrap_or(u32::MAX);
+	unsafe { GetModuleFileNameW(0, base, cap) != 0 }
+		.true_val_or_errno(())
+		.and_then(|()| unsafe {
+			U16CStr::from_ptr_truncate(base.cast_const(), cap as usize).map_err(io::Error::other)
+		})
+}
+
+pub(super) fn test_main() -> TestResult {
+	let sd = {
+		let mut pathbuf = [MaybeUninit::uninit(); MAX_PATH as _];
+		let path: OsString = get_self_exe(&mut pathbuf)
+			.opname("query of path to own executable")?
+			.into();
+		let file = File::open(path).opname("own executable open")?;
+		get_sd(file.as_handle(), SE_KERNEL_OBJECT)
+			.opname("query of own executable's security descriptor")?
+	};
+	sd.serialize(SECINFO, |s| {
+		eprintln!("SDDL of the running executable: {}", s.display());
+	})
+	.opname("serialize")?;
+
+	let (name, listener) =
+		listen_and_pick_name(&mut namegen_local_socket(make_id!(), false), |nm| {
+			ListenerOptions::new()
+				.name(nm.borrow())
+				.security_descriptor(sd.try_clone()?)
+				.create_sync()
+		})?;
+	let _ = Stream::connect(Arc::try_unwrap(name).unwrap()).opname("client connect")?;
+
+	let listener_handle = match listener {
+		Listener::NamedPipe(l) => OwnedHandle::from(l),
+	};
+	let listener_sd =
+		get_sd(listener_handle.as_handle(), SE_KERNEL_OBJECT).opname("get listener SD")?;
+
+	sd.serialize(SECINFO, |old_s| {
+		listener_sd.serialize(SECINFO, |new_s| {
+			eprintln!("SDDL of the local socket listener: {}", new_s.display());
+			let start = ensure_equal_non_acl_part(old_s, new_s)?;
+			ensure_equal_number_of_opening_parentheses(&old_s[start..], &new_s[start..])?;
+			TestResult::Ok(())
+		})
+	})
+	.opname("serialize and check")???;
+
+	Ok(())
+}