refactor(ffi): enable lints for unsafe code (#430)

Author: TheBestTvarynka

ffi/src/dpapi/api.rs

@@ -20,8 +20,7 @@ mod inner {
     use dpapi::{CryptProtectSecretArgs, CryptUnprotectSecretArgs, Result};
     use dpapi_transport::{ProxyOptions, Transport};
     use ffi_types::{Dword, LpByte, LpCStr, LpCUuid, LpDword};
-    use sspi::network_client::AsyncNetworkClient;
-    use sspi::{KerberosConfig, Secret};
+    use sspi::Secret;
     use url::Url;
     use uuid::Uuid;
 

ffi/src/dpapi/mod.rs

@@ -182,7 +182,7 @@ pub unsafe extern "system" fn DpapiProtectSecret(
             return NTE_INTERNAL_ERROR;
         }
 
-        // SAFETY: Memory allocation should be safe. Moreover, we check for the null value below.
+        // SAFETY: Memory allocation is safe. Moreover, we check for the null value below.
         let blob_buf = unsafe { libc::malloc(blob_data.len()) as *mut u8 };
         if blob_buf.is_null() {
             error!("Failed to allocate memory for the output DPAPI blob: blob buf pointer is NULL");
@@ -327,7 +327,7 @@ pub unsafe extern "system" fn DpapiUnprotectSecret(
             return NTE_INTERNAL_ERROR;
         }
 
-        // SAFETY: Memory allocation should be safe. Moreover, we check for the null value below.
+        // SAFETY: Memory allocation is safe. Moreover, we check for the null value below.
         let secret_buf = unsafe { libc::malloc(secret_data.as_ref().len()) as *mut u8 };
         if secret_buf.is_null() {
             error!("Failed to allocate memory for the output DPAPI blob: blob buf pointer is NULL.");

ffi/src/lib.rs

@@ -1,6 +1,8 @@
 #![allow(clippy::missing_safety_doc)]
 #![allow(clippy::print_stdout)]
 #![allow(non_snake_case)]
+#![deny(unsafe_op_in_unsafe_fn)]
+#![warn(clippy::undocumented_unsafe_blocks)]
 
 #[macro_use]
 extern crate tracing;
@@ -13,6 +15,4 @@ pub mod logging;
 pub mod sspi;
 mod utils;
 #[cfg(feature = "scard")]
-#[deny(unsafe_op_in_unsafe_fn)]
-#[warn(clippy::undocumented_unsafe_blocks)]
 pub mod winscard;

ffi/src/sspi/common.rs

@@ -3,8 +3,8 @@ use std::slice::{from_raw_parts, from_raw_parts_mut};
 use libc::{c_ulonglong, c_void};
 use num_traits::cast::{FromPrimitive, ToPrimitive};
 use sspi::{
-    BufferType, DataRepresentation, DecryptionFlags, EncryptionFlags, ErrorKind, SecurityBuffer, SecurityBufferRef,
-    SecurityBufferType, ServerRequestFlags, Sspi,
+    BufferType, DataRepresentation, DecryptionFlags, EncryptionFlags, Error, ErrorKind, SecurityBuffer,
+    SecurityBufferRef, SecurityBufferType, ServerRequestFlags, Sspi,
 };
 #[cfg(windows)]
 use symbol_rename_macro::rename_symbol;
@@ -25,10 +25,15 @@ use crate::utils::into_raw_ptr;
 pub unsafe extern "system" fn FreeCredentialsHandle(ph_credential: PCredHandle) -> SecurityStatus {
     check_null!(ph_credential);
 
-    let cred_handle = (*ph_credential).dw_lower as *mut CredentialsHandle;
+    // SAFETY: `ph_credentials` is not null. We've checked for this above.
+    let cred_handle = unsafe { (*ph_credential).dw_lower as *mut CredentialsHandle };
     check_null!(cred_handle);
 
-    let _cred_handle = Box::from_raw(cred_handle);
+    // SAFETY: `cred_handle` is not null. We've checked for this above.
+    // We create and allocate credentials handles using `Box::into_raw`. Thus,
+    // it is safe to deallocate them using `Box::from_raw`.
+    // The user have to ensure that the credentials handle was created by us.
+    let _cred_handle = unsafe { Box::from_raw(cred_handle) };
 
     0
 }
@@ -58,25 +63,40 @@ pub unsafe extern "system" fn AcceptSecurityContext(
         check_null!(p_output);
         check_null!(pf_context_attr);
 
-        let credentials_handle = (*ph_credential).dw_lower as *mut CredentialsHandle;
+        // SAFETY: `ph_credentials` is not null. We've checked for this above.
+        let credentials_handle = unsafe { (*ph_credential).dw_lower as *mut CredentialsHandle };
 
-        let (auth_data, security_package_name, attributes) = match transform_credentials_handle(credentials_handle) {
-            Some(data) => data,
-            None => return ErrorKind::InvalidHandle.to_u32().unwrap(),
+        // SAFETY: It's safe to call the function, because it has internal null check and proper error handling.
+        let (auth_data, security_package_name, attributes) = unsafe {
+            match transform_credentials_handle(credentials_handle) {
+                Some(data) => data,
+                None => return ErrorKind::InvalidHandle.to_u32().unwrap(),
+            }
         };
 
-        let sspi_context_ptr = try_execute!(p_ctxt_handle_to_sspi_context(
+        // SAFETY: It's safe to call the function, because:
+        // *`ph_context` can be null;
+        // * the value behind `ph_context` must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+        // * other parameters are type checked.
+        let mut sspi_context_ptr = try_execute!(unsafe { p_ctxt_handle_to_sspi_context(
             &mut ph_context,
             Some(security_package_name),
             attributes,
-        ));
-
-        let sspi_context = sspi_context_ptr
-            .as_mut()
-            .expect("security context pointer cannot be null");
-
-        let mut input_tokens =
-            p_sec_buffers_to_security_buffers(from_raw_parts((*p_input).p_buffers, (*p_input).c_buffers as usize));
+        )});
+
+        // SAFETY: It's safe to call the `as_mut` function, because `sspi_context_ptr` is a local pointer,
+        // which is initialized by the `p_ctx_handle_to_sspi_context` function. Thus, the value behind this pointer is valid.
+        let sspi_context = unsafe { sspi_context_ptr.as_mut() };
+
+        // SAFETY: `p_input` is not null. We've checked for this above. Additionally, we check `p_buffers` for null.
+        // All other guarantees must be provided by user.
+        let mut input_tokens = try_execute!(unsafe {
+                if (*p_input).p_buffers.is_null() {
+                    Err(Error::new(ErrorKind::InvalidParameter, "p_buffers cannot be null"))
+                } else {
+                    Ok(p_sec_buffers_to_security_buffers(from_raw_parts((*p_input).p_buffers, (*p_input).c_buffers as usize)))
+                }
+        });
 
         let mut output_tokens = vec![SecurityBuffer::new(Vec::with_capacity(1024), BufferType::Token)];
 
@@ -88,12 +108,18 @@ pub unsafe extern "system" fn AcceptSecurityContext(
             .with_output(&mut output_tokens)
             .execute(sspi_context);
 
-        copy_to_c_sec_buffer((*p_output).p_buffers, &output_tokens, false);
+        // SAFETY: `p_output` is not null. We've checked this above.
+        try_execute!(unsafe { copy_to_c_sec_buffer((*p_output).p_buffers, &output_tokens, false) });
 
-        (*ph_new_context).dw_lower = sspi_context_ptr as c_ulonglong;
-        (*ph_new_context).dw_upper = into_raw_ptr(security_package_name.to_owned()) as c_ulonglong;
+        // SAFETY: `ph_new_context` is not null. We've checked this above.
+        let ph_new_context = unsafe { ph_new_context.as_mut() }.expect("ph_new_context should not be null");
 
-        *pf_context_attr = f_context_req;
+        ph_new_context.dw_lower = sspi_context_ptr.as_ptr() as c_ulonglong;
+        ph_new_context.dw_upper = into_raw_ptr(security_package_name.to_owned()) as c_ulonglong;
+        // SAFETY: `pf_context_attr` is not null. We've checked this above.
+        unsafe {
+            *pf_context_attr = f_context_req;
+        }
 
         let result = try_execute!(result_status);
         result.status.to_u32().unwrap()
@@ -123,16 +149,27 @@ pub unsafe extern "system" fn CompleteAuthToken(
         check_null!(ph_context);
         check_null!(p_token);
 
-        let sspi_context = try_execute!(p_ctxt_handle_to_sspi_context(
+        // SAFETY: `p_token` is not null. We've checked this above.
+        unsafe { check_null!((*p_token).p_buffers); }
+
+        // SAFETY: It's safe to call the function, because:
+        // *`ph_context` can be null;
+        // * the value behind `ph_context` must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+        // * other parameters are type checked.
+        let mut sspi_context_ptr = try_execute!(unsafe { p_ctxt_handle_to_sspi_context(
             &mut ph_context,
             None,
             &CredentialsAttributes::default()
-        ))
-        .as_mut()
-        .expect("security context pointer cannot be null");
+        )});
 
-        let raw_buffers = from_raw_parts((*p_token).p_buffers, (*p_token).c_buffers as usize);
-        let mut buffers = p_sec_buffers_to_security_buffers(raw_buffers);
+        // SAFETY: It's safe to call the `as_mut` function, because `sspi_context_ptr` is a local pointer,
+        // which is initialized by the `p_ctx_handle_to_sspi_context` function. Thus, the value behind this pointer is valid.
+        let sspi_context = unsafe { sspi_context_ptr.as_mut() };
+
+        // SAFETY: This function is safe to call because `p_buffers` is not null. We've checked this above.
+        let raw_buffers = unsafe { from_raw_parts((*p_token).p_buffers, (*p_token).c_buffers as usize) };
+        // SAFETY: This function is safe to call because `raw_buffers` is type checked. All other guarantees must be provided by user.
+        let mut buffers = unsafe { p_sec_buffers_to_security_buffers(raw_buffers) };
 
         sspi_context.complete_auth_token(&mut buffers).map_or_else(
             |err| err.error_type.to_u32().unwrap(),
@@ -150,14 +187,29 @@ pub unsafe extern "system" fn DeleteSecurityContext(mut ph_context: PCtxtHandle)
     catch_panic!(
         check_null!(ph_context);
 
-        let _context: Box<SspiHandle> = Box::from_raw(try_execute!(p_ctxt_handle_to_sspi_context(
+        // SAFETY: It's safe to call the function, because:
+        // * the value behind `ph_context` must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+        // * other parameters are type checked.
+        let mut sspi_context_ptr = try_execute!(unsafe { p_ctxt_handle_to_sspi_context(
             &mut ph_context,
             None,
             &CredentialsAttributes::default()
-        )));
+        )});
+
+        // SAFETY: It's safe to constructs a box from a raw pointer because:
+        // * the `sspi_context_ptr` is not null;
+        // * the value behind `sspi_context_ptr` must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+        let _context: Box<SspiHandle> = unsafe {
+            Box::from_raw(sspi_context_ptr.as_mut())
+        };
 
-        if (*ph_context).dw_upper != 0 {
-            let _name: Box<String> = Box::from_raw((*ph_context).dw_upper as *mut String);
+        // SAFETY: `ph_context` is not null. We've checked for it above.
+        let dw_upper = unsafe { (*ph_context).dw_upper };
+        if dw_upper != 0 {
+            // SAFETY: It's safe to constructs a box from a raw pointer because:
+            // * the `dw_upper` is not equal to zero;
+            // * the value behind `dw_upper` pointer must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+            let _name: Box<String> = unsafe { Box::from_raw(dw_upper as *mut String) };
         }
 
         0
@@ -226,7 +278,12 @@ pub type VerifySignatureFn = extern "system" fn(PCtxtHandle, PSecBufferDesc, u32
 #[no_mangle]
 pub unsafe extern "system" fn FreeContextBuffer(pv_context_buffer: *mut c_void) -> SecurityStatus {
     // NOTE: see https://github.com/Devolutions/sspi-rs/pull/141 for rationale behind libc usage.
-    libc::free(pv_context_buffer);
+    // SAFETY: Memory deallocation is safe.
+    // The user must call this function to free buffers allocated by ourself. On our side, we always use `malloc`
+    // to allocate buffers in in FFI.
+    unsafe {
+        libc::free(pv_context_buffer);
+    }
 
     0
 }
@@ -270,25 +327,43 @@ pub unsafe extern "system" fn EncryptMessage(
         check_null!(ph_context);
         check_null!(p_message);
 
-        let sspi_context = try_execute!(p_ctxt_handle_to_sspi_context(
+        // SAFETY: `p_message` is not null. We've checked this above.
+        unsafe { check_null!((*p_message).p_buffers); }
+
+        // SAFETY: It's safe to call the function, because:
+        // *`ph_context` can be null;
+        // * the value behind `ph_context` must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+        // * other parameters are type checked.
+        let mut sspi_context_ptr = try_execute!(unsafe { p_ctxt_handle_to_sspi_context(
             &mut ph_context,
             None,
             &CredentialsAttributes::default()
-        ))
-        .as_mut()
-        .expect("security context pointer cannot be null");
+        )});
+
+        // SAFETY: It's safe to call the `as_mut` function, because `sspi_context_ptr` is a local pointer,
+        // which is initialized by the `p_ctx_handle_to_sspi_context` function. Thus, the value behind this pointer is valid.
+        let sspi_context = unsafe { sspi_context_ptr.as_mut() };
 
-        let len = (*p_message).c_buffers as usize;
-        let raw_buffers = from_raw_parts((*p_message).p_buffers, len);
-        let mut message = try_execute!(p_sec_buffers_to_decrypt_buffers(raw_buffers));
+        // SAFETY: `p_message` is not null. We've checked this above.
+        let len = unsafe { (*p_message).c_buffers as usize };
+
+        // SAFETY: `p_message` is not null. We've checked this above. Moreover, we've checked `p_buffers` for null above.
+        let raw_buffers = unsafe {
+            from_raw_parts((*p_message).p_buffers, len)
+        };
+
+        // SAFETY: The user must provide guarantees about the correctness of buffers in `raw_buffers'.
+        let mut message = try_execute!(unsafe { p_sec_buffers_to_decrypt_buffers(raw_buffers)});
 
         let result_status = sspi_context.encrypt_message(
             EncryptionFlags::from_bits(f_qop.try_into().unwrap()).unwrap(),
             &mut message,
             message_seq_no.try_into().unwrap(),
         );
 
-        try_execute!(copy_decrypted_buffers((*p_message).p_buffers, message));
+        // SAFETY: `p_message` and `p_buffers` are not null. We've checked this above.
+        // All other guarantees must be provided by user.
+        try_execute!(unsafe { copy_decrypted_buffers((*p_message).p_buffers, message) });
 
         let result = try_execute!(result_status);
         result.to_u32().unwrap()
@@ -311,28 +386,44 @@ pub unsafe extern "system" fn DecryptMessage(
         check_null!(ph_context);
         check_null!(p_message);
 
-        let sspi_context = try_execute!(p_ctxt_handle_to_sspi_context(
+        // SAFETY: `p_message` is not null. We've checked this above.
+        unsafe { check_null!((*p_message).p_buffers); }
+
+        // SAFETY: It's safe to call the function, because:
+        // *`ph_context` can be null;
+        // * the value behind `ph_context` must be initialized by ourself: the user does not have to create the [CtxHandle] values ​​themselves.
+        // * other parameters are type checked.
+        let mut sspi_context_ptr = try_execute!(unsafe { p_ctxt_handle_to_sspi_context(
             &mut ph_context,
             None,
             &CredentialsAttributes::default()
-        ))
-        .as_mut()
-        .expect("security context pointer cannot be null");
+        )});
+
+        // SAFETY: It's safe to call the `as_mut` function, because `sspi_context_ptr` is a local pointer,
+        // which is initialized by the `p_ctx_handle_to_sspi_context` function. Thus, the value behind this pointer is valid.
+        let sspi_context = unsafe { sspi_context_ptr.as_mut() };
 
-        let len = (*p_message).c_buffers as usize;
-        let raw_buffers = from_raw_parts((*p_message).p_buffers, len);
-        let mut message = try_execute!(p_sec_buffers_to_decrypt_buffers(raw_buffers));
+        // SAFETY: `p_message` is not null. We've checked this above.
+        let len = unsafe { (*p_message).c_buffers as usize };
+        // SAFETY: `p_message` and `p_buffers` is not null. We've checked this above.
+        let raw_buffers = unsafe { from_raw_parts((*p_message).p_buffers, len) };
+        // SAFETY: The user must provide guarantees about the correctness of buffers in `raw_buffers'.
+        let mut message = try_execute!(unsafe { p_sec_buffers_to_decrypt_buffers(raw_buffers) });
 
         let (decryption_flags, result_status) =
             match sspi_context.decrypt_message(&mut message, message_seq_no.try_into().unwrap()) {
                 Ok(flags) => (flags, Ok(())),
                 Err(error) => (DecryptionFlags::empty(), Err(error)),
             };
 
-        try_execute!(copy_decrypted_buffers((*p_message).p_buffers, message));
+        // SAFETY: `p_message` and `p_buffers` is not null. We've checked this above.
+        // All other guarantees must be provided by user.
+        try_execute!(unsafe { copy_decrypted_buffers((*p_message).p_buffers, message) });
         // `pf_qop` can be null if this library is used as a CredSsp security package
         if !pf_qop.is_null() {
-            *pf_qop = decryption_flags.bits().try_into().unwrap();
+            let flags = try_execute!(decryption_flags.bits().try_into(), ErrorKind::InternalError);
+            // SAFETY: `pf_qop` is not null. We've checked this above.
+            unsafe { *pf_qop = flags };
         }
 
         try_execute!(result_status);