fix(ffi): unsound FreeContextBuffer (#141)

ffi/src/common.rs

@@ -223,14 +223,8 @@ pub type VerifySignatureFn = extern "system" fn(PCtxtHandle, PSecBufferDesc, u32
 #[cfg_attr(windows, rename_symbol(to = "Rust_FreeContextBuffer"))]
 #[no_mangle]
 pub unsafe extern "system" fn FreeContextBuffer(pv_context_buffer: *mut c_void) -> SecurityStatus {
-    // FIXME(SAFETY): this is unsound because `c_void` does not have the same memory layout as the original types.
-    // It’s likely we need to use `libc::malloc` and `libc::free` instead of Rust’s default global allocator.
-    // Indeed, libc allocator is writing a metadata chunk in front of the user data chunk, and this
-    // metadata chunk is used to find the size of the user data chunk so that it’s possible to free the
-    // memory using a void pointer. In Rust, knowledge of the original type is required. Alternatively,
-    // we could allocate a global table holding type metadata so we can cast back into the original type at this point.
-    #[allow(clippy::from_raw_with_void_ptr)]
-    let _ = Box::from_raw(pv_context_buffer as *mut _);
+    // NOTE: see https://github.com/Devolutions/sspi-rs/pull/141 for rationale behind libc usage.
+    libc::free(pv_context_buffer);
 
     0
 }

ffi/src/sec_buffer.rs

@@ -1,4 +1,3 @@
-use std::alloc::{alloc, Layout};
 use std::slice::{from_raw_parts, from_raw_parts_mut};
 
 use libc::c_char;
@@ -47,8 +46,7 @@ pub(crate) unsafe fn copy_to_c_sec_buffer(to_buffers: PSecBuffer, from_buffers:
         to_buffers[i].cb_buffer = buffer_size.try_into().unwrap();
         to_buffers[i].buffer_type = buffer.buffer_type.to_u32().unwrap();
         if allocate || to_buffers[i].pv_buffer.is_null() {
-            let memory_layout = Layout::from_size_align_unchecked(buffer_size, 8);
-            to_buffers[i].pv_buffer = alloc(memory_layout) as *mut c_char;
+            to_buffers[i].pv_buffer = libc::malloc(buffer_size) as *mut c_char;
         }
         let to_buffer = from_raw_parts_mut(to_buffers[i].pv_buffer, buffer_size);
         to_buffer.copy_from_slice(from_raw_parts(buffer.buffer.as_ptr() as *const c_char, buffer_size));

ffi/src/sec_pkg_info.rs

@@ -1,4 +1,3 @@
-use std::alloc::{alloc, Layout};
 use std::ffi::CStr;
 use std::mem::size_of;
 use std::ptr::copy_nonoverlapping;
@@ -42,8 +41,7 @@ impl From<PackageInfo> for &mut SecPkgInfoW {
         let raw_pkg_info;
         let pkg_info_w;
         unsafe {
-            let memory_layout = Layout::from_size_align_unchecked(size, 8);
-            raw_pkg_info = alloc(memory_layout);
+            raw_pkg_info = libc::malloc(size);
             pkg_info_w = (raw_pkg_info as *mut SecPkgInfoW).as_mut().unwrap();
         }
 
@@ -102,8 +100,13 @@ impl From<PackageInfo> for &mut SecPkgInfoA {
         let pkg_info_a;
 
         unsafe {
-            let memory_layout = Layout::from_size_align_unchecked(size, 8);
-            raw_pkg_info = alloc(memory_layout);
+            raw_pkg_info = libc::malloc(size);
+
+            // FIXME(safety): it is illegal to construct a reference to uninitialized data
+            // Useful references:
+            // - https://doc.rust-lang.org/nomicon/unchecked-uninit.html
+            // - https://doc.rust-lang.org/core/mem/union.MaybeUninit.html#initializing-a-struct-field-by-field
+            // NOTE: this is not the only place that needs to be fixed. An audit is required.
             pkg_info_a = (raw_pkg_info as *mut SecPkgInfoA).as_mut().unwrap();
         }
 
@@ -165,8 +168,7 @@ pub unsafe extern "system" fn EnumerateSecurityPackagesA(
             size += package.name.as_ref().as_bytes().len() + package.comment.as_bytes().len();
         }
 
-        let memory_layout = Layout::from_size_align_unchecked(size, 8);
-        let raw_packages = alloc(memory_layout);
+        let raw_packages = libc::malloc(size);
 
         let mut package_ptr = raw_packages as *mut SecPkgInfoA;
         let mut data_ptr = raw_packages.add(size_of::<SecPkgInfoA>() * packages.len()) as *mut SecChar;
@@ -236,8 +238,7 @@ pub unsafe extern "system" fn EnumerateSecurityPackagesW(
             comments.push(comment);
         }
 
-        let memory_layout = Layout::from_size_align_unchecked(size, 8);
-        let raw_packages = alloc(memory_layout);
+        let raw_packages = libc::malloc(size);
 
         let mut package_ptr = raw_packages as *mut SecPkgInfoW;
         let mut data_ptr = raw_packages.add(size_of::<SecPkgInfoW>() * packages.len()) as *mut SecWChar;

tools/sspi-ffi-attacker/Cargo.toml

@@ -3,7 +3,8 @@ name = "sspi-ffi-attacker"
 version = "0.1.0"
 edition = "2021"
 
-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+[profile.release]
+debug = 1
 
 [dependencies]
 libc = "0.2.137"

tools/sspi-ffi-attacker/src/attack.rs

@@ -43,7 +43,7 @@ pub unsafe fn init_a_table(path_to_library: &str) -> PSecurityFunctionTableA {
     security_table
 }
 
-pub unsafe fn attac_auth_identity(path_to_dll: &str) {
+pub unsafe fn attack_auth_identity(path_to_dll: &str) {
     let sspi_handle = load_library(path_to_dll);
 
     if sspi_handle.is_null() {

tools/sspi-ffi-attacker/src/main.rs

@@ -5,7 +5,7 @@ mod utils;
 use std::env;
 use std::path::Path;
 
-use attack::{attac_auth_identity, attack_a, attack_w, init_a_table, init_w_table};
+use attack::{attack_a, attack_auth_identity, attack_w, init_a_table, init_w_table};
 
 fn main() {
     let args: Vec<String> = env::args().collect();
@@ -29,6 +29,6 @@ fn main() {
         attack_a(sec_a_table);
         let _ = Box::from_raw(sec_a_table);
 
-        attac_auth_identity(path_to_library);
+        attack_auth_identity(path_to_library);
     }
 }